Should web API controller actions that perform no async work always be declared as async?

Question

I have been going over our app web api and making sure all async work is async all the way - and no artificial asynchronicity is enforced.

Say I have the following web api controller:

 [HttpGet]
 public async Task<IActionResult> Foo()
 {
 //no async work
 return OK(...);
 }

I planned to remove the async keyword from actions like the one above - those that do not perform any async work - changing it to

 [HttpGet]
 public IActionResult Foo()
 {
 //no async work
 return OK(...);
 }

Then one of my teammates claims controller actions should be always async. I could not find explicit evidence over the internet for this argument - but even if it is true, it has the drawback of polluting my solution by adding CS1998 warnings (This async method lacks 'await' operators and will run synchronously. Consider using the 'await' operator to await non-blocking API calls, or 'await Task.Run(...)' to do CPU-bound work on a background thread) all over (One can suppress those, but, I find it contradictory that on the one hand it would be a best practice to have them async, and on the other compiler will raise a flag indicating something needs attention on these).

Is it really a good practice to have non-async controller actions - async ?

• Laiv: Then one of my teammates claims controller actions should always be async did he or she give you any argument?
• Veverke: He thinks this enhances scalability. AFAIU, the idea is that using async enforces asp.net core to use the async/await model over the old asp.net one-thread-per-request model. If this is really true, sounds like a good argument. I just could not find clear evidence on that. And if this is the case, as I state in the question's body - the rosalyn should take that into consideration and not show such warnings... ?
• Ben Cottrell: @Veverke "using async enforces asp.net core to use the async/await model" - this is incorrect I'm afraid; the async keyword by itself (without an await) does no such thing, it merely enables the await keyword, nothing more - certainly nothing to do with threads (while it changes the method return type so that the result is wrapped in a Task, Task by itself does not alter behaviour). if await isn't used, then it is merely a plain synchronous method. Stephen Cleary's blog has a good, in-depth explanation: blog.stephencleary.com/2012/02/async-and-await.html
• Veverke: @BenCottrell: thanks Ben, that's what I thought, when I heard that at first.

Answer 1

I think this is one of those things that doesn't concretely matter enough to die on either hill; but developers tend to err one way or another. I, for one, err the other way than you. This answer is my reasoning, but at the end of the day both our approaches yield working solutions, there is no right and wrong here.

---

Taking a bit of a detour, let's refresh our definition of encapsulation. We abstract away the concrete, in order for the consumer to no longer be attached to the concrete implementation but rather the contract. The idea here is that by doing so, we are about to swap out our implementation without impacting the consumer, as long as we can adhere to the contract that the consumer was built to work with.

Again a small detour, I want to focus on methods in general for a second, not just API endpoints. I reckon the spirit of encapsulation is best continued by making all signatures async, because then the concrete implementation is free to choose whether it implements a synchronous or an asynchronous solution.

In a sense, having a synchronous signature reveals to the consumer that you are working synchronously. If you ever need to work asynchronously, you need to disrupt the established contract. Vice versa, this would not have been a problem, as you could have made a previously async method synchronous without needing to disrupt the contract.

Therefore, erring towards async signatures is more change-friendly than only using them when the concrete need arises, which is why I'm in favor of sticking with a consistent async signature once my application is predominantly async.

Focusing back to API endpoints, the argument is slightly weaker, since the framework handles async/sync endpoints for you. There is no real consumer that you need to take care of since the consumer is .NET and it can adapt to either.
Maybe you consider that a reason to ignore the above reasoning. I could see how you get there. But I am a stickler for consistency in a codebase, and since the same reasoning applies, even if only on principle, I prefer to keep things consistent instead of introducing exceptional sync cases here and there.

Are there exception to this? Of course. Not every extension method or helper class I write is therefore async, because there are some cases where the remit of the class is clearly not of an asynchronous nature. This is not a dogma nor a blanket statement, and I'm not trying to sell it as such.

---

Update To be clear, because I notice from the comments that this was left too ambiguous, I am not suggesting you use the async keyword everywhere. My focus here is on the contracts being async-oriented.

The async keyword doesn't impact the contract (you can freely change it without forcing the consumer to make any change), but things like adding/removing the Task<T> return type, passing a CancellationToken, ... does impact the contract, and it's those things that I would add even if you know that the method body is currently synchronous.

In other words, I would leave the method differently than either of your examples. I would be more inclined to do:

[HttpGet]
public Task<IActionResult> Foo()
{
 IActionResult result = OK(...);
 return Task.FromResult(result); 
}

No async keyword since there is no await happening. However, I did keep the Task<T> return type as this forms part of the contract.

---

As an aside, I think part of this is due to async only having appeared on the stage much later than when synchronous execution did.

Because of it, using an async signature feels like doing something extra on top of the (synchronous) baseline, which in turn triggers developers' need to be efficient and makes them go "why would I do this extra thing when I don't need it right now? YAGNI!".

I think that's the wrong way around. I think our baseline should be the most permissive options, in this case async (which, as established), can also house sync operations). We should only strengthen the condition to only allow async when there are cases where we either clearly don't need or explicitly want to disallow asynchronous execution.

Had we have had async right from the get go with sync, I reckon we wouldn't have thought about async as a quirky extra. But we didn't, and so we don't use async as the baseline since we were first taught synchronous operations. Maybe the next generation of developers will be different now that async is here to stay.

• Eternal21: @Veverke I'm with you when it comes to warnings. In fact the first thing I do when creating a proejct is enable 'treat warnings as errors', and then stomp every single one out.
• Flater: @user253751 Asynchrony and multithreading are two distinct concepts. You can write async code that utilizes a single thread. Realistically, that's not a constraint people work with, but it proves the point that asynchrony is not "called multithreading" as you suggest.
• Flater: @Veverke: I'll amend this in the answer because I understand that this was left ambiguous, but I wasn't suggesting you use the async keyword everywhere - my focus was on the contracts being async-oriented. The async keyword doesn't impact the contract (you can freely change it without forcing the consumer to make any change), but things like adding/removing the Task<T> return type, passing a CancellationToken, ... does impact the contract, and it's those things that I would add even if you know that the method body is currently synchronous.
• Flater: @Veverke: You may have glossed over the fact that you can have a method that returns a Task<T> (therefore being async-friendly) but without using the async keyword. That is what I am suggesting. Adding/removing the async keyword does not change the contract, and therefore the async keyword is not part of what I'd suggest you do in order to remain async-friendly even if the current implementation is synchronous. If you omit the async keyword (aside: which means you'll have to wrap your returned value in Task.FromResult(...)), you will not get the warning.
• Flater: @JasonWeber: Not saying it can't be done the way you suggest, but versioning is a necessary evil that exists to deal with the breaking of a contract. Not breaking a contract to begin with is even better. I'm also not a fan of the "async" suffix unless you have competing sync and async alternatives for the same method. Once you assume async by default, the "async" suffix ceases to be useful. This is, in my opinion, a variation on why Hungarian notation has gone out of style. We should not be baking technical details into the human-readable name we use.