davidfowl / AspNetCoreDiagnosticScenarios

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

Async void is not always bad #86

Open yBother2 opened 2 years ago

yBother2 commented 2 years ago

I wouldn't agree on the statement that async void is evil. There are definitely exceptions where you're fine to use async void. Moreover I'd really appreciate more details on how async void can cause process crashes when exceptions occur.

To summarize this first guideline, you should prefer async Task to async void. Async Task methods enable easier error-handling, composability and testability. The exception to this guideline is asynchronous event handlers, which must return void. This exception includes methods that are logically event handlers even if they’re not literally event handlers (for example, ICommand.Execute implementations).

from https://docs.microsoft.com/en-us/archive/msdn-magazine/2013/march/async-await-best-practices-in-asynchronous-programming

dferretti commented 2 years ago

Here is an article from years ago explaining how unobserved Task exceptions can cause crashes: https://devblogs.microsoft.com/pfxteam/task-exception-handling-in-net-4-5/

And here is a more recent conversation about it (with a comment from the same author) explaining that unobserved exceptions on Tasks will cause the UnobservedTaskException to be raised when the Task (its TaskExceptionHolder) is finalized. https://github.com/dotnet/runtime/issues/21269

Also see:

And here is an example of it crashing the process:

try
{
    Console.WriteLine("Testing async void method");
    Test();
}
catch
{
    Console.WriteLine("caught exception"); // never hit
}

// if we get this far, the loop should never exit right?
while (true)
{
    await Task.Delay(10);
    Console.WriteLine("waiting for task to be GCed");
}

async void Test()
{
    await Task.Yield();
    throw new Exception("oops");
}

When you call an async void method, there is a Task involved, but not one that you can try to handle exceptions for. On my machine, this program will print about a dozen times before the dotnet process exits with an error code.

yBother2 commented 2 years ago

I did not finish studiying but this is really interesting. I still find it hard to believe that async void is generally evil since it is explicetly said (see referenced article) to be fine in some scenarios. What is it now?

dferretti commented 2 years ago

I've also seen similar issues where a developer might accidentally cause the process to crash by not realizing they were creating unobserved Task exceptions in writing essentially async void versions of Action<T>.

List<T> has a method ForEach(Action<T> action), and I've seen developers write code like

myList.ForEach(async element => 
{
    await SomeMethodThatThrows(element);
});

This will also crash the process - and can be difficult to debug if the crash might not be immediately when this code executes, but instead some time later when the task is finalized.

This is what is mentioned in this repo here: https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#implicit-async-void-delegates

dferretti commented 2 years ago

It's more just about the fact that it is hard or impossible to properly handle exceptions when you are calling an async void method. If you are writing an async void method (because you have to, for a button click callback etc), then you have to make very sure to handle exceptions.

The linked article doesn't specifically say it is "fine" to use async void in those scenarios. It is just unavoidable because what you are coding against does not offer an async-friendly alternative. So you should always avoid writing async void methods, and only write them when you absolutely have to.

yBother2 commented 2 years ago

I just wrote a very simple test that basically proofs you right:

 var list = Enumerable.Range(0, 10).ToList();
            list.ForEach(
                async x =>
                    {
                        try
                        {
                            if (x % 2 == 0)
                            {
                                throw new Exception();
                            }

                            await Task.Delay(1000);
                        }
                        catch (Exception ex)
                        {
                            Console.WriteLine(ex.Message);

                            // throw // this will crash the test itself!
                        }
                    });

            await Task.Delay(2000);

            Assert.Pass();

This test throws an exception. To workaround that one could either catch exceptions or wrap the lambda in Task.Run()

var list = Enumerable.Range(0, 10).ToList();
            list.ForEach(
                x => Task.Run(
                    async () =>
                        {
                            if (x % 2 == 0)
                            {
                                throw new Exception();
                            }

                            await Task.Delay(1000000);
                        }));

            await Task.Delay(2000);

            Assert.Pass();
yBother2 commented 2 years ago

It's more just about the fact that it is hard or impossible to properly handle exceptions when you are calling an async void method. If you are writing an async void method (because you have to, for a button click callback etc), then you have to make very sure to handle exceptions.

The linked article doesn't specifically say it is "fine" to use async void in those scenarios. It is just unavoidable because what you are coding against does not offer an async-friendly alternative. So you should always avoid writing async void methods, and only write them when you absolutely have to.

Fine but https://github.com/davidfowl/AspNetCoreDiagnosticScenarios/blob/master/AsyncGuidance.md#async-void states that it is never okay to do so. And that's where I still like to disagree. It's typically just a bad idea because you really have to deal with exceptions but it still can work just fine, right?

dferretti commented 2 years ago

Ah so this guidance is specific to ASP.NET CORE:

Use of async void in ASP.NET Core applications is ALWAYS bad.

In ASP.NET Core there shouldn't ever be a place where you have to write async void. It is built async-friendly top to bottom.

The other examples are for things like UWP where you have no other choice. (I haven't written UWP in a while so that may have changed, not positive)

CwjXFH commented 1 year ago

@dferretti Sometimes, we will create a Timed background tasks , which need execute an async task, but the Timer only receive a callback which returns void.

So, in this case, we need to use async void. In order to catch exceptions, we can call an async Task method in the async void method, which handles the real logic:

async void TimerCallback(object? state)
{
    try
    {
        await TimerCallbackCore(state);
    }
    catch (Exception ex)
    {

    }
}

async Task TimerCallbackCore(object? state)
{
    // do something
}

new Timer(TimerCallback)
newbe36524 commented 1 year ago

@dferretti Sometimes, we will create a Timed background tasks , which need execute an async task, but the Timer only receive a callback which returns void.

So, in this case, we need to use async void. In order to catch exceptions, we can call an async Task method in the async void method, which handles the real logic:

async void TimerCallback(object? state)
{
    try
    {
        await TimerCallbackCore(state);
    }
    catch (Exception ex)
    {

    }
}

async Task TimerCallbackCore(object? state)
{
    // do something
}

new Timer(TimerCallback)

PeriodicTimer for this case is better choice for async code since .net 6

CwjXFH commented 1 year ago

@newbe36524

PeriodicTimer.WaitForNextTickAsync will block the caller, it's not what I want.

davidfowl commented 1 year ago

It's what you want, and if you want to ignore backpressure then you can always fire and forget.