Implementing an asynchronous mutex in C#

Question

I wrote a simple synchronization primitive that I can use with async operations on an external REST service so that I don't call it twice from different threads and have one call fail due to not having the latest state. Despite obvious limitations of that design decision I think this should solve my problem...

struct DumbAsyncMutex
{
    int busy;

    public async Task WithMutexAsync(Func<Task> t)
    {
        while (Interlocked.CompareExchange(ref busy, 1, 0) != 0)
        {
            await Task.Delay(100);
        }

        try
        {
            await t();
        }
        finally
        {
            while (Interlocked.CompareExchange(ref busy, 0, 1) != 1)
            {
                throw new Exception("Releasing the mutex failed - but this should never happen");
            }
        }
    }
}

Of course this is a hack... did I just overlook a better way to do this using the standard framework?

Answer 1

A better alternative to the busy wait loop would be to use a Semaphore - in this case a SemaphoreSlim would probably be the most appropriate. Semaphores are not reentrant. This would simplify the implementation to:

struct DumbAsyncMutex
{
    private SemaphoreSlim _Semaphore = new SemaphoreSlim(1, 1);

    public async Task WithMutexAsync(Func<Task> t)
    {
        await _Semaphore.WaitAsync();

        try
        {
            await t();
        }
        finally
        {
            _Semaphore.Release(1);
        }
    }
}

The advantage is that it has async support and you can abandon the waiting by using cancellation tokens.

Answer 2

Naming

The following qoute from Naming Guidelines on MSDN is one of my favorites:

The goal of this chapter is to provide a consistent set of naming conventions that results in names that make immediate sense to developers.

There are a couple of alternative names that I can suggest:

• The name DumbAsyncMutex is confusing because

  1. The type it represents is not a mutex at all
  2. An asynchronous mutex does not make sense
  3. It can be changed to TaskExecutionSynchronizer
  4. I think the latter describes the funtion of the type way better because the type simply "synchronizes execution of tasks"

• The method name WithMutexAsync is not a good name and can be changed to QueueTask because that is what this method actually does, queueing tasks for synchronized execution.

• The name busy makes sense but

  1. It sounds too broad to represent a flag used for synchronization
  2. It's name can be changed to numberOfTasksInProgress.
  3. Yes, this will change the meaning of the variable from being a flag to being an integer counting ongoing parallel executions
  4. But we limit it's values to 1 or 0 anyway so it should be fine. (The number of parallel executions is either 1 or 0)

• The parameter name t

  1. Is t
  2. It can at least be changed to task.
  3. You can use your creativity to name it better, unfortunately mine refused to co-operate for a name like t

Functionality

• It is very likely that the tasks registered to execute (by await WithMutexAsync() call) will not be executed in the same order. This may or may not cause an issue, I don't know the full flow.
• That is, if a call to this method is made while a previous call is waiting at Task.Delay() and the mutex at that moment is released (busy = 0) during that delay, the latest passed Task will be executed instantly while the previously passed Task will wait for this one's execution as well. This actually introduces a FIGKWIOF type of queue. (First In God Knows Which Is Out First).
• Another important point is, you cannot pass the instance of this struct around to ensure synchronized execution of REST service calls because it is a value type (struct) and the value of the field busy will be copied and Tasks passed into that new instance will lost the synchronization context with the Tasks executed by the original instance. You can overcome this by changing the type to a class.
• Apart from that, the use of Interlocked.CompareExchange for this purpose feels like kind of abuse. Why don't you instead use Monitor.TryEnter without a timeout?
• Also, the while loop in the finally block seems completely unnecessary (is it a result of a copy/paste?). You can achieve the same behaviour by an if statement.

Answer 3

I see one possible problem with this approach namely that any exception (except for a ThreadAbortException) which is maybe thrown by the Func<Task> t is silently swallowed if the mutex couldn't be freed.

See: what happens if a finally block throws an exception

A possible workaround could be to have a catch block which stores the exception in a variable which is later passed as an innerexception to the new Exception.

---

The naming of the struct and the method parameter is not very good, because it doesn't tell a reader of the code anything about it. Try to use better names.