My team is in the process of refactoring a large application that we want to deploy to our production environment soon. I've just been assigned as lead developer to this project, since the prior lead got another job in a different state, and I've been digging through the code to see where I can improve its performance (this is something the principal users have relayed to me was already an issue).

I was told this project had already implemented async/await, a statement which turned out to be true in the same way that asking if my five-year-old had played in the sandbox recently turns out to be true; they did, and now the sand is all over the house. The asynchronous code was implemented rather poorly. It was bad enough, in fact, that I promptly began ripping out most of the async/await code that I could find.


Photo by NeONBRAND / Unsplash

During this process, I've come across two common scenarios in which async/await was applied incorrectly. For the benefit of anyone else who might have this problem (including the future me), let's see how we can fix this code to implement async/await the correct way.

Scenario 1: Negating Async Benefits

Here's our first example, and hopefully the problem becomes immediately obvious:

ServiceClient client = new ServiceClient();
ServiceRequest request = new ServiceRequest();
request.Id = newId;            
var responseTask = Task.Run(() => client.GetServicesAsync(request));
ServiceResponse response = await responseTask;

That's right, it is wrapping an async method which returns a Task in another Task!

This is redundant. If the method already returns a task, we don't want to wrap it in another. So, in this case, the solution is fairly simple:

ServiceClient client = new ServiceClient();
ServiceRequest request = new ServiceRequest();
request.Id = newId;            
var responseTask = client.GetServicesAsync(request);
ServiceResponse response = await responseTask;

But there's another, more pernicious problem I'm running into, and that's our second scenario.

Scenario 2: Abuse of Task.Run()

The team that implemented this project was under the impression that async helps improve performance. This is true, but only tangentially; what async/await does is help improve throughput. Improving performance is a mere side effect.

Hence, I found all sorts of code which looked like this:

MyService client = new MyService();
var responseTask = Task.Run(() => client.GetData(request));
var response = await responseTask;

In other words: a synchronous call was wrapped using Task.Run().

The problem here is the method client.GetData() is not, in and of itself, an asynchronous method. By putting an asynchronous wrapper over a synchronous method, we are doing an antipattern known as async-over-sync and this ends up being a bad idea most of the time. Not because it doesn't work (it does) but because it isn't efficient.

The reasoning for why this is true is long and rather involved, so this article will probably help clear it up. To sum up that article, we don't want to do this because the benefit of implementing asynchrony is to improve throughput by "releasing" threads when they aren't doing any work (e.g. when waiting for a service to respond), and while the above example does release a thread it also immediately consumes another to perform the task-wrapped code, and this consumed thread is blocked while waiting for the service to respond. So we didn't improve throughput, we just moved the work from one thread to another.

The solution for this can be reduced to: don't use async wrappers for synchronous methods! Just call them synchronously; that's how they were written. The theoretical performance benefits in this scenario are outweighed by the potential problems, which may include deadlock in the worst case.

Over-using Task.Run() is a code smell! I've almost reached the opinion that the any use of it is a reason to remove it, though I won't go quite that far yet. Don't tempt me.

Summary

Using async/await in ASP.NET is a skill, one that must be used cautiously. In the examples above, the development team was trying to make their application perform better, but ended up making things worse by over-applying async/await to their code.
There are two things you should keep in mind:

  1. Don't wrap asynchronous tasks in another call to Task.Run().
  2. Don't use asynchronous wrappers over synchronous calls.

There are lots of ways to screw up your ASP.NET code that uses async/await. Hopefully, these two will no longer be included in that group.

Happy Coding!