76

I have a .net core API which has a controller that builds an aggregated object to return. the object it creates is made of data that comes from 3 method calls to a service class. These are all independent of each other and can be run in isolation from each other. Currently I am using tasks to improve the performance of this controller. the current version looks something like this...

[HttpGet]
public IActionResult myControllerAction()
{      
    var data1 = new sometype1();
    var data2 = new sometype2();
    var data3 = new List<sometype3>();

    var t1 = new Task(() => { data1 = service.getdata1(); });
    t1.Start();

    var t2 = new Task(() => { data2 = service.getdata2(); });
    t2.Start();

    var t3 = new Task(() => { data3 = service.getdata2(); });
    t3.Start();

    Task.WaitAll(t1, t2, t3);

    var data = new returnObject
    {
         d1 = data1,
         d2 = data2,
         d2 = data3
    };

    return Ok(data);
}

This works well however I am wondering if using tasks is the best solution here? Would using async/await be a better idea and more accepted way?

For example should the controller be marked as async and an await put on each call to the the service methods?

Ben Cameron
  • 4,335
  • 6
  • 51
  • 77
  • 1
    Does `service` not offer asynchronous alternatives for the `getdata` methods? Also, generally you should prefer `Task.Run()` over creating tasks and then manually starting them. – Damien_The_Unbeliever Jan 31 '17 at 08:47
  • 2
    async/await does not help with parallelism in that sense. In fact, it would serialize your 3 service requests. It would help, however, when you do `Task.WaitAll`, since you are parking a thread with that line. – Gabriel Garcia Jan 31 '17 at 08:49
  • 1
    There's no reason at all to create cold tasks then call `Start` on them. They aren't threads. Just use `Task.Run`. Then use `await Task.WhenAll` – Panagiotis Kanavos Jan 31 '17 at 09:06
  • do this :>>> var t1 = new Task(() => { data1 = service.getdata1(); }); var t2 = new Task(() => { data2 = service.getdata2(); }); await Task.WhenAll(t1, t2, t3); – Vaibhav.Inspired Oct 14 '22 at 10:04

3 Answers3

132

This works well however I am wondering if using tasks is the best solution here? Would using async/await be a better idea and more accepted way?

Yes, absolutely. Doing parallel processing on ASP.NET consumes multiple threads per request, which can severely impact your scalability. Asynchronous processing is far superior for I/O.

To use async, first start with your lowest-level call, somewhere inside your service. It's probably doing an HTTP call at some point; change that to use asynchronous HTTP calls (e.g., HttpClient). Then let async grow naturally from there.

Eventually, you'll end up with asynchronous getdata1Async, getdata2Async, and getdata3Async methods, which can be consumed concurrently as such:

[HttpGet]
public async Task<IActionResult> myControllerAction()
{
  var t1 = service.getdata1Async();
  var t2 = service.getdata2Async();
  var t3 = service.getdata3Async();
  await Task.WhenAll(t1, t2, t3);

  var data = new returnObject
  {
    d1 = await t1,
    d2 = await t2,
    d3 = await t3
  };

  return Ok(data);
}

With this approach, while the three service calls are in progress, myControllerAction uses zero threads instead of four.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
  • 4
    Is it not redundant to do await Task.WhenAll() *and* await for each task in the returnObject? Not sure why it wouldn't just be sufficient to do one or the other. Can you please explain? :) – susieloo_ Jul 11 '17 at 23:45
  • 9
    If the return types are different, you have to `await` each task. I prefer `Task.WhenAll` even in this case because it makes the code clearer. It's also a bit more efficient (only resuming on the context once instead of 3 times), but my main reason is code clarity. – Stephen Cleary Jul 12 '17 at 00:46
  • @StephenCleary How to use zero threads if my API need to call System.Diagnostics.Process with WaitForExit? I am planning to implement https://stackoverflow.com/a/10789196/241004 as workaround. Do you have better idea? – Jawad Al Shaikh Oct 04 '17 at 08:25
  • 1
    @JawadAlShaikh: No, that's about the best you can do. `Process` just doesn't have a very async-friendly API. :/ – Stephen Cleary Oct 04 '17 at 10:41
  • @StephenCleary Is this also true if running .NET 4.7 WEB API ? – Stephen Patten Jan 24 '18 at 15:02
  • 1
    @StephenPatten: Yes. – Stephen Cleary Jan 24 '18 at 15:29
