Common async pitfalls—part two

Nov 28, 2020 · 7 minute read · Comments

Following on from part one, here’s some more of the most common pitfalls I’ve come across—either myself, colleagues and friends, or examples in documentation—and how to avoid them.

‘Fake’-sync is not async

If the method you are calling is synchronous, even in an async method, then call it like any other synchronous method. If you want to yield the thread, then you should use Task.Yield in most cases. For UI programming, see this note about Task.Yield from the .NET API documentation.

 1public async Task DoStuffAsync(CancellationToken cancellationToken)
 2{
 3    await SomeAsyncMethod(cancellationToken);
 4
 5    // BAD: This provides no benefit and incurs the overhead of a thread hop
 6    await Task.Run(() =>
 7    {
 8        SomeSyncMethod();
 9        return Task.CompletedTask
10    });
11
12    // GOOD: Just keep the synchronous call on the current thread
13    SomeSyncMethod();
14
15    // GOOD: If you have a long-running loop, etc, then periodically yield for maximum thread sharing
16    for (int i = 0; i < 1000; i++)
17    {
18        if (i % 100 == 0)
19        {
20            await Task.Yield();
21        }
22
23        // In some cases you will need to do CPU intensive work. This is ok but
24        // care should be taken to yield at regular intervals
25        AnExpensiveMethod();
26    }
27}

Delegates

Here’s a common pitfall when passing actions as method parameters:

 1// A standard method which accepts a callback. The expectation is that
 2// all methods execute sequentially
 3public void DoSomething(Action callback)
 4{
 5    First();
 6
 7    try
 8    {
 9        callback();
10    }
11    catch (Exception ex)
12    {
13        Trace.WriteException(ex);
14    }
15    Second();
16}
17
18// GOOD: Invoked with an explicit action
19DoSomething(() => Console.WriteLine("Hello"));
20
21// BAD: This delegate is convertible to an async void method, which is allowed to be passed
22// to a method which accepts an Action.
23DoSomething(async () =>
24{
25    await Task.Delay(5000);
26    // UH-OH! This exception will not be observed by the catch handler in DoSomething above
27    // since the caller is not aware that this method is asynchronous
28    throw new Exception("This will not be observed");
29});

The implicit type conversion from the async function to Action is, surprisingly, not a compiler error! This happens because the function doesn’t have a return value, so it’s converted to a method with an async void signature. In this example the side effects aren’t bad, but in a real application this could be terrible as it violates the expected execution contract.

Synchronization

Synchronizing asynchronous code is slightly more complicated than synchronizing synchronous code. Mostly, this is because awaiting a task will result in switching to a different thread. This means that the standard synchronization primitives, which require the same thread to acquire and release a lock, won’t work when used in an async state machine.

Therefore, you must take care to use thread safe synchronization primitives in async methods. For example, using lock, will block the current thread while your code waits to gain exclusive access. In asynchronous code, threads should only block for a short amount of time.

In general, it’s not a good idea to perform any I/O under a lock. There’s usually a much better way to synchronize access in asynchronous programming.

 1private object _lock = new object();
 2
 3// GOOD: Don't do anything expensive inside a lock
 4lock (_lock)
 5{
 6    _cache = new Cache();
 7}
 8
 9// BAD: This is performing unnecessary I/O under a lock
10lock (_lock)
11{
12    _cache = Cache.LoadFromDatabase();
13}

Lazy Initialization

Imagine you need to lazy initialize some object under a lock.

 1private object _lock = new object();
 2private bool _initialized = false;
 3private int _value;
 4
 5// BAD: This method performs blocking I/O under a lock
 6public void Initialize()
 7{
 8    if (_initialized)
 9    {
10        return _value;
11    }
12
13    lock (_lock)
14    {
15        if (!_initialized)
16        {
17            _value = RetrieveData();
18            _initialized = true;
19        }
20    }
21
22    return _value;
23}

When converting RetrieveData to run asynchronously, you might try to rewrite Initialize a few different ways:

 1// BAD: Performs I/O and blocks under a lock
 2lock (_lock)
 3{
 4    if (!_initialized)
 5    {
 6        _value = RetrieveDataAsync().SyncResult();
 7        _initialized = true;
 8    }
 9}
10
11// BAD: Fails at runtime since you cannot change threads under a lock
12lock (_lock)
13{
14    if (!_initialized)
15    {
16        _value = await RetrieveDataAsync();
17        _initialized = true;
18    }
19}

But there are a few issues:

You shouldn’t call external code under a lock. The caller has no idea what work the external code will do, or what assumptions it has made.
You shouldn’t perform I/O under a lock. Code sections under a lock should execute as quickly as possible, to reduce contention with other threads. As soon as you perform I/O under a lock, avoiding contention isn’t possible.

