Avoid async void methods

csharp async 23 comments suggest edit

Repeat after me, “Avoid async void!” (Now say that ten times fast!) Ha ha ha. You sound funny.

In C#, async void methods are a scourge upon your code. To understand why, I recommend this detailed Stephen Cleary article, Best Practices in Asynchronous Programming. In short, exceptions thrown when calling an async void method isn’t handled the same way as awaiting a Task and will crash the process. Not a great experience.

Recently, I found another reason to avoid async void methods. While investigating a bug, I noticed that the unit test that should have ostensibly failed because of the bug passed with flying colors. That’s odd. There was no logical reason for the test to pass given the bug.

Then I noticed that the return type of the method was async void. On a hunch I changed it to async Task and it started to fail. Ohhhhh snap!

If you write unit tests using XUnit.NET and accidentally mark them as async void instead of async Task, the tests are effectively ignored. I furiously looked for other cases where we did this and fixed them.

Pretty much the only valid reason to use async void methods is in the case where you need an asynchronous event handler. But if you use Reactive Extensions, there’s an even better approach that I’ve written about before, Observable.FromEventPattern.

Because there are valid reasons for async void methods, Code analysis won’t flag them. For example, Code Analysis doesn’t flag the following method.

public async void Foo()
{
    await Task.Run(() => {});
}

It’s pretty easy to manually search for methods with that signature. You might even catch them in code review. But there are other ways where async void methods crop up that are extremely subtle. For example, take a look at the following code.

new Subject<Unit>().Subscribe(async _ => await Task.Run(() => {}));

Looks legit, right? You are wrong my friend. Take a shot of whiskey (or tomato juice if you’re a teetotaler)! Do it even if you were correct, because, hey! It’s whiskey (or tomato juice)!

If you look at all the overloads of Subscribe you’ll see that we’re calling one that takes in an Action<T> and not a Func<T, Task>. In other words, we’ve unwittingly passed in an async void lambda. Because of the beauty of type inference and extension methods, it’s hard to look at code like this and know whether that’s being called correctly. You’d have to know all the overloads as well as any extension methods in play.

Here I Come To Save The Day

Clearly I should tighten up code reviews to keep an eye out for this problem, right? Hell nah! Let a computer do this crap for you. I wrote some code I’ll share here to look out for this problem.

These tests make use of this method I wrote a while back to grab all loadable types from an assembly.

public static IEnumerable<Type> GetLoadableTypes(this Assembly assembly)
{
    if (assembly == null) throw new ArgumentNullException("assembly");
    try
    {
        return assembly.GetTypes();
    }
    catch (ReflectionTypeLoadException e)
    {
        return e.Types.Where(t => t != null);
    }
}

I also wrote this other extension method to make the final result a bit cleaner.

public static bool HasAttribute<TAttribute>(this MethodInfo method) where TAttribute : Attribute
{
    return method.GetCustomAttributes(typeof(TAttribute), false).Any();
}

And the power of Reflection compels you! Here’s a method that will return every async void method or lambda in an assembly.

public static IEnumerable<MethodInfo> GetAsyncVoidMethods(this Assembly assembly)
{
    return assembly.GetLoadableTypes()
      .SelectMany(type => type.GetMethods(
        BindingFlags.NonPublic
        | BindingFlags.Public
        | BindingFlags.Instance
        | BindingFlags.Static
        | BindingFlags.DeclaredOnly))
      .Where(method => method.HasAttribute<AsyncStateMachineAttribute>())
      .Where(method => method.ReturnType == typeof(void));
}

And using this method, I can write a helper method for all my unit tests.

public static void AssertNoAsyncVoidMethods(Assembly assembly)
{
    var messages = assembly
        .GetAsyncVoidMethods()
        .Select(method =>
            String.Format("'{0}.{1}' is an async void method.",
                method.DeclaringType.Name,
                method.Name))
        .ToList();
    Assert.False(messages.Any(),
        "Async void methods found!" + Environment.NewLine + String.Join(Environment.NewLine, messages));
}

Here’s an example where I use this method.

[Fact]
public void EnsureNoAsyncVoidTests()
{
    AssertExtensions.AssertNoAsyncVoidMethods(GetType().Assembly);
    AssertExtensions.AssertNoAsyncVoidMethods(typeof(Foo).Assembly);
    AssertExtensions.AssertNoAsyncVoidMethods(typeof(Bar).Assembly);
}

Here’s an example of the output. In this case, it found two async void lambdas.

------ Test started: Assembly: GitHub.Tests.dll ------

Test 'GitHub.Tests.IntegrityTests.EnsureNoAsyncVoidTests' failed: Async void methods found!
'<>c__DisplayClass10.<RetrievesOrgs>b__d' is an async void method.
'<>c__DisplayClass70.<ClearsExisting>b__6f' is an async void method.
	IntegrityTests.cs(104,0): at GitHub.Tests.IntegrityTests.EnsureNoAsyncVoidTests()

0 passed, 1 failed, 0 skipped, took 0.97 seconds (xUnit.net 1.9.2 build 1705).

These tests will help ensure my team doesn’t make this mistake again. It’s really subtle and easy to miss during code review if you’re not careful. Happy coding!

Found a typo or error? Suggest an edit! If accepted, your contribution is listed automatically here.

Comments

avatar

23 responses

  1. Avatar for distantcam
    distantcam November 11th, 2014

    Very nice! This should be pretty easy to turn into a VS14 diagnostic and have it turn up as a compiler error.

    One thing though, when searching for the AsyncStateMachineAttribute instead of using GetType it's better to search by name, as the type may come from a different assembly. See https://github.com/nunit/nu...

  2. Avatar for Jesse Slicer
    Jesse Slicer November 12th, 2014

    Since there are two big problems in computer science: naming things, cache invalidation and off-by-one errors, I would think that

    public static bool HasAttribute<tattribute>(this MethodInfo property) where TAttribute : Attribute
    {
    return property.GetCustomAttributes(typeof(TAttribute), false).Any();
    }

    would be better off having the "property" parameter named as "method", no?

  3. Avatar for dotnetchris
    dotnetchris November 12th, 2014

    I seriously don't understand why they made async void any different than async Task, it's ridiculous that they leave this tiny difference be the cause for a world of hurt.

    I also think they really botched ConfigureAwait(). There is absolutely no reason we should be forced to muck around with that, the TPL really should be able to figure out ConfigureAwait on its own.

  4. Avatar for haacked
    haacked November 12th, 2014

    Ooh, not a bad idea. I have yet to write a diagnostic. This might be a good one to get my hands dirty with.

    Thanks for the tip about searching `AsyncStateMachineAttribute` by name. Am I correct to assume that only applies to .NET 4.0 and before when using the Microsoft.Bcl.* packages?

  5. Avatar for haacked
    haacked November 12th, 2014

    Ummm, yeah! That was a copy paste error.

  6. Avatar for Rex Ng
    Rex Ng November 12th, 2014

    why not simply:

    public static bool HasAttribute<tattribute>(this MethodInfo property) where TAttribute : Attribute
    {
    return property.GetCustomAttribute<tattribute>() != null ;
    }

  7. Avatar for haacked
    haacked November 12th, 2014

    Yep, that would work too. :)

  8. Avatar for haacked
    haacked November 12th, 2014

    Actually, no. I remember now. Because if the attribute exists more than once, that method throws an exception.

  9. Avatar for distantcam
    distantcam November 12th, 2014

    It may also affect PCL and mono libraries. In general, when doing that sort of reflection tricks the framework may not be the same one that the unit test is running under.

  10. Avatar for Robert Iagar
    Robert Iagar November 13th, 2014

    so when using MVVM light and I have commands that call Async methods I pass them into the RelayCommand constructor like:

    this.Command = new RelayCommand(async () => this.SomeFooAsync());

    that's async void right?

    Any ways to fix this?

  11. Avatar for Alxandr
    Alxandr November 14th, 2014

    Please do write a "diagnostic" (that's a horribly undescriptive name btw, should be called "Diagnostic Provider" or something along those lines) for this and blog about it :)

    I would very much like to read how you find the proccess of so doing to be.

  12. Avatar for Stefan
    Stefan November 16th, 2014

    Looks like you skipped the "watch the new test fail" step of TDD or you should have noticed the test pass incorrectly.

  13. Avatar for Tomasz Cielecki
    Tomasz Cielecki November 17th, 2014

    You could create your own ICommand implementation which does not do that. Take a look at what Ross is using for MvvmCross: https://gist.github.com/ros...

  14. Avatar for Robert Iagar
    Robert Iagar November 17th, 2014

    oh nice... I didn't know that... this looks really nice :) thanks for the tip ;)

  15. Avatar for Harry McIntyre
    Harry McIntyre November 17th, 2014

    This should really be baked into Xunit.

  16. Avatar for Thomas Levesque
    Thomas Levesque December 4th, 2014

    Well, how would you want async void to behave? Just silently swallow any unhandled exception? I think it would be worse, because it would hide bugs. At least, if your app crashes, you know somehing is wrong. Having async void silently swallow exceptions would be the same as an empty catch block. At least, right now it forces you to handle exceptions explicitly. BTW, it's the same if you have an async Task method and ignore the result: an exception will be thrown when the Task is GC'd, because the exception was not observed; it will also crash your process, unless you handle the UnobservedTaskException event.

    Regarding ConfigureAwait, I don't think it would be possible. There is no way for the compiler or the TPL to know whether or not you want to resume on the same sync context. Typically, you do, so the default is to capture the sync context; if you don't want that, you have to opt out explicitly. What heuristic would you use to decide to do it automatically?

  17. Avatar for dotnetchris
    dotnetchris December 4th, 2014

    Both paragraphs, i just don't accept the answer that "it's impossible". I think they just took the "easy" way out.

  18. Avatar for Thomas Levesque
    Thomas Levesque December 4th, 2014

    You "don't accept that it's impossible"? It's like saying "I don't accept that I can't fly"... The compiler can't guess what you want.

    It's easy to criticize, but I don't see you proposing a better solution, apart from "do the right thing without making me think". If you're so sure it's possible, why don't you fork the Roslyn compiler and try to make it do what you want? ;)

  19. Avatar for Bruno Juchli
    Bruno Juchli January 26th, 2015

    Since i'm quite "afraid" of `async void` i took the time to write a StyleCop rule. You can find it on Github https://github.com/BrunoJuc... and Nuget https://www.nuget.org/packa... . I'd
    appreciate feedback if anything is missing or not working as it should. Since VS2015 is not that far away i guess i'll have to look into it again, quite soon. If someone has pointers on how this could be implemented for VS2015 without relying on 3rd party tools like StyleCop and FxCop i'd appreciate it, too.
    Edit: fixed links :|

  20. Avatar for Jakub Suchý
    Jakub Suchý November 18th, 2015

    Nice thing! I have my TestSolution where I am adding all these interesting mini projects that I read online. And to cover all those projects with this EnsureNoAsyncVoidTest first I typed all Assemblies by hand, but then found better way with:

    Assembly[] assemblies = AppDomain.CurrentDomain.GetAssemblies();
    foreach( var assembly in assemblies )
    AssertExtensions.AssertNoAsyncVoidMethods( assembly );

    Turns out, that my assemblies are fine, but got this interesting Fail on added Reference:
    Assert.IsFalse failed. Async void methods found!
    'WebSocketBase.OnKeepAlive' is an async void method.

  21. Avatar for Kristian Barrett
    Kristian Barrett November 30th, 2016

    You should do something like this instead:
    var myNamespace = "YOUR NAMESPACE HERE";
    var assemblies = GetType().Assembly.GetReferencedAssemblies()
    .Where(x => x.Name.Contains(myNamespace));
    foreach (var assemblyName in assemblies)
    {
    var assembly = Assembly.Load(assemblyName);
    AssertExtensions.AssertNoAsyncVoidMethods(assembly);
    }

    And then reference all the projects you want to test from your unit test project.

  22. Avatar for quetzalcoatl
    quetzalcoatl July 12th, 2017

    Rad!

  23. Avatar for Robolize
    Robolize October 3rd, 2017

    I like this, but I have a question. What about the cases when an async void is valid? I want to setup the unit tests as you have shown, but I think I'm going to have cases where I can't change to async Task. I'm thinking I could maintain a white-list in the unit test of approved usages. Is there a better way?