17
[HttpGet]
public async Task<IActionResult> GetAsync()
{      
    var t1 = Task.Run(() => service.getdata1());
    var t2 = Task.Run(() => service.getdata2());
    var t3 = Task.Run(() => service.getdata3());

    await Task.WhenAll(t1, t2, t3);

    var data = new returnObject
    {
        d1 = t1.Status == TaskStatus.RanToCompletion ? t1.Result : null,
        d2 = t2.Status == TaskStatus.RanToCompletion ? t2.Result : null,
        d3 = t3.Status == TaskStatus.RanToCompletion ? t3.Result : null
    };

   return Ok(data);
}
  1. Your action thread is currently blocked when you are waiting for tasks. Use TaskWhenAll to return awaitable Task object. Thus with async method you can await for tasks instead of blocking thread.
  2. Instead of creating local variables and assigning them in tasks, you can use Task<T> to return results of required type.
  3. Instead of creating and running tasks, use Task<TResult>.Run method
  4. I recommend to use convention for action names - if action accepts GET request, it's name should starts with Get
  5. Next, you should check whether tasks completed successfully. It is done by checking task status. In my sample I used null values for return object properties if some of tasks do not complete successfully. You can use another approach - e.g. return error if some of tasks failed.
Sergey Berezovskiy
  • 232,247
  • 41
  • 429
  • 459
  • 5
    I think you need to explain the check for `RanToCompletion`. Are you anticipating early cancellation ? Had any of the calls faulted, `await Task.WhenAll` would raise an exception – Panagiotis Kanavos Jan 31 '17 at 09:08
  • Are your sure you can just rename the get method in WebApi? In MVC you can't without altering behavior. – Sefe Jan 31 '17 at 09:43
  • @Sefe yep, you are right. Didn't notice it mvc core, will revert some web-api related changes in a moment – Sergey Berezovskiy Jan 31 '17 at 09:44
3

As i understand, you want this to execute in parallel, so I don't think there is anything wrong with your code. As Gabriel mentioned, you could await the the finishing of the tasks.

[HttpGet]
public async Task<IActionResult> myControllerAction()
{      
  var data1 = new sometype1();
  var data2 = new sometype2();
  var data3 = new List<sometype3>();

  var t1 = Task.Run(() => { data1 = service.getdata1(); });
  var t2 = Task.Run(() => { data2 = service.getdata2(); });
  var t3 = Task.Run(() => { data3 = service.getdata3(); });

  await Task.WhenAll(t1, t2, t3); // otherwise a thread will be blocked here

  var data = new returnObject
  {
      d1 = data1,
      d2 = data2,
      d2 = data3
  };

 return Ok(data);
}

You could also use the results of the tasks to save some lines of codes and make the code overall "better" (see comments):

[HttpGet]
public async Task<IActionResult> myControllerAction()
{      
  var t1 = Task.Run(() => service.getdata1() );
  var t2 = Task.Run(() => service.getdata2() );
  var t3 = Task.Run(() => service.getdata3() );

  await Task.WhenAll(t1, t2, t3); // otherwise a thread will be blocked here

  var data = new returnObject
  {
      d1 = t1.Result,
      d2 = t2.Result,
      d2 = t3.Result
  };

 return Ok(data);
}
Thomas D.
  • 1,031
  • 6
  • 17
  • 1
    The first snippet contains several problems - initializing variables with throwaway objects, then *capturing* those variables. – Panagiotis Kanavos Jan 31 '17 at 09:10
  • I agree that this is not optimal, that's why I provided a "better" solution with my 2nd code snippet. Anyways, I don't think that the 1st snipped has real problems. The compiler will make the throwaway local variables to private fields which should work just fine. (Again, I know it's not ideal.) – Thomas D. Jan 31 '17 at 11:34
  • At least you should use Task for the method type because of async usage – Can PERK Feb 19 '19 at 18:26
  • @CanPERK: you are right, I have added the suggestion. – Thomas D. Feb 22 '19 at 16:43