Starting and Forgetting an Async task in MVC Action

Question

I have a standard, non-async action like:

[HttpPost]
public JsonResult StartGeneratePdf(int id)
{
    PdfGenerator.Current.GenerateAsync(id);
    return Json(null);
}

The idea being that I know this PDF generation could take a long time, so I just start the task and return, not caring about the result of the async operation.

In a default ASP.Net MVC 4 app this gives me this nice exception:

System.InvalidOperationException: An asynchronous operation cannot be started at this time. Asynchronous operations may only be started within an asynchronous handler or module or during certain events in the Page lifecycle. If this exception occurred while executing a Page, ensure that the Page is marked <%@ Page Async="true" %>.

Which is all kinds of irrelevant to my scenario. Looking into it I can set a flag to false to prevent this Exception:

<appSettings>
    <!-- Allows throwaway async operations from MVC Controller actions -->
    <add key="aspnet:AllowAsyncDuringSyncStages" value="true" />
</appSettings>

https://mcmap.net/q/1328186/-mvcmailer-sendasync-fails-with-amazon-ses
http://msdn.microsoft.com/en-us/library/hh975440.aspx

But the question is, is there any harm by kicking off this Async operation and forgetting about it from a synchronous MVC Controller Action? Everything I can find recommends making the Controller Async, but that isn't what I'm looking for - there would be no point since it should always return immediately.

Unknot

Answer 1

Relax, as Microsoft itself says (http://msdn.microsoft.com/en-us/library/system.web.httpcontext.allowasyncduringsyncstages.aspx):

This behavior is meant as a safety net to let you know early on if you're writing async code that doesn't fit expected patterns and might have negative side effects.

Just remember a few simple rules:

• Never await inside (async or not) void events (as they return immediately). Some WebForms Page events support simple awaits inside them - but RegisterAsyncTask is still the highly preferred approach.

• Don't await on async void methods (as they return immediately).

• Don't wait synchronously in the GUI or Request thread (.Wait(), .Result(), .WaitAll(), WaitAny()) on async methods that don't have .ConfigureAwait(false) on root await inside them, or their root Task is not started with .Run(), or don't have the TaskScheduler.Default explicitly specified (as the GUI or Request will thus deadlock).

• Use .ConfigureAwait(false) or Task.Run or explicitly specify TaskScheduler.Default for every background process, and in every library method, that does not need to continue on the synchronization context - think of it as the "calling thread", but know that it is not one (and not always on the same one), and may not even exist anymore (if the Request already ended). This alone avoids most common async/await errors, and also increases performance as well.

Microsoft just assumed you forgot to wait on your task...

UPDATE: As Stephen clearly (pun not intended) stated in his answer, there is an inherit but hidden danger with all forms of fire-and-forget when working with application pools, not solely specific to just async/await, but Tasks, ThreadPool, and all other such methods as well - they are not guaranteed to finish once the request ends (app pool may recycle at any time for a number of reasons).

You may care about that or not (if it's not business-critical as in the OP's particular case), but you should always be aware of it.

Hogle

Since it's really meant as a warning, it's unfortunate this can't be presented that way (for example as a warning to the output console in Debug mode). – Unknot

The linked documentation also doesn't hedge very well - they refer to it as: "detects the application misusing the async API." Perhaps this exception needs to be separated from the more insidious danger, "there is still outstanding asynchronous work when an asynchronous module or handler signals completion." – Unknot

yes, it's very unfortunate that there isn't an attribute or some other mechanism to signal that in your case it's the intended (and perfectly valid) behavior - but like the unhandled task exceptions crashing the app in earlier version (better safe than sorry), maybe the next one will relax this requirement as well... – Loxodromics

and even that "insidious danger" is only potentially fatal - if you need the HttpContext back, otherwise it's quite safe – Loxodromics

Without doubt the exception message needs to be modified to be less heavy-handed, saying "If this asynchronous task is intended to be fired and forgotten, you can disable this exception by adding aspnet:AllowAsyncDuringSyncStages to appSettings in Web.Config." Then this would at least feel like the warning it's intended to be. – Unknot

downvoting without explaining why is not constructive at all (especially if you are likely wrong)... – Loxodromics

1) The docs you linked to state the code is "misusing the async API". 2) You shouldn't set the dangerous AllowAsyncDuringSyncStages flag unless you can describe the "negative side effects", which should be a part of your answer. 3) Awaiting inside async void events is supported by ASP.NET, so your first "never" should be "avoid". 4) It's not possible to await on async void methods. 5) There's no GUI thread in this scenario. 6) The error (not warning) is not just because the op "forgot to wait"; it is because they are completing the request when there is still more work to do. – Delphine

@StephenCleary: 1.) you are taking that out of context, and my quote comes from a later, more relevant, part of the document 2.) ok, yes, but i did point out all the ways to avoid them 3.) only some of them, and just for simple tasks, and it's still not recommended (hanselman.com/blog/…) 4.) i can't remember now, but I think that VS2010 with async/await CTP didn't warn you about it 5.) for completeness sake 6.) exactly as he wanted – Loxodromics

One of the "negative side effects" is that the context is no longer valid when the async method returns, as Chris detailed in his answer. Another of the "negative side effects" is that your additional work simply may never complete. Your answer doesn't even mention these problems, much less ways to avoid/mitigate them. – Delphine

@StephenCleary: sigh... ok, I expanded my third bulletin point with the fourth one to fully cover "one" (although they are just two sides of the same synchronization problem), and specifically mentioned "two" (although OP never cared about that) - now would you please kindly remove the downvote (or are you going to nitpick something else)? – Loxodromics

Answer 2

The InvalidOperationException is not a warning. AllowAsyncDuringSyncStages is a dangerous setting and one that I would personally never use.

The correct solution is to store the request to a persistent queue (e.g., an Azure queue) and have a separate application (e.g., an Azure worker role) processing that queue. This is much more work, but it is the correct way to do it. I mean "correct" in the sense that IIS/ASP.NET recycling your application won't mess up your processing.

If you absolutely want to keep your processing in-memory (and, as a corollary, you're OK with occasionally "losing" reqeusts), then at least register the work with ASP.NET. I have source code on my blog that you can drop in your solution to do this. But please don't just grab the code; please read the entire post so it's clear why this is still not the best solution. :)

Delphine

Just to clarify, the job I'm describing in the question is non-essential - if the AppDomain blows up, server melts down, whatever, I don't care. Either I got a completed PDF out of it or I didn't. The user isn't left wondering if it was created - there's a separate call that checks on its status. For that kind of throwaway, I disagree that it's "dangerous." – Unknot

you would be quite correct if he needs to return some kind of response, but in his case he doesn't even need the context at all (did you read the whole question) - so both of your solutions (perfectly valid as they are in most other cases) would here just be a horribly bloated needles overhead... – Loxodromics

@NikolaBogdanović: Not at all; it has nothing to do with whether he needs to return a response; it has to do with whether he needs the background processing to be reliable. When hosted in IIS, AppDomains and application pools are regularly shut down or recycled when there are no active requests. So, my first solution is the only correct answer if the extra processing (PDF generation in this case) is business-critical. And my second solution notifies ASP.NET that the extra processing is going on; it's not "horribly bloated" at all. – Delphine

@ChrisMoschini: In your case, then, you don't need the first solution. However, I do recommend you use the second solution: the source code from my blog allows you to register your background code with ASP.NET so that it is aware of it. – Delphine

@StephenCleary: you are absolutely right of course, but he clearly stated that this is not a business-critical scenario. And other than every 29 hours, app pools are recycled after 20 minutes of the last request, although on shared hosting that can and does happen at any time - so you would be quite right in that case as well. But for a simple quick and dirty fix this is a perfectly valid solution - and sorry for the "horribly bloated" part, but it would have scared away any new adopter of async/await pattern with it's added complexity. – Loxodromics

@NikolaBogdanović: I disagree about scaring away async adopters; async is mostly easy to use on ASP.NET, and I especially recommend keeping the safety nets active for new adopters. This is more about trying to use async for the wrong thing (returning a response when you still have work to do), which I discourage in general. I have two blog posts 1, 2 that go into more detail. – Delphine

@StephenCleary I can't experimentally verify your claims that ConfigureAwait is enough to solve it, so I'm going to have to say I'm skeptical given my solution works in both Production and local development, and yours does not produce useful results - I am fully open to evidence to show otherwise, for example a test case that demonstrates it on github. I'm aware of IRegisteredObject, and I find it inappropriate in this use case. Typically the AppDomain takes about 30 seconds to shut off, and this job is meant to take less time than that anyway; even if it doesn't, if it dies, I don't care. – Unknot

@ChrisMoschini: I'm not sure what you mean about ConfigureAwait; I have never suggested it as a solution for your problem. I do believe IRegisteredObject is appropriate here; it is specifically for the purpose of registering work with ASP.NET that is done outside the scope of a request... which is... well... exactly what you're doing... – Delphine

@StephenCleary Agree to disagree; IRegisteredObject is a waste of effort here. – Unknot

Finally getting a chance to dig through all of this in detail. If the work is safe to die at any time (an ideal way to write any background work anyway), what is the benefit of IRegisteredObject; is there some harm I'm posing to ASP.Net if I have a Task still running when it chooses to tear itself down? To be clear, I don't care if the Task dies abruptly. Either it completes its work, which is easy to check on, or it doesn't, which just means start over. So Task risk doesn't matter in this case - but is there some other outside system risk I'm not seeing? – Unknot

There's no harm to ASP.NET; your task will get a thread abort. If you have a reliable way to detect that this happened, then there's no problem in doing a fire-and-forget. Registering just gives you a cleaner shutdown. Since I posted the original answer, I wrapped the code in a library which has since been obsoleted by .NET 4.5.2. So the ASP.NET team felt this technique was useful enough to include it right in the core. – Delphine

There already is a similar thing as HostingEnvironment.QueueBackgroundWorkItem in previous versions of ASP.Net - it's HostingEnvironment.IncrementBusyCount and here is a simple implementation of mine: bitbucket.org/NikolaBogdanovic/utils/src/… – Loxodromics

Answer 3

The answer turns out to be a bit more complicated:

If what you're doing, as in my example, is just setting up a long-running async task and returning, you don't need to do more than what I stated in my question.

But, there is a risk: If someone expanded this Action later where it made sense for the Action to be async, then the fire and forget async method inside it is going to randomly succeed or fail. It goes like this:

1. The fire and forget method finishes.
2. Because it was fired from inside an async Task, it will attempt to rejoin that Task's context ("marshal") as it returns.
3. If the async Controller Action has completed and the Controller instance has since been garbage collected, that Task context will now be null.

Whether it is in fact null will vary, because of the above timings - sometimes it is, sometimes it isn't. That means a developer can test and find everything working correctly, push to Production, and it explodes. Worse, the error this causes is:

1. A NullReferenceException - very vague.
2. Thrown inside .Net Framework code you can't even step into inside of Visual Studio - usually System.Web.dll.
3. Not captured by any try/catch because the part of the Task Parallel Library that lets you marshal back into existing try/catch contexts is the part that's failing.

So, you'll get a mystery error where things just don't occur - Exceptions are being thrown but you're likely not privy to them. Not good.

The clean way to prevent this is:

[HttpPost]
public JsonResult StartGeneratePdf(int id)
{
    #pragma warning disable 4014 // Fire and forget.
    Task.Run(async () =>
    {
        await PdfGenerator.Current.GenerateAsync(id);
    }).ConfigureAwait(false);

    return Json(null);
}

So, here we have a synchronous Controller with no issues - but to ensure it still won't even if we change it to async later, we explicitly start a new Task via Run, which by default puts the Task on the main ThreadPool. If we awaited it, it would attempt to tie it back to this context, which we don't want - so we don't await it, and that gets us a nuisance warning. We disable the warning with the pragma warning disable.

Unknot

you are correct, but that is a needles overhead - just make sure all of your awaits in PdfGenerator.Current.GenerateAsync are not on the sychronization context (get familiar with .ConfigureAwait(false)) – Loxodromics

you need to fully comprehend my third bulletin point, before doing anything else async/await related - Stephen Cleary below has some very good articles about that... – Loxodromics

@NikolaBogdanović Have you tested that? Running with simply .ConfigureAwait(false) wasn't enough to prevent Task reentry on a null Task context; the above was. I'm not sure why your third bullet point would have any relevance here, since it's about the GUI thread, and this is an ASP.Net Controller Action. – Unknot

GUI thread is just the most frequent example, I was talking about the synchronization context - where exactly did you put .ConfigureAwait(false)? it needs to be on every root await inside your PdfGenerator.Current.GenerateAsync (use the original code from your question) - that is exactly the same as you surrounding it with Task.Run, or surrounding it's whole body with it instead (but with just less needles code). In fact use .ConfigureAwait(false) for every background process or library that doesn't need to continue on the calling thread, as it increases performance. – Loxodromics

And your Task.Run way also introduces a closure on "id" - they are not perfectly reliable, and are even recommended against in a whitepaper about async/await explaining why they added state object parameter to TaskFactory.StartNew() methods (but strangely enough not to Task.Run and PageAsyncTask as well???). I can't find that particular article now, but closures also reduce performance: code.jonwagner.com/2012/09/06/best-practices-for-c-asyncawait – Loxodromics

...still can't find that document, but it talked about Task.ContinueWith overloads instead of TaskFactory.StartNew – Loxodromics

I updated my answer with the fourth bulletin point - not following it is the direct cause of your synchronization context error (and my third bulletin error - btw, GUI and Request are completely interchangeable there) – Loxodromics

@NikolaBogdanović Finally had a chance to test this - .ConfigureAwait(false) is in fact necessary as you noted. Updated answer to reflect this. – Unknot