1

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?

Improve this question
1

4 Answers 4

Reset to default
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.

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

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.
@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 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'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, 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.
3

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.

Improve this answer

1 Comment

Yep. It seems like a bad idea to pretend that the action has been completed when it has not.
3

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.

Improve this answer

Comments

0

You can use a QueueBackgroundWorkItem

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

Improve this answer

Comments

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.