SemaphoreSlim

If you absolutely must perform asynchronous work which limits the number of callers, .NET provides SemaphoreSlim which support asynchronous, non-blocking, waiting.

You still need to take care when converting from a synchronous locking construct. Semaphores, unlike monitor locks, aren’t re-entrant.

 1private readonly SemaphoreSlim m_gate = new SemaphoreSlim(1, 1);
 2
 3public async Task DoThingAsync(CancellationToken cancellationToken)
 4{
 5    // Be careful, semaphores aren't re-entrant!
 6    bool acquired = false;
 7    try
 8    {
 9        acquired = m_gate.WaitAsync(cancellationToken);
10        if (acquired)
11        {
12            // Now that we have entered our mutex it is safe to work on shared data structures
13            await DoMyCriticalWorkAsync(cancellationToken);
14        }
15    }
16    finally
17    {
18        if (acquired)
19        {
20            m_gate.Release();
21        }
22    }
23}

IDisposable

IDisposible is used to finalize acquired resources. In some cases, you need to dispose of these resources asynchronously, to avoid blocking. Unfortunately, you can’t do this inside Dispose().

Thankfully, .NET Core 3.0 provides the new IAsyncDisposible interface, which allows you to handle asynchronous finalization like so:

 1class Foo : IAsyncDisposable
 2{
 3    public async Task DisposeAsync()
 4    {
 5        await SomethingAsync();
 6    }
 7}
 8
 9await using (var foo = new Foo())
10{
11    return await foo.DoSomethingAsync();
12}

IEnumerable and IEnumerator

Usually you would implement IEnumerable or IEnumerator so you can use syntactic sugar, like foreach and LINQ-to-Objects. Unfortunately, these are synchronous interfaces that can only be used on synchronous data sources. If your underlying data source is actually asynchronous, you shouldn’t expose it using these interfaces, as it will lead to blocking.

With the release of .NET Core 3.0 we got the IAsyncEnumerable and IAsyncEnumerator interfaces, which allow you to enumerate asynchronous data sources:

1await foreach (var foo in fooCollection)
2{
3    await foo.DoSomethingAsync();
4}

Prefer the compiler-generated state machine

There are some valid cases for using Task.ContinueWith, but it can introduce some subtle bugs if not used carefully. It’s much easier to avoid it, and just use async and await instead.

 1// BAD: This is using promise-based constructs
 2public Task DoThingAsync()
 3{
 4    return DoMoreThingsAsync().ContinueWith((t) => DoSomethingElse());
 5}
 6
 7// BAD: A much worse version of the above
 8public Task<int> DoThingAsync()
 9{
10    var tcs = new TaskCompletionSource<int>();
11    DoMoreThingsAsync().ContinueWith(t =>
12    {
13        var result = t.Result;
14
15        // UH-OH! The call to .Result is valid, however, it doesn't properly handle exceptions
16        // which can lead to an async call chain which never completes. This is an very difficult
17        // issue to debug as all evidence is garbage collected.
18        tcs.SetResult(result);
19    }
20
21    return tcs.Task;
22}
23
24// GOOD: This is using await
25public async Task DoThingAsync()
26{
27     await DoMoreThingsAsync();
28     DoAnotherThing();
29}

TaskCompletionSource

TaskCompletionSourc<T> allows you to support manual completion in asynchronous code. In general, this class should not be used… but when you have to use it you should be aware of the following behaviour:

 1var waiters = new List<TaskCompletionSource<int>>();
 2
 3// BAD: This uses default continuation options which result in synchronous callbacks
 4Task SomeMethod(CancellationToken cancellationToken)
 5{
 6    var tcs1 = new TaskCompletionSource<int>();
 7    waiters.Add(tcs1);
 8    return tcs1.Task;
 9}
10
11// GOOD: This uses explicit asynchronous continuations
12Task SomeMethod(CancellationToken cancellationToken)
13{
14    var tcs2 = new TaskCompletionSource<int>(TaskCreationOptions.RunContinuationsAsynchronously); 
15    waiters.Add(tcs2);
16    return tcs2.Task;
17}
18
19// The problem comes in when a caller awaits your task. The callee may have an expectation that the callbacks execute
20// quickly, but in the example below this is only true for the implementation which specifies asynchronous continuation.
21// The reason is the caller of TaskCompletionSource<T>.SetResult will be blocked for the entire duration of the loop 
22// below when using the first implementation, while in the second implementation the loop will run on a different thread
23// and the caller may continue execution quickly as expected.
24async Task SomeCaller(CancellationToken cancellationToken)
25{
26    var foo = await SomeMethod();
27    for (int i = 0; i < 10000; i++)
28    {
29        // Do something synchronous and expensive
30    }
31}

Jamie Magee