SynchronizationContext.Post(...) in transport event handler

Question

We have a method which, due to threading in the client application requires the usage of SynchronizationContext.

There is a bit of code which one of my colleagues has written which doesnt "feel" right to me, and a performance profiler is telling me that quit a lot of processing is being used in this bit of code.

void transportHelper_SubscriptionMessageReceived(object sender, SubscriptionMessageEventArgs e)
        {
            if (SynchronizationContext.Current != synchronizationContext)
            {
                synchronizationContext.Post(delegate
                     {
                         transportHelper_SubscriptionMessageReceived(sender, e);
                     }, null);

                return;
            }
  [code removed....]
}

This just doesnt feel right to me, as we are basically posting the same request to the gui thread event queue...however, I cannot see anyhting oviously problematic either, other than the performance of this area of code.

This method is an event handler attached to an event raised by our middle-tier messaging layer helper (transportHelper) and it exists within a service which handles requests from the GUI.

Does this seem like an acceptable way of making sure that we do not get cross-thread errors? If not, is there a better solution?

Thanks

Answer 1

Let's trace what's going on inside this method, and see what that tells us.

1. The method signature follows that of event handlers, and as the question indicates, we can expect it to be first called in the context of some thread that is not the UI thread.

2. The first thing the method does is to compare the SynchronizationContext of the thread it's running in with a SynchronizationContext saved in a member variable. We'll assume the saved context is that of the UI thread. (Mike Peretz posted an excellent series of introductory articles to the SynchronizationContext class on CodeProject)

3. The method will find the contexts not equal, as it is called in a thread different from the UI thread. The calling thread's context is likely to be null, where the UI thread's context is pretty much guarantied to be set to an instance of WindowsFormsSynchronizationContext. It will then issue a Post() on the UI context, passing a delegate to itself and its arguments, and return immediately. This finishes all processing on the background thread.

4. The Post() call causes the exact same method to be invoked on the UI thread. Tracing the implementation of WindowsFormsSynchronizationContext.Post() reveals that this is implemented by queueing a custom Windows message on the UI thread's message queue. Arguments are passed, but are not "marshaled", in the sense that they aren't copied or converted.

5. Our event handler method is now called again, as a result of the Post() call, with the exact same arguments. This time around, however, the thread's SynchronizationContext and the saved context are one and the same. The content of the if clause is skipped, and the [code removed] portion is executed.

Is this a good design? It's hard to say without knowing the content of the [code removed] portion. Here are some thoughts:

1. Superficially, this doesn't seem to be a horrible design. A message is received on a background thread, and is passed on to the UI thread for presentation. The caller returns immediately to do other things, and the receiver gets to continue with the task. This is somewhat similar to the Unix fork() pattern.

2. The method is recursive, in a unique way. It doesn't call itself on the same thread. Rather, it causes a different thread to invoke it. As with any recursive piece of code, we would be concerned with its termination condition. From reading the code, it appears reasonably safe to assume that it will always be invoked recursively exactly once, when passed to the UI thread. But it's another issue to be aware of. An alternative design might have passed a different method to Post(), perhaps an anonymous one, and avoid the recursion concern altogether.

3. There doesn't seem to be an obvious reason for a large amount of processing to occur inside the if clause. Reviewing the WindowsFormsSynchronizationContext implementation of Post() with the .NET reflector reveals some moderately long sequences of code in it, but nothing too fancy; It all happens in RAM, and it does not copy large amounts of data. Essentially it just prepares the arguments and queues a Windows message on the receiving thread's message queue.

4. You should review what is going on inside the [code removed] portion of the method. Code that touches UI controls totally belongs there -- it must execute inside the UI thread. However, if there is code in there that doesn't deal with UI, it might be a better idea to have it execute in the receiving thread. For example, any CPU-intensive parsing would be better hosted in the receiving thread, where it does not impact the UI responsiveness. You could just move that portion of the code above the if clause, and move the remaining code to a separate method -- to ensure neither portion gets executed twice.

5. If both the receiving thread and the UI thread need to remain responsive, e.g. both to further incoming message and to user input, you might need to introduce a third thread to process the messages before passing them to the UI thread.