Using a custom SynchronizationContext to serialize execution when using async/await

Question

My team is developing a multi-threaded application using async/await in C# 5.0. In the process of implementing thread synchronization, after several iterations, we came up with a (possibly novel?) new SynchronizationContext implementation with an internal lock that:

1. When calling Post:
   • if a lock can be taken, the delegate is executed immediately
   • if a lock cannot be taken, the delegate is queued
2. When calling Send:
   • if a lock can be taken the delegate is executed
   • if a lock cannot be taken, the thread is blocked

In all cases, before executing the delegate, the context sets itself as the current context and restores the original context when the delegate returns.

It’s an unusual pattern and since we’re clearly not the first people writing such an application I’m wondering:

1. Is the pattern really safe?
2. Are there better ways of achieving thread synchronization?

Here’s the source for SerializingSynchronizationContext and a demo on GitHub.

Here’s how it’s used:

• Each class wanting protection creates its own instance of the context like a mutex.

• The context is awaitable so that statements like the following are possible.

  await myContext;

  This simply causes the rest of the method to be run under protection of the context.

• All methods and properties of the class use this pattern to protect data. Between awaits, only one thread can run on the context at a time and so state will remain consistent. When an await is reached, the next scheduled thread is allowed to run on the context.
• The custom SynchronizationContext can be used in combination with AsyncLock if needed to maintain atomicity even with awaited expressions.
• Synchronous methods of a class can use custom context for protection as well.

Answer 1

Having a sync context that never runs more than one operation at a time is certainly not novel, and also not bad at all. Here you can see Stephen Toub describing how to make one two years ago. (In this case it's used simply as a tool to create a message pump, which actually sounds like it might be exactly what you want, but even if it's not, you could pull the sync context out of the solution and use it separately.)

It of course makes perfect conceptual sense to have a single threaded synchronization context. All of the sync contexts representing UI states are like this. The winforms, WPF, winphone, etc. sync contexts all ensure that only a single operation from that context is ever running at one time.

The one worrying bit is this:

In all cases, before executing the delegate, the context sets itself as the current context and restores the original context when the delegate returns.

I'd say that the context itself shouldn't be doing this. If the caller wants this sync context to be the current context they can set it. If they want to use it for something other than the current context you should allow them to do so. Sometimes you want to use a sync context without setting it as the current one to synchronize access to a certain resource; in such a case only operation specifically accessing that resource would need to use this context.

Answer 2

Regarding the use of locks. This question would be more appropriate for Code Review, but from the first glance I don't think your SerializingSynchronizationContext.Post is doing all right. Try calling it on a tight loop. Because of the Task.Run((Action)ProcessQueue), you'll quickly end up with more and more ThreadPool threads being blocked on lock (_lock) while waiting to acquire it inside ProcessQueue().

[EDITED] To address the comment, here is your current implementation:

public override void Post(SendOrPostCallback d, object state)
{
    _queue.Enqueue(new CallbackInfo(d, state));

    bool lockTaken = false;
    try
    {
        Monitor.TryEnter(_lock, ref lockTaken);

        if (lockTaken)
        {
            ProcessQueue();
        }
        else
        {
            Task.Run((Action)ProcessQueue);
        }
    }
    finally
    {
        if (lockTaken)
        {
            Monitor.Exit(_lock);
        }
    }
}

// ...

private void ProcessQueue()
{
    if (!_queue.IsEmpty)
    {
        lock (_lock)
        {
            var outer = SynchronizationContext.Current;
            try
            {
                SynchronizationContext.SetSynchronizationContext(this);

                CallbackInfo callback;
                while (_queue.TryDequeue(out callback))
                {
                    try
                    {
                        callback.D(callback.State);
                    }
                    catch (Exception e)
                    {
                        Console.WriteLine("Exception in posted callback on {0}: {1}", 
                            GetType().FullName, e);                 
                    }
                }
            }
            finally
            {
                SynchronizationContext.SetSynchronizationContext(outer);
            }
        }
    }
}

In Post, why enqueue a callback with _queue.Enqueue and then preoccupy a new thread from the pool with Task.Run((Action)ProcessQueue), in the situation when ProcessQueue() is already pumping the _queue in a loop on another pool thread and dispatching callbacks? In this case, Task.Run looks like wasting a pool thread to me.