1

I have inherited some code which was written in web api that I am not sure I understand 100%. It looks similar to this:

public async Task<IHttpActionResult> GetAll()
{
    var items = await repo.GetItems();

    Task.Run(async () =>
    {
        await MakeCallToExternalServiceAsync();

        await MakeCallToAnotherExternalServiceAsync();
    });

    return Ok(items);
}

After the call to the repository to get some data from the data store, the code fires off a task to make some calls to some external services. Assuming none of these calls fail are there any other issues with this? Since the request is finished are these calls going to be aborted? There might be resources that are "request scope" using Autofac - would those get disposed since the request has finished?

Dismissile
  • 32,564
  • 38
  • 174
  • 263
  • Yes, there is an issue with that: https://blog.stephencleary.com/2013/11/taskrun-etiquette-examples-dont-use.html – Crowcoder Jul 21 '17 at 13:14
  • Task.Run will cause more issues than you think, is there a certain use case for using this ? – EasyE Jul 21 '17 at 13:37
  • Yes the use case is to "fire and forget" like Stephen mentioned below. What other issues did he not identify? – Dismissile Jul 21 '17 at 13:59

1 Answers1

3

This is the "poor man's" way of returning early from an ASP.NET request. A.K.A. "fire and forget". There are much better solutions available.

Assuming none of these calls fail are there any other issues with this?

Well, if any of them do fail, you would never know.

Since the request is finished are these calls going to be aborted?

Not necessarily. They may run to completion, or they may be aborted. ASP.NET will not abort them just because the request finishes, but IIS will abort them whenever it decides to recycle the AppDomain / process (which it does do occasionally).

There might be resources that are "request scope" using Autofac - would those get disposed since the request has finished?

Yes. Once the request has finished, using any request-scoped resources is not a good idea.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • 1
    Yeah I'm sure it was done as "let's get this done quickly and figure out a better long-term approach later". Unfortunately later never came. – Dismissile Jul 21 '17 at 13:35
  • `Well, if any of them do fail, you would never know.` I don't totally agree with this you can always attach a global error handler services see here https://stackoverflow.com/questions/16028919/catch-all-unhandled-exceptions-in-asp-net-web-api – BRAHIM Kamel Jul 21 '17 at 13:47
  • @BRAHIMKamel: And that would not catch exceptions from `MakeCallToExternalServiceAsync`. "Fire and forget" literally means "I don't care if this fails." – Stephen Cleary Jul 21 '17 at 15:30