davidfowl / AspNetCoreDiagnosticScenarios

This repository has examples of broken patterns in ASP.NET Core applications
7.69k stars 740 forks source link

Returning async/await advice contradicts other MSFT advice #53

Open jnsn opened 5 years ago

jnsn commented 5 years ago

In the AsyncGuidance document, there's a section that states that async/await is preferred over directly returning Task.

In his talk Correcting Common Async/Await Mistakes in .NET, Brandon Minnick advises to directly return the Task over async/await. (The specific bit is around 24:27.)

Since this is contradictory advice given by two MSFT employees, I feel like you guys should align this somehow, or clarify this use-case a bit more. :-)

dferretti commented 5 years ago

The guidance here to never just return the Task seems like a safer default to use. In that video it looks like he mentions the troubles you can have if you return the Task from within a try/catch block, but there are more ways you can trip yourself up if you aren't awaiting the Task. This article from Stephen Cleary https://blog.stephencleary.com/2016/12/eliding-async-await.html mentions more of the pitfalls, and at the bottom has some recommendations that basically say don't just return the Task unless it is a very simple method that just passes through to another Task returning method and will not throw any exceptions or anything.

slang25 commented 5 years ago

Agreed, the guidance here is a better general reference for people, default to the style that will be less surprising.

It even mentions the caveat that for high-performance scenarios it is then worth considering eliding, but only if you want/need that perf gain.

georg-jung commented 4 years ago

While reading through MSDN I found another directly contradictory example: The very last code block in https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/classes-and-structs/local-functions#local-functions-and-exceptions

that basically say don't just return the Task unless it is a very simple method that just passes through to another Task returning method and will not throw any exceptions or anything.

It's directly contradictory to this advice too as it basically is a wrapper which throws exceptions.

davidglassborow commented 4 years ago

Useful notes on sync contexts when using async/await - https://devblogs.microsoft.com/dotnet/configureawait-faq/

stevendarby commented 4 years ago

... has some recommendations that basically say don't just return the Task unless it is a very simple method that just passes through to another Task returning method and will not throw any exceptions or anything.

The example given in the article in the OP saying not to return the Task directly seems to be exactly the type of method this other advice says is safe to return the Task directly.

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

So I’m still confused!

epignosisx commented 4 years ago

Having something like this:

public class C
{
  public Task M()
  {
    return FooAsync(); //a Task returning method.
  }
}

It's totally fine if you do not care about handling exceptions in FooAsync().

In regard as to why it is faster to pass through the Task instead of awaiting. Check out the generated code to support async/await vs just passing through the Task:

https://sharplab.io/#v2:D4AQDABCCMCsDcBYAUCAzFATBAwhA3ihMVBiAGwQAKAhgM50AqAFgE4CuAFAJQFEkCQAdijkAdABEApgBsaAT07RuSZAIC+/YltJQAHKIgBBAO40AlgBcpAEx47CagSRABOUZNkKlKnZuTqQA===