Common async pitfalls—part one

The .NET Framework provides a great programming model that enables high performance code using an easy to understand syntax. However, this can often give developers a false sense of security, and the language and runtime aren’t without pitfalls. Ideally static analysers, like the Microsoft.VisualStudio.Threading.Analyzers Roslyn analysers, would catch all these issues at build time. While they do help catch a lot of mistakes, they can’t catch everything, so it’s important to understand the problems and how to avoid them.

Here’s a collection of some of the most common pitfalls I’ve come across—either myself, colleagues and friends, or examples in documentation—and how to avoid them.

Blocking calls

The main benefit of asynchronous programming is that the thread pool can be smaller than a synchronous application while performing the same amount of work. However, once a piece of code begins to block threads, the resulting thread pool starvation can be ugly.

 1public async Task DoStuffAsync(CancellationToken cancellationToken)
 2{
 3    Task<bool> resultTask = StuffAsync(cancellationToken);
 4
 5    // BAD: These may deadlock depending on thread pool capacity or the presence of a sync context
 6    resultTask.Wait();
 7    resultTask.Result;
 8    resultTask.GetAwaiter().GetResult();
 9
10    // BAD: This blocks the thread
11    Thread.Sleep(5000);
12
13    // GOOD: Always await
14    await resultTask;
15
16    // GOOD: This does not block and allows for maximum throughput
17    await Task.Delay(5000, cancellationToken);
18}

If I run a small test, which makes 5000 concurrent HTTP requests to a local server, there are dramatically different results depending on how many blocking calls are used.

% blocking shows the number of calls that use Task.Result, which blocks the thread. All other requests use await.

% BlockingThreadsTotal DurationAvg. Duration
02400:00:11.9610.0023923
526800:02:16.5740.0273148

The increased total duration when using blocking calls is due to the thread pool growth, which happens slowly. You can always tune the thread pool settings to achieve better performance, but it will never match the performance you can achieve with non-blocking calls.

Streams

Like all other blocking calls, any methods from System.IO.Stream should use their async equivalents: Read to ReadAsync, Write to WriteAsync, Flush to FlushAsync, etc. Also, after writing to a stream, you should call the FlushAsync method before disposing the stream. If not, the Dispose method may perform some blocking calls.

CancellationToken

You should always propagate cancellation tokens to the next caller in the chain. This is called a cooperative cancellation model. If not, you can end up with methods that run longer than expected, or even worse, never complete.

To indicate to the caller that cancellation is supported, the final parameter in the method signature should be a CancellationToken object.

 1public async Task DoStuffAsync()
 2{
 3    // BAD: This method does not accept a cancellation token. Cooperative cancellation is extremely important.
 4    await StuffAsync();
 5}
 6
 7public async Task DoStuffAsync(CancellationToken cancellationToken)
 8{
 9    var httpClient = new HttpClient();
10
11    // BAD: The caller has provided a cancellation token for cooperative cancellation but it was not provided
12    // downstream. Regardless of the token being signaled the http client will not return until the
13    // operation completes.
14    await httpClient.GetAsync("http://example.com");
15
16    // GOOD: Always propagate the token to the next caller in the chain
17    await httpClient.GetAsync("http://example.com", cancellationToken);
18}

Linked tokens

If you need to put a timeout on an inner method call, you can link one cancellation token to another. For example, you want to make a service-to-service call, and you want to enforce a timeout, while still respecting the external cancellation.

 1public async Task<int> DoThingAsync(CancellationToken cancellationToken)
 2{
 3    using (var cancelTokenSource = CancellationTokenSource.CreateLinkedTokenSource(cancellationToken))
 4    {
 5        // The cancellation token source will now manage a timer which automatically notifies the parent
 6        // token.
 7        cancelTokenSource.CancelAfter(TimeSpan.FromSeconds(10));
 8
 9        // By linking the token source we will now cooperatively cancel. We have also
10		// provided a time based auto-cancellation signal which only applies to the 
11		// the single method. This is how nested cancellation scopes may be used in circuit breakers.
12        await GetAsync(cancelTokenSource.Token);
13    }
14
15    // Other calls within the method may not need this restricted timeout so the original
16    // cancellation token is used instead.
17    await DoOtherThingAsync(cancellationToken);
18}

Cancelling uncancellable operations

Sometimes you may find the need to call an API which does not accept a cancellation token, but your API receives a token and is expected to respect cancellation. In this case the typical pattern involves managing two tasks and effectively abandoning the un-cancellable operation after the token signals.

 1public async Task<int> DoThingAsync(CancellationToken cancellationToken)
 2{
 3    var tcs = new TaskCompetionSource<int>();
 4    using (var registration = cancellationToken.Register(() => tcs.SetResult(0)))
 5    {
 6         var completedTask = await Task.WhenAny(tcs.Task, DoNonCancellableCallAsync());
 7         if (completedTask == tcs.Task)
 8         {
 9             // We have been cancelled, so take appropriate action here. At this point in time the non-cancellable call is
10             // still running. Once it completes everything will be properly garbage collected
11             throw new OperationCanceledException(cancellationToken);
12         }
13
14        return await completedTask;
15    }
16}

