Is AsyncFixer01 really an anti-pattern?

[Eli-Black-Work]

Thanks for the analyzer! 🙂

We've been using the analyzer in several of our projects for a while now. We always disable the AsyncFixer01: "Unnecessary async/await usage" warning, because the fix elides methods from the stack trace when an exception is thrown.

For example:

async Task SaveItemAsync() {
   throw new Exception();
}

async Task UpdateAndSaveAsync() {
   //...
   await SaveItemAsync();
}

Main() {
   await UpdateAndSaveAsync();
}

In this case the stack track will show like this:

Main()
UpdateAndSaveAsync()
SaveItemAsync()

If we take the AsyncFixer01 suggestion and change the code to this:

async Task SaveItemAsync() {
   throw new Exception();
}

Task UpdateAndSaveAsync() {
   //...
   return SaveItemAsync();
}

Main() {
   await UpdateAndSaveAsync();
}

Then the stack track will not contain PrintAsync():

Main()
UpdateAndSaveAsync()

To me, this suggests that "unnecessarily" using await isn't an really an antipattern.

Would like to hear your thoughts. Thanks again for the analyzer 🙂

[jannesrsa]

Here is guidance from David Fowler.

https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#prefer-asyncawait-over-directly-returning-task

Prefer async/await over directly returning Task

There are benefits to using the async/await keyword instead of directly returning the Task:

• Asynchronous and synchronous exceptions are normalized to always be asynchronous.
• The code is easier to modify (consider adding a using, for example).
• Diagnostics of asynchronous methods are easier (debugging hangs etc).
• Exceptions thrown will be automatically wrapped in the returned Task instead of surprising the caller with an actual exception.

❌ BAD This example directly returns the Task to the caller.

public Task<int> DoSomethingAsync()
{
    return CallDependencyAsync();
}

✅ GOOD This examples uses async/await instead of directly returning the Task.

public async Task<int> DoSomethingAsync()
{
    return await CallDependencyAsync();
}

[Eli-Black-Work]

Personally, I think our team would prefer an analyzer that does the opposite of AsyncFixer01: Warn whenever someone directly returns a task instead of awaiting it 🙂

Perhaps it's a different between application authors and library authors: Application authors probably prefer to await before returning, so they can debug more easily, while library authors may sometimes want to directly return without awaiting.

[RudolfJan]

I miss a clear explanation for the AyncFixer01. Except for the argument that has a performance impact, I see no arguments. That is lacking for this tool, it is a kind of recipe book but it does not explain when the recipe is valid and when it is better to solve the issue into another way. I really would like a tutorial, not only with the do not examples, but also with examples the other way round.

[bstordrup]

I agree that AsyncFixer01 should not be treated as an anti-pattern.

In Stephen Cleary's blog, there is a post Eliding Async and Await that states, that the performance gain by eliding is minimal, and that the pitfalls for eliding are much greater than the performance gain.

He presents the pitfalls in the post and also presents his recommended guidelines:

1. Do not elide by default. Use the async and await for natural, easy-to-read code.
2. Do consider eliding when the method is just a passthrough or overload.

So based on this, the AsyncFixer01 should not be considered a must do fix, but a suggestion to consider.