2

We have Web API controllers that look like this:

public class HomeController : ApiController
{
    Logger logger = new Logger();

    [HttpGet, Route("")]
    public IHttpActionResult Index()
    {
        logger.Info("Some info logging");

        return Ok();
    }
}

Internally, our loggers use the HttpClient to POST data to an Azure Event Hub. This means that the logger has a synchronous method which internally calls an asynchronous method.

The broken implementation

When our logger has a private method with an async void signature like this:

public class Logger
{
    public void Info(string message)
    {
        LogAsync(message);
    }

    private async void LogAsync(string message)
    {
        await PostAsync(message);
    }
}

We see the following error:

Yellow screen of death

The working implementation

If we change our logger to use an async Task signature like this it works:

public class Logger
{
    public void Info(string message)
    {
        LogAsync(message);
    }

    private async Task LogAsync(string message)
    {
        await PostAsync(message);
    }
}

The question

In both cases our logging messages are sent to the Event Hub. I'd like to know why async Task allows the controller to return the expected result, and async void results in an InvalidOperationException?

Also, is there any best practice regarding calling into asynchronous code from a synchronous method?

I've posted a sample solution here: https://github.com/kevinkuszyk/async-task-vs-async-void-sample

Kevin Kuszyk
  • 1,958
  • 24
  • 37
  • 2
    [Don't use async void](http://haacked.com/archive/2014/11/11/async-void-methods/) – stuartd Dec 06 '16 at 16:54
  • 2
    Why do you have to call it from a synchronous method in the first place? If the underlying operation is asynchronous, make the operation which invokes it `async`. That's why `async` exists. – David Dec 06 '16 at 16:55
  • 2
    Both of the implementations are broken. One tells you that it's broken so you can fix it, the other one just doesn't work and doesn't tell you that it doesn't work so you don't realize that it's not working. – Servy Dec 06 '16 at 16:56
  • @stuartd yes, but why? – Kevin Kuszyk Dec 06 '16 at 16:56
  • @KevinKuszyk lots of info [here](https://msdn.microsoft.com/en-us/magazine/jj991977.aspx) – stuartd Dec 06 '16 at 16:59
  • @David it's a logging framework. It needs to be synchronous so that consuming code can log and move on. The mechanics of how the logs are send (in our case on a background thread) should be abstracted away from consumers. – Kevin Kuszyk Dec 06 '16 at 16:59
  • I literally have no idea so I am only going to speculate: but would returning `void` just execute the method and then forget about it but `Task` would encapsulate the async context into something that can be managed by .NET? – Michael Coxon Dec 06 '16 at 17:00
  • 2
    @MichaelCoxon Returning a task that you just then drop on the floor isn't any better though. It's only useful when the consumer actually uses it to determine when the operation finishes. – Servy Dec 06 '16 at 17:03
  • 2
    @KevinKuszyk: `"It needs to be synchronous so that consuming code can log and move on."` - I'm not entirely following the reasoning there. If the operation advertises itself as being synchronous, then consuming code is going to expect to wait for it to complete. Attempting to hide an asynchronous operation sounds like an attempt at a kind of "fire and forget" methodology. But the "forget" part of that methodology means that you don't care if the operation succeeds or not. So why handle errors in the first place? – David Dec 06 '16 at 17:04
  • So in other words, I just need to drop that "by .NET" from my previous comment and I hit the nail on the head? – Michael Coxon Dec 06 '16 at 17:05
  • 1
    @MichaelCoxon: This is perhaps an oversimplification, but I generally describe it as... An asynchronous operation can only be observed, guaranteed, responded to, etc. if it can be awaited. Tasks can be awaited, `void` can not. `async void` doesn't just ignore the result, it ignores the fact that there was an operation in the first place. Consuming code can't do anything with that operation, because as far as it knows there wasn't an operation. (`async void` exists *only* to support async forms events, and I'm sure that was a begrudging acceptance by the language team.) – David Dec 06 '16 at 17:09
  • @David don't forget that not all logging is for errors. We have a lot of info and verbose log messages across the code base. As I mention [below](http://stackoverflow.com/a/41003224/1288481), the example here is a simplification. The short of it is we want calling the logger to be synchronous, with separate listener sending the messages to Azure. – Kevin Kuszyk Dec 07 '16 at 08:55
  • @KevinKuszyk: That's all well and good, but you're still trying to tell the application host that no async operation is taking place when in fact one is. The result of that will potentially behave differently in different application hosts, and can manifest as difficult and inconsistent bugs. A buggy and unstable logging system sounds like a recipe for trouble. I just don't see why the logger contract can't be async. Then the consuming code can determine how to handle it based on the needs of the application. – David Dec 07 '16 at 11:34
  • @David have a look at this short twitter conversation: https://twitter.com/Smudge202/status/718459310850879489 – Kevin Kuszyk Dec 07 '16 at 12:00
  • @KevinKuszyk: If the calls being made to the logger by the application are synchronously writing to a local (fast) location, why do you need an async operation in that stack in the first place? Wouldn't the background thread/process/etc. be using the async operation (and doing so asynchronously as it should)? That Twitter conversation sounds reasonable, but it doesn't sound like what you're doing in this case. – David Dec 07 '16 at 12:16

2 Answers2

1

Avoiding judgement on the merits of this approach, the answer to your actual question is in the answer to this question.

There are various ways you could fix it while maintaining that usage pattern:

  • Don't await in LogAsync()
  • Use Task.Run(() => LogAsync());
Community
  • 1
  • 1
sellotape
  • 8,034
  • 2
  • 26
  • 30
  • Thanks @sellotape - the question you link to provides the background information I was missing. The example above is a simplification to repro the issue we have. In our actual implementation the logger writes to a `TraceSource` and we have a listener plugged in on the other end which sends the messages to the event hub. The contract on `TraceListener` means our entry point is a synchronous method. As I mentioned above we use `HttpClient` which is asynchronous to send the messages to Azure, hence the problem. I would also be interested in your thoughts on this approach. – Kevin Kuszyk Dec 07 '16 at 08:48
-1

If you call an async method synchronously call it as follows:

async Task MyMethod()
{
   ...
}

public void WaitOnTask()
{
  var resultTask = MyMethod();
  resultTask.GetAwaiter().GetResult();
}

async void should be avoided because there is nothing stopping the method returning immediately and this causes problems when the caller is not expecting the method to return without the asynchronous process completing as is the case in the ASP.NET runtime.

toadflakz
  • 7,764
  • 1
  • 27
  • 40