Rick Strahl's Weblog  

Wind, waves, code and everything in between...
.NET • C# • Markdown • WPF • All Things Web
Contact   •   Articles   •   Products   •   Support   •   Advertise
Sponsored by:
West Wind WebSurge - Rest Client and Http Load Testing for Windows

Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware


:P
On this page:
Edit this Post

This post falls into the category of stupid developer mistakes that are difficult to track down. In this post I'll discuss a nasty bug I ran into with my code, and which I totally misdiagnosed at first. It refers to an intermittent failure of HTTP requests in a custom middleware component where I would get HTTP errors even though the actual response apparently was received properly.

Check out this request trace in Fiddler:

Notice that the response code is a 200 response, but yet it shows with an error icon next to it. Per Eric Lawrence (the original author of Fiddler) I also checked the Session properties:

which seems to suggest that the response was aborted (X-ABORTED-WHEN).

In the browser this page renders... blank. It comes up as a totally white page - not as an error, just blank. This happens both with and without Fiddler attached.

Looking at the actual response output in Fiddler, it looks like the HTML output is all there and matches the content length generated on the server. And yes, I logged the actual content size sent to the client and it matches the Content-Length header the client sees in Fiddler.

What... the... heck???

As you can imagine, it took a while and a lot of false starts to find the culprit. Apparently ASP.NET Core is cutting the connection before the final output is sent, even though it appears that all bytes have been received per Fiddler.

And... even worse: it works most of the time - it only fails occasionally on the same pages, creating the exact same content. Most of the times it works, sometimes it doesn't. A timing issue most likely... Lovely!

(a)Wait for me!

It turns out that in this application I failed to call an output generation method with an await statement.

The response writing method in question looks like this:

 public async Task SendResponse()
 {  
     // pick up response data, add headers, re-route file transfers etc.
     ...
    
     // eventually writes manipulated bytes into the output stream
     await Response.Body.WriteAsync(msResponseOutput.ToArray(),
                    (int) ContentPosition,
                    (int) msResponseOutput.Length - (int) ContentPosition);
 }

And the method should then be called like this:

await  SendResponse();    // internally eventually does `await Response.WriteAsync(outputBytes)`

But because this is legacy code that was moved from an old HttpHandler I forgot to add the await in this call and left it as it originally was:

SendResponse();   // sometimes no workey

This is perfectly valid C# code, although in this case not correct in terms of behavior.

And that right there is the culprit for the intermittent failures.

I had this code running for some time on .NET Core 2.1 without issues even without the await in place, but in .NET Core 3.1 the intermittent errors kept cropping up. .NET Core 3.1 and later is more strict with its async output generation as requests go through the IHttpFeatures pipeline rather than to direct Response output which seems to exacerbate the problem.

In most cases the latter code actually works fine, and only occasionally would the code fail with the odd errors described above.

This appears to be a timing issue - it works when the writing completes before ASP.NET shuts down the actual connection, or it fails if the entire response has not been written. Note that no error appears in the ASP.NET application if the latter occurs - it fails silently.

But Rick - don't you look at Warnings?

Fair question.

Chances are your Visual Studio, Rider and OmniSharp development environments/tools all should flag this call:

SendResponse();   // sometimes no workey

with a warning in that async call is called without awaiting the result. And they did. Now that I know where to look, I could see the warning at the scene of the crime:

or in the warning list:

So the tooling can help avoid this error and in most cases that's probably a pretty obvious catch.

In my defense (ha!), I missed this because it's a legacy project that has been ported from a classic ASP.NET HttpHandler to an ASP.NET Core middleware component. Because this code is ancient there are tons of [Obsolete], documentation and unused variable warnings etc. and the SendResponse() await warning was simply lost in the shuffle of a nearly 200 warnings. I've since fixed that (as you can see in the screen shot above), but it looked quite different before when I was trying to track down the bug.

This was made even more insidious by the fact that the original code in .NET Core 2.1 worked just fine even without the await call, meaning this error cropped up a year or so after I had originally ported the code and had been running it successfully for that long.

That'll teach me to clean up even legacy code warnings or at least slog through huge numbers of warnings for that one critical one. Right...

Off Topic: Async Cascade

I mentioned that this project was a port of an old, old HttpHandler moved from a classic ASP.NET application server. This process was surprisingly easy to work through with easily 90%+ of the code migrating straight across even though it includes a ton of legacy technology (COM) integration.

The one thing that didn't 'just work' was the async requirement of ASP.NET Core's output generation. The old code - which originally was written for .NET 1.1 and then moved through generations of .NET through 4.5+ - didn't use async output, so all the code at the middleware API 'seam' needed to be changed to async. It's not too difficult to do but it's a thankless task: Just when you think "I've got them all!" there will be another async method you need to convert. And another, and another 😄

Initially there was one method that required Async access (SendResponse()) but then it turned out that the cascade required every single method that touched the HTTP output had to be made async as well as all the methods that call them. 90% of the original HttpHandler code ported without changes except for the changes around async conversion.

