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.

Brigidabrigit

Is this re-inventing the TPL DataFlow's ActionBlock a little? – Harkey

@Andrew Not really; it's not necessarily going to be used for primarily CPU bound work, which is what TPL dataflow is there for. It's essentially a way of replicating one of the UI's sync contexts. – Entirely

@Entirely That's true. The idea though to use only a single thread to do all the labor seems to reflect it. – Harkey

@Entirely I'm not disagreeing... – Harkey

@Entirely No worries, we seem to debate frequently. – Harkey

It's not quite clear, are you trying to achieve something similar to WinFormsSynchronizationContext which always restores the original thread, on something like AspNetSynchronizationContext, which still serializes execution, but nevertheless allows a thread switch (so it scales better)? Also, what's the goal for this way of synchronizing, are you objects not thread-safe by design? I can't help myself linking my own question here. – Zilpah

@Noseratio This custom context has no thread affinity. It simply allows only one thread at a time to execute any code being protected by the context so it's more like ASP.NET. The concern is more about how this context is being used (see demo code), i.e. like a kind of monitor lock where the lock is automatically released and re-acquired before and after awaits. The objects have state and may be touched by multiple threads and so not safe. The goal is mainly an easy-to-use-and-apply pattern. – Brigidabrigit

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.

Entirely

I'd say, single threaded synchronization context makes perfect conceptual sense only if really needed, e.g. if the workflow objects are COM STA objects or UI elements. Otherwise, something like AspNetSynchronizationContext would make more sense for me. – Zilpah

The problem being addressed with setting the current context to the custom context is because we need to set the current synchronization context before awaits happen in protected code so that the compiler will cause continuations to be posted to the custom context. This could have been placed in the custom awaiter, true. It's done inside the context just so the caller doesn't have to, thus simplifying the usage pattern. – Brigidabrigit

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.

Zilpah

I've tried tight loops as you describe in several different situations and am unable to end up with any poor behaviour you wouldn't expect considering the situation. Manually posting like this is a bit contrived. – Brigidabrigit

@revane, see my edit. To take this discussion further, maybe you can update your GitHub code with the aforementioned test cases? Otherwise, no worries, as long as you are satisfied with your logic. – Zilpah