views:

170

answers:

3

I have a simple question, but I'm about 80% sure that the answer to the question will be accompanied by "you're doing it wrong," so I'm going to ask the not-simple question too.

The simple question: I have a public method of a public class. I want it to throw an exception if it's called on the UI thread. How can I do this?

The much-less-simple question is: is there an easier way to refactor this design?

Background:

I've developed a desktop program that interoperates with a legacy application via its API. The API is not remotely thread-safe. I've built a class library that encapsulates interoperation with the API (which involves marshalling and unmarshalling data in an enormous byte[] buffer and then calling an external function loaded from a DLL) so that as many implementation details of the legacy API are hidden from my code as possible. Because I knew that making multiple instances of the core API object would be a catastrophe, I implemented it as a static class.

I've also built a set of classes for running tasks in the background of my app. The TaskManager maintains a queue of Task objects and runs them using a BackgroundWorker. Using a background thread allows the desktop app's UI to remain responsive while interoperation with the turgid legacy app is going on; using a queue insures that only one task is calling the API at any given time.

Unfortunately, I never thought to build certain safeguards into this design. I've recently discovered places in the code where I was directly calling the API on the UI thread. I believe I've fixed all of them, but I'd like to guarantee I don't do this again.

If I'd designed this properly from the beginning, I'd have made the API wrapper class non-static, hidden its constructor from everything except the TaskManager, and then passed the instance of the API class to each Task when it gets created. Any method the Task called that talked to the API would need to be passed the API object. This would make it impossible to use the API on the foreground thread.

The thing is, there's a lot of code that talks to the API. Implementing this change (which I think is ultimately the right thing to do) will touch all of it. So in the meantime, I'd like to modify the API's Call method so that it throws an exception if it's being called on the foreground thread.

I know I'm solving the wrong problem. I can feel it in my bones. But I'm also pretty wrapped up in it right now and can't quite see the right solution.

Edit:

I clearly framed the question the wrong way, which is why it was hard to answer. I shouldn't be asking "How can this method know if it is running on the UI thread?" The real question is: "How can this method know if it is running on the wrong thread?" There could (in theory) be a thousand threads running. As JaredPar points out, there could be more than one UI thread. Only one thread is the right thread, and its thread ID is easy to find.

In fact, even after I refactor this code so that it's properly designed (which I mostly did today), it'll be worth having a mechanism in the API that checks to make sure it's being run on the appropriate thread.

A: 

Justin Rogers has a detailed discussion how Invoke/BeginInvoke does that, and where it gets ugly.

Basically, it falls back to finding some window on the UI thread, and calling GetWindowThreadProcessId to compare the current thread id to the window's one. (Of course, when you have but one UI thread, you can cache the thread ID).

peterchen
+1  A: 

Part of the problem with determining if you're on the UI thread or not is that there can be more than one UI thread. In WPF and WinForms it's quite possible to create more than one thread for displaying UI.

In this case though, it sounds like you have a fairly constrained scenario. The best bet is to record the Id of the UI thread or background thread in a shared location and then use Thread.CurrentThread.ManagedThreadId to ensure you're on the correct thread.

public class ThreadUtil {
  public static int UIThreadId;

  public static void EnsureNotUIThread() {
    if ( Thread.CurrentThread.ManagedThreadId == UIThreadId ) {
      throw new InvalidOperationException("Bad thread");
    }
  }
}

This approach has a couple of caveats. You must set the UIThreadId in a atomic manner and must do so before any background code runs. The best way is to probably add the following lines to your program startup

Interlocked.Exchange(ref ThreadUtil.UIThreadID, Thread.CurrentThread.ManagedThreadId);

Another trick is to look for a SynchronizationContext. Both WinForms and WPF will setup a SynchronizationContext on their UI threads in order to allow communication with background threads. For a background created and controlled by your program (i really want to stress that point) there will not be a SynchronizationContext installed unless you actually install one. So the following code can be used in that very limited circumstance

public static bool IsBackground() { 
  return null == SynchronizationContext.Current;
}
JaredPar
This is pretty close to what I actually did. What I did instead was make the Task class that calls the API set a ThreadID property on the API wrapper before it does a call. Every API call throws an exception if the current thread's ID isn't equal to this ThreadID. A UI developer could defeat this if he really wanted to, but since the exception message explicitly says "Don't call the API from the UI thread," I like to think if he runs across this, he won't. Also, the moment I turned this on, I found a half-dozen API calls in the UI code. So: win.
Robert Rossney
+1  A: 

I'd reverse the ISynchronizeInvoke use case, and throw an exception if ISynchronizeinvoke.InvokeRequired == false. That lets WinForms take care of the gory work of finding the UI thread. Performance will suck a bit, but this is a debugging scenario - so it really doesn't matter. You could even hide the check behind an #IF DEBUG flag to only check on debug builds.

You do need to give your API a reference to an ISynchronizeInvoke - but you can easily do that at startup (just pass your main Form), or let it use the static Form.ActiveForm call.

Mark Brackett