And that's precisely where this error above was introduced. As I went through and updated calls I missed that one which ironically was the most important one through which most of the output in this middleware flows.

The Moral of the Story

The moral of this story is this: Make sure your Async methods are actually called asynchronously especially if it deals with ASP.NET Core Response output. Without it, the code might work - or, it might not! It depending on timing of the async code running making for some really crazy inconsistent errors. Chances are if you screw this up, your IDE will catch this as a warning and hopefully you won't have the error clusterf*ck that I had to obscure the error.

The second moral is directed at myself: Clean up your code, even if it's ported legacy code. I ended up doing this in this project after I figured out the problem and realized that a little housekeeping would have probably saved me a few hours of troubleshooting trying to track down exactly what was happening... well, next time I'll be wiser, right? (maybe)

this post created and published with the Markdown Monster Editor
Posted in ASP.NET  

The Voices of Reason


 

Matthias
September 15, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

Had something like this just a while ago:

long currentUserId = _userRepository.GetCurrentUser().Id;

Forgot the await and got the Task id. 😅


Cepewka
September 15, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

Great post! Unfortunately, it once again shows that the async/await is like a cancer spreading throughout the codebase and if functions are not colored properly then nasty errors pop up. I wish they just implemented it using fibers + implicit await in the callsite, this is the most common scenario and now it takes so much code to do it right (await Foo().ConfigureAwait(false), most likely with .WithTimeout() or something similar).


Bhanu Chhabra
September 15, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

Another fair warning to others reading this blog post.

Be careful with HttpClient and how it is configured to used as well as where it is being called!!!! Specially with async await.


Alexei
September 15, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

Great post! Thank you! I agree with idea of cleaning warnings for legacy code, but sometimes it is not feasible. I think at least grouping of warnings by type and looking at what types of warnings code had would help


Rick Strahl
September 16, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

@Capewka - Implicit call site handling of await calls would also be heavy handed. I actually have a number of scenarios where I don't await async code and that's a good thing. For example, in that same bit of middleware there's some Web Socket handling that is async and runs on a background thread, and that code can safely execute without an await while in other locations that same method can and should use an await.

And to be fair - as I point out in the post - the tooling does catch this inconsistency and warns. In my case I was unable to find it because of - warning overload (ie. my own fault). In most cases you'd see a fairly obvious warning message that you'd likely catch.


Carl
September 16, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

I've experienced this same hell.

You could set things up so that CS4014 is an error instead of a warning and then suppress the message for the places (rare probably) where you actually don't want to await.


Frank3
September 16, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

Ahh, nasty. 😛 These types of conversions might warrant the treatment of those warnings as errors at compile-time!


Kerem Ispirli
September 16, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

The first thought I had was the convention of appending names of the async methods with "Async". From Microsoft docs:

**You should add "Async" as the suffix of every async method name you write.

This is the convention used in .NET to more easily differentiate synchronous and asynchronous methods.**

It could help in 2 ways:

  1. Name change of the method would draw attention to every call site during review.
  2. Method name with "Async" suffix would highlight the need for "await" keyword.

Rick Strahl
September 16, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

@Kereem - yup, usually I agree with that. In this case because it was legacy code that was converted that didn't happen because the code was essentially upgraded. Could have happened but didn't in the melee that was going on while working the Async cascade 😄

Also frankly I'm not sure anymore whether I actually left the original code to explicitly not be awaited because it worked perfectly fine without it and there is quite a bit cleanup code that happens post Write that actually benefits from running in parallel. It worked reliably in 2.x - started failing only in 3.x which is essentially a time bomb and was definitely a bad idea (if that what the thinking was on my part). Good example of code debt, collected in kind 😢


PilotBob
September 19, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

For example, in that same bit of middleware there's some Web Socket handling that is async and runs on a background thread, and that code can safely execute without an await while in other locations that same method can and should use an await.

I thought using asyc without await for this type of thing was a code smell. Async isn't the same as multi-threaded. If an exception occurs in the non-awaited task it could crash the process.

Or have things changed?


Rick Strahl
September 19, 2020

# re: Don't get burned by missing await Calls for Async Code in ASP.NET Core Middleware

@PilotBob - That depends on what the process is doing and how it handles the operation.

await is just returns a task. If you assign a task and move on, it behaves as any other separate thread (or local depending on whether the task is already complete) would. An exception on a separate non-awaited async thread is likely to fail silently unless it's a critical system error.

I think the recommended way to signify that is to use code like this:

var _ = WriteOutputAsync();

This avoids the compiler warning, plus gives a visual signal that you are explicitly ignoring the result of a call and forget operation.

It should be very rare that this is done - async is usually there for a reason, but there are times when code can go on without waiting for completion.


West Wind  © Rick Strahl, West Wind Technologies, 2005 - 2024