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 ?

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.