Efficiency driven developer. Ex-Microsoft (ADX, AATP, ATA). Ex-Aorato (Acquired by Microsoft).

profile for i3arnon on Stack Overflow
© Bar Arnon
  • February 21, 2017 | Async/Await, Bugs, IDisposable | comments | Edit
  • The Issue With Scoped Async Synchronization Constructs
  • With async/await becoming more and more prevalent in modern code so has the need for async synchronization constructs. Unlike their synchronous counterparts (e.g. Monitor, Mutex, ReaderWriterLock, etc.) .NET doesn’t offer almost any built-in asynchronous synchronization constructs, but it does contain the basic building blocks to build them on your own (mainly Task, TaskCompletionSource<T> and SemaphoreSlim.WaitAsync). Stephen Toub published a series of posts (over 5 years ago) on the Parallel Framework team’s blog demonstrating that by building AsyncSemaphore, AsyncLock, AsyncReaderWriterLock and more.

    However, there’s an issue with most implementations of the scoped async synchronization constructs: they usually return a task.

    Let’s take AsyncLock built using a SemaphoreSlim as an example. Since a semaphore enables concurrent access up to a certain limit a semaphore with the limit of 1 is essentially a lock. Here’s a simple implementation (a more efficient one can be found here):

    class AsyncLock
    {
        private readonly SemaphoreSlim _semaphore = new SemaphoreSlim(1, 1);
    
        public async Task<IDisposable> LockAsync()
        {
            await _semaphore.WaitAsync();
            return new Releaser(_semaphore);
        }
    
        private class Releaser : IDisposable
        {
            private readonly SemaphoreSlim _semaphore;
    
            public Releaser(SemaphoreSlim semaphore)
            {
                _semaphore = semaphore;
            }
    
            public void Dispose()
            {
                _semaphore.Release();
            }
        }
    }
    

    While we could expose an API of AsyncLock.AcquireAsync and AsyncLock.Release to be used with a try-finally block:

    await _lock.AcquireAsync();
    try
    {
        // critical section...
    }
    finally
    {
        _lock.Release();
    }
    

    It’s much simpler to use (as most implementation do) a scoped API that returns a Task<IDisposable>. This allows using a using block to automatically release the lock at the end of the critical section (even when an exception is thrown inside it):

    using (await _lock.LockAsync())
    {
        // critical section...
    }
    

    The subtle issue with this approach is that most developers forget that tasks are by themselves IDisposable. That makes the following code compile perfectly well:

    using (_lock.LockAsync())
    {
        // critical section...
    }
    

    The missing await means the code doesn’t wait for the lock to be acquired and proceeds regardless so the lock doesn’t actually protect anything and the critical section can be run concurrently. Usually when you don’t await a Task inside an async method the compiler will warn you, but not in this case as the returned Task isn’t abandoned and is being used by the using block.

    It’s trivially easy to include this bug in your code (I know I have) but quite difficult to realize that once you did, especially since this will only blow up in runtime if you dispose of the task at the exact time when the lock is contended.

    My simple solution is not to return Task<IDisposable> from async synchronization constructs, but a new awaitable (TaskWrapper) that doesn’t implement IDisposable. All TaskWrapper needs to do to be an awaitable is to return a valid awaiter, and it does so by returning the TaskAwaiter for the original underlying task:

    struct TaskWrapper<T>
    {
        private readonly Task<T> _task;
    
        public TaskWrapper(Task<T> task)
        {
            _task = task;
        }
    
        public TaskAwaiter<T> GetAwaiter() => _task.GetAwaiter();
    }
    

    Since you can’t return something other than Task/Task<T>/void from async methods (at least until C# 7.0 is released supporting arbitrary async returns) it’s simpler to split LockAsync into 2 separate methods, an internal async one that actually implements the locking logic and another that wraps the returned Task<IDisposable> with a TaskWrapper<T>:

    public TaskWrapper<IDisposable> LockAsync() =>
        new TaskWrapper<IDisposable>(LockInternalAsync());
    
    private async Task<IDisposable> LockInternalAsync()
    {
        await _semaphore.WaitAsync();
    
        return new Releaser(_semaphore);
    }
    

    Doing this prevents our offensive code from compiling as TaskWrapper<IDisposable> doesn’t implement IDisposable and so can’t be used in a using statement, but awaiting it results in an IDisposable (not to be confused with Task<IDisposable>) that can and should be used in that using statement. That way you don’t need to worry whether you’ve missed an await somewhere since the compiler will do that for you.

    Next you can find a more efficient (and more complicated) implementation of AsyncLock using TaskWrapper:

    class AsyncLock
    {
        private readonly SemaphoreSlim _semaphore;
        private readonly IDisposable _releaser;
        private readonly Task<IDisposable> _releaserTask;
    
        public AsyncLock()
        {
            _semaphore = new SemaphoreSlim(1, 1);
            _releaser = new Releaser(_semaphore);
            _releaserTask = Task.FromResult(_releaser);
        }
    
        public TaskWrapper<IDisposable> LockAsync()
        {
            var waitTask = _semaphore.WaitAsync();
            return new TaskWrapper<IDisposable>(
                waitTask.IsCompleted
                    ? _releaserTask
                    : waitTask.ContinueWith(
                        (_, releaser) => (IDisposable)releaser,
                        _releaser,
                        CancellationToken.None,
                        TaskContinuationOptions.ExecuteSynchronously,
                        TaskScheduler.Default));
        }
    
        private class Releaser : IDisposable
        {
            private readonly SemaphoreSlim _semaphore;
    
            public Releaser(SemaphoreSlim semaphore)
            {
                _semaphore = semaphore;
            }
    
            public void Dispose()
            {
                _semaphore.Release();
            }
        }
    }