Constructors

Occasionally, you may find yourself wanting to perform asynchronous work during initialization of a class instance. Unfortunately, there is no way to make constructors async.

 1public class Foo
 2{
 3    public Foo(int a, int c)
 4    {
 5        _a = a;
 6        // DON'T DO THIS!
 7        _b = CallSomethingAsync().SyncResult();
 8        _c = c;
 9    }
10
11    private readonly int _a;
12    private readonly string _b;
13    private readonly int _c;
14}

There are a couple of different ways to solve this. Here’s a pattern I like:

  1. A public static creator method, which publicly replaces the constructor
  2. A private async member method, which does the work the constructor used to do
  3. A private constructor, so callers can’t directly instantiate the class by mistake

So, if I apply the same pattern to the class above the class becomes:

 1public class Foo
 2{
 3    public static async Task<Foo> CreateAsync(int a, int c)
 4    {
 5        Foo instance = new Foo(a, c);
 6        await instance.InitializeAsync();
 7        return instance;
 8    }
 9
10    private Foo(int a, int c)
11    {
12        // GOOD: readonly keyword is retained
13        _a = a;
14        _c = c;
15    }
16
17    private async Task InitializeAsync()
18    {
19        // GOOD: all async work is performed here
20        // NOTE: make sure initialization order is correct when porting existing code
21        _b = await CallSomethingAsync();
22    }
23
24    private readonly int _a;
25    private readonly int _c;
26
27    string _b;
28}

And we can instantiate the class by calling var foo = await Foo.CreateAsync(1, 2);.

In cases where the class is part of an inheritance hierarchy, the constructor can be made protected and InitializeAsync can be made protected virtual, so it can be overridden and called from derived classes. Each derived class will need to have its own CreateAsync method.

Parallelism

Avoid premature optimization

It might be very tempting to try to perform parallel work by not immediately awaiting tasks. In some cases, you can make significant performance improvements. However, if not used with care you can end up in debugging hell involving socket or port exhaustion, or database connection pool saturation.

 1public async Task DoTwoThingsAsync(CancellationToken cancellationToken)
 2{
 3    // Depending on what you are doing under the covers, this may cause unintended
 4    // consequences such as exhaustion of outgoing sockets or database connections.
 5    var thing1Task = FirstAsync(cancellationToken);
 6    var thing2Task = SecondAsync(cancellationToken);
 7    await Task.WhenAll(thing1Task, thing2Task);
 8
 9    // Prefer sequential execution of asynchronous calls
10    await FirstAsync(cancellationToken);
11    await SecondAsync(cancellationToken);
12}

Using async everywhere generally pays off without having to make any individual piece of code faster via parallelization. When threads aren’t blocking you can achieve higher performance with the same amount of CPU.

Avoid Task.Factory.StartNew, and use Task.Run only when needed

Even in the cases where not immediately awaiting is safe, you should avoid Task.Factory.StartNew, and only use Task.Run when you need to run some CPU-bound code asynchronously.

The main way Task.Factory.StartNew is dangerous is that it can look like tasks are awaited when they aren’t. For example, if you async-ify the following code:

1Task.WhenAll(Task.Factory.StartNew(Foo), Task.Factory.StartNew(Foo2)).Wait();

be careful because changing the delegate to one that returns Task, Task.Factory.StartNew will now return Task<Task>. Awaiting only the outer task will only wait until the actual task starts, not finishes.

1// BAD: Only waits until all tasks have been scheduled, not until the actual work has completed
2await Task.WhenAll(Task.Factory.StartNew(FooAsync), Task.Factory.StartNew(Foo2Async));

Normally what you want to do, when you know delegates are not CPU-bound, is to just use the delegates themselves. This is almost always the right thing to do.

1// GOOD: Will wait until the tasks have all completed
2await Task.WhenAll(FooAsync(), Foo2Async());

However, if you are certain the delegates are CPU-bound, and you want to offload this to the thread pool, you can use Task.Run. It’s designed to support async delegates. I’d still recommend reading Task.Run Etiquette and Proper Usage for a more thorough explanation.

1
2// Ok if FooAsync is known to be primarily CPU-bound (especially before going async).
3// Will schedule the CPU-bound work like Task.Factory.StartNew does,
4// and automatically convert Task<Task> into a 'proxy' Task that represents the actual work
5await Task.WhenAll(Task.Run(FooAsync), Task.Run(Foo2Async))

If, for some extremely unlikely reason, you really do need to use Task.Factory.StartNew you can use Unwrap() or await await to convert a Task<Task> into a Task that represents the actual work. I’d recommend reading Task.Run vs Task.Factory.StartNew for a deeper dive into the topic.

Null conditionals

Using the null conditional operator with awaitables can be dangerous. Awaiting null throws a NullReferenceException.

1// BAD: Will throw NullReferenceException if foo is null
2await foo?.DoStuffAsync()

Instead, you must do a manual check first.

1// GOOD
2if (foo != null)
3{
4    await foo.DoStuffAsync();
5}

A Null-conditional await is currently under consideration for future versions of C#, but until then you’re stuck with manually checking.

comments powered by Disqus