Is it ok to not await Tasks in Web API Controller

Question

So I've come across some code that makes me uncomfortable, but I can't find a definitive answer as to whether it's actually problematic.

We have a ASP.Net Web API that is primarily used by a message bus. There is a balancing process that needs to be started for several accounts. The balancing service method is asyncronous and returns a Task. The code is called like this:

foreach (AccountingGroup accountingGroup in Groups)
{
    ledgerService.CreateItemsAsync(accountingGroup.GLAccountingHeaderId);
}
return StatusCode(HttpStatusCode.NoContent);

This strikes me as wrong on quite a few levels. I get the intention. "We want to run this method on all of these groups, but we don't need to wait on them to finish.

Obviously CancellationToken's aren't being used. They are relying on AWS to just kill the entire process if it runs to long, and that's not a refactor I can really get into right now anyways.

I've been out of C# for a year an a half, and asynchronous code for 2.5 years and feel like I knew the issue here at some point, but I just can't find it again.

What is the proper way to handle this problem? Is it even a problem?

You've got a problem with your async method to begin with - it is a `void async` method, that's not a great idea https://stackoverflow.com/questions/12144077/async-await-when-to-return-a-task-vs-void — Vidmantas Blazevicius, Mar 15 '18 at 15:16

Scott Chamberlain · Accepted Answer · 2018-03-15T15:20:22.957

4

No it is not ok, the server may shut down the app domain while the background work is running. The best way to handle this is use a library for background work like https://www.hangfire.io/.

If you feel the work will be done within the next minute or so you could use the short term system HostingEnvironment.QueueBackgroundWorkItem(Func<CancellationToken,Task>) however i am not sure if this works with ASP.NET Core or not, it was designed to be used with the previous versions of ASP.NET.

EDIT: Found a reference, QueueBackgroundWorkItem indeed does not work in ASP.NET Core but there is a similar way to handle these situations there.

edited Mar 15 '18 at 15:20

answered Mar 15 '18 at 15:17

Scott Chamberlain

124,994
33
282
431

1

Interesting, but dangerous if you signal the result was okay, when it is still running and you don't know it will even be okay. – Patrick Hofman Mar 15 '18 at 15:17
@PatrickHofman I agree, but as a consultant I think there is also something to be said for minimizing structural changes to an existing project. The core problem is that it really shouldn't be done this way. But I do like having a solution available that fixes the bug without requiring major structural changes. So thanks, Scott. – Riplikash Mar 15 '18 at 16:42
@Riplikash that is the entire point: it does not fix the problem. It hides errors. You don't want that to happen. Take the database server out for a second and see how your code will return success when it actually failed. – Patrick Hofman Mar 15 '18 at 20:09
Patrick's point is why I recommended hangfire first, if your background work fails it will retry a sepecified number of times then log the failure a place where admins can find out about it. `QueueBackgroundWorkItem` and `IApplicationLifetime` do not give you those benefits out of the box. – Scott Chamberlain Mar 15 '18 at 20:15
Scott, thanks for the further information on choosing between QueueBackgroundWorkItems, IApplicationLifetime, and Hangfire. Good to know where the recommendation comes from. @PatrickHofman I don't disagree, but as I said, in a consulting situation I see value finding better ways to handle problems when you can't implement the proper solution. You can't always change an interface to an application, so there is value in even partial solutions that don't require you to change the application interface and work flow. – Riplikash Mar 15 '18 at 20:22

ProgrammingLlama · Answer 2 · 2018-03-15T15:20:07.093

The correct way is to define your API method as async and then wait for all of the async methods to complete:

public Task<IHttpActionResult> DoStuff()
{
    await Task.WhenAll(groups.Select(g =>
                     ledgerService.CreateItemsAsync(g.GLAccountingHeaderId));
    return StatusCode(HttpStatusCode.NoContent);
}

Patrick's answer has an explanation of "why". It seems like a bad idea to pretend to the client that an action has been completed when it has not.

If you want to run these things in the background, you might look into using messages queues like RabbitMq and develop a fail-safe way of ensuring that these tasks are completed. Feedback when things are failing is good. With your current approach, you have absolutely no way to find out if this code is failing, meaning that if it stops working you won't realise until it affects something else.

score 3 · Answer 3 · answered Mar 15 '18 at 15:15

Is it even a problem?

Yes, there is a difference in not wanting to wait for it, and actually be able to handle exceptions.

For example, if your code fails, for whatever reason, you now return a HTTP 204, a success state. If you would await the result, and it fails, you will get an HTTP 500 most likely.

What is the proper way to handle this problem?

You should await the results, for example aggregating the tasks and call Task.WhenAll on them, so you don't have to wait on each and every one of them separately.

Yep. It seems like a bad idea to pretend that the action has been completed when it has not. — ProgrammingLlama, Mar 15 '18 at 15:17

score 0 · Answer 4 · answered Mar 15 '18 at 21:21

0

You can use a QueueBackgroundWorkItem

Please take a look at Getting QueueBackgroundWorkItem to complete if the web page is closed

answered Mar 15 '18 at 21:21

Rodrigo Werlang

2,118
1
17
28

Is it ok to not await Tasks in Web API Controller

4 Answers4