Why you should (almost) never write void asynchronous methods?

I came across this interesting post by Christian Jacobsen titled async/await in C# – a disaster waiting to happen? in which he describes that simple refactoring can introduce serious bugs in async code. While there is definitely something to be said about possible problems with the new Async feature, his example can teach us something.

I will borrow his code to illustrate the point:

async void AcquireFromCamera(object sender, RoutedEventArgs e)
{
    try
    {
        var imageStream = await _cameraCapture.Shoot();
        var dto = new Dto(){ImageStream = imageStream};
        _handler.ImageCaptured(dto);
        Frame.Navigate(typeof(EditDataPage), dto.Id);
    }
    catch (Exception ex)
    {
        new MessageDialog(ex.Message).ShowAsync();
    }
}

async void ImageCaptured(Dto dto)
{
    dto.Id = Guid.NewGuid().ToString();
    var file = await _fileHndlr.CreateFileAsync(dto.Id);
    dto.ImageFilePath = file.Path;
    _fileOperator.StoreStream(dto.ImageStream, file);
    SaveNewDataItem(dto);
    var dataItem = dataSource.GetItem(dto.Id);
    StoreData(dataItem);
}

Christiansen argues that this is a buggy piece of software since the AcquireFromCamera method calls ImageCaptured as if it was a synchronous method. And he is right since both methods are non-returning. This usage of asynchrony is called fire and forget.

To fix this one needs to change the return type from void to Task and the compiler will now warn you with the following warnings:

warning CS4014: Because this call is not awaited, execution of the current method continues before the call is completed. Consider applying the 'await' operator to the result of the call.

Void returning asynchronous methods are called fire and forget simply because you don’t need to know when they complete, you initiate them and that is it. The code above requires the ImageCaptured function to complete before continuing and for that you need to await it.

Change the return type for all asynchronous methods from void to Task or Task<T>. If the asynchronous method does not return any value, use Task. If it returns an instance of type T, use Task<T>.

So which methods can be both void and async? Only event handlers since the caller does not need to know when the method completes.

If you enjoyed this post, please consider leaving a comment or subscribing to the RSS feed to have future articles delivered to your feed reader.

Last updated by at .

  • natman

    So, essentially you’re saying throw in an “await” keyword at the method call in AcquireFromCamera like, “await _handler.ImageCaptured(dto);” and then replace void in the method declaration to be Task?
    Since you cannot pass objects by ref in the parameters, how is the value updated unless you also explicitly return it? Say by having “var newDto = await _handler.ImageCaptured(dto);”?
    Or was that implied?
    Also: can’t you also do callback parameters (“callback dtoReturn”) and then in the method call, have it be, “await _handler.ImageCaptured(dto, dtoReturned => { Frame.Navigate(typeof(EditDataPage), dtoReturned.Id);});”?
    If you can, I would be curious to know which is a better way of working. If you can not, I would most definitely like to know why.
    Thanks!

    • http://www.tonicodes.net/blog/ Toni Petrina

      Hi Nathan,

      It is not a good idea to pass parameters to async methods and then change them after the first await. That is one reason for not supporting out and ref parameter modifiers in the asynchronous functions.

      Since await “forks” method execution and returns to the caller code immediately, you cannot reasonably expect that changing the variable afterwards is a valid code contract and if there is a result of the potentially long asynchronous operation, you should return the result rather than modifying the input parameters.

      Don’t add callbacks, we have the Async to avoid them :)

      Instead write this:
      var dto = await _handler.ImageCaptured(imageStream);// …async Task ImageCaptured(ImageStream imageStream) {}

      You pass the input parameters to the asynchronous function and once it is done, the result will be stored in the returned Task.

      Consider the case where you process several images or where you want to use regular ContinueWith style chaining. Never modify input parameters after the await.

      • natman

        I can see the reasoning.
        So, let me give you a real example I’m working with and see if you agree with this:
        I have a UserData object with a number of properties, some are server login access data.
        Then there is a method:
        “async void GetUserImages(UserData user, callback userReturn)
        { this returns the UserData object with a collection of images }”.

        Calling this method:
        “await serverConn.GetUserImages(user, userReturned =>
        { aCollection = userReturned.userImageCollection;});”
        That is what I currently do now. Either this is failing in the databinding area (another story altogether of possible incorrect XAML) or because of the nature of the callback, it isn’t setting that collection properly for the databinding.

        Instead I should have the method be:
        “async Task<ObservableCollection> GetUserImages(UserData user)
        { this returns the collection of images using the properties of the user data }”

        Then have it be called in this way:
        “user.userImageCollection = await serverConn.GetUserImages(user);”
        then follow that with setting the UI’s grid collection to the now set userImageCollection property?

        Of course, I’m having difficulties getting binding to the XAML, as I said before, but I think that part can be set aside for the sake of understanding proper async calls.
        I wish I was at home so i could try this out now and see.

        • http://www.tonicodes.net/blog/ Toni Petrina

          You got it, the result of the asynchronous operation is the collection of images.which you then bind to view.

          • natman

            Hi, I finally got to try that implementation we discussed and I have an odd issue arise because of it.
            The response from the server never happens when I call a .PostAsync within that GetUserImages method mentioned in my previous comments.
            I did some digging into the PHP server code (I didn’t write and spent probably too much time learning it) and everything seemed to be functioning properly. It worked before when I was using callbacks and now it doesn’t.

            I have the method called like this:
            “var returnedImages = await serverConn.GetUserImages(user);”
            then I followed with:
            “ObservableCollection coll = new ObservableCollection(returnedImages.result);”
            and when result gets called, it hangs apparently indefinitely, and based on my debugging, at the .PostAsync call, which is “await”.
            But if I remove result, the response is received. still cannot get the results though…
            Any ideas why that would happen within the async method returning a Task?

          • natman

            Scratch that. Just realized what it was by perusing some forums and figuring it out from a sort-of related topic.
            I was making the call to my server class that has that method in the LoadState event of the XAML page…which is the UI thread and forced synchronous, which of course will prevent the response from completing because the UI thread must finish first.
            My solution was to move that code outside of the LoadState event method into a new async method (still in the xaml page code behind) and have that method called within the LoadState method.
            It’s all sorts of confusing, but I think I get it now.
            Thanks again for the initial help!

          • http://www.tonicodes.net/blog/ Toni Petrina

            No problem :) I am glad I was able to help at all. Happy coding.

  • Pingback: Windows 8 Developer Links – 2012-10-11 | Dan Rigby

  • Pingback: Why you should (almost) never write void asynchronous methods?

  • Pingback: The Morning Brew - Chris Alcock » The Morning Brew #1210

  • http://twitter.com/AwkwardCoder Ollie Riches

    to be honest the problem with the void return type for the method ImageCapture could be avoided if this method followed the single responsibilty principle and only did one thing, currently it is creating the file, persisting the stream, persisting the DTO and persisting the data item…

    • http://www.tonicodes.net/blog/ Toni Petrina

      Well, the problem is that the code afterwards actually relied on what it did inside. It did not specify itself as “async”, it modified its input parameters even though the caller might not be around anymore and that it simply did not follow the simple rule – fire and forget.

      Main code not only didn’t forget that it is done, it expected it to be complete after calling it.

      That is why the recommendation is as it is: mark all asynchronous functions with the “Async” suffix and use void only if it is unnamed function or an event handler.

  • http://www.jaylee.org Jérôme Laban

    That’s actually worse than just forgetting that something is happening… Wait until you do have exceptions in there… See for yourself (Ok, shameless plug) : http://jaylee.org/post/2012/07/08/c-sharp-async-tips-and-tricks-part-2-async-void.aspx
    In short: Try *very* hard not to use async void.

    • http://www.tonicodes.net/blog/ Toni Petrina

      I agree, exceptions are a nuisance especially in void async methods.

      I have read your post (I have no problem if you mention your good posts ;) ) and I like it. It seems that my post is a rehash of yours :)