4

I have this piece of code in my .netcore application

[HttpPost]
[Route("doSomething")]
public async Task<IActionResult> DoSomethingAsync([FromBody] Input input)
{
    // Do Something
    var task1 = Task.Run(async () => 
    {
        await taskFactory.DoTask(input);
    });

    // Do Something Differently
    var task2 = Task.Run(async () => 
    {
        await taskFactory.DoAnotherTask(input);
    });

    await Task.WhenAll(task1, task2);

    return Accepted();
}

DoTask() and DoAnotherTask() are both independent of each other and can be executed in parallel but they have to be awaited until both of them are in completed status.

So, I created two tasks and awaited them using Task.WhenAll().

But I have got a review comment to not use Task.Run() in an async method as it can lead to thread pool starvation.

Question 1: How does my code leading to thread pool starvation?
Question 2: If it is leading to thread pool starvation, how can I run both tasks in parallel?

Uwe Keim
  • 39,551
  • 56
  • 175
  • 291
Code-47
  • 413
  • 5
  • 17
  • Why are you doing `Task.Run` here instead of just returning the task from `DoTask`? – DavidG Apr 22 '20 at 15:20
  • Also, how have you diagnosed that ThreadPool starvation is your issue? – DavidG Apr 22 '20 at 15:22
  • 1
    Note how you don't `await` the result from `Task.Run` and instead capture the task to use in a `Task.WhenAll` you can also just do the same for the results from `DoTask` and `DoAnotherTask` instead. – juharr Apr 22 '20 at 15:22
  • @DavidG Yes, that can be avoided. It is unnecessary. But just want to understand how will it lead to thread pool starvation. – Code-47 Apr 22 '20 at 15:22
  • The only reason to wrap these calls in `Task.Run` is if they have CPU bound code up front before they get to whatever `async` code they are doing. – juharr Apr 22 '20 at 15:26
  • The thread pool has a finite number of available threads. `Task.Run` queues work to run on the thread pool, so it's possible to use up all the threads, if it is over used. – Johnathan Barclay Apr 22 '20 at 15:26
  • @JohnathanBarclay juharr Thanks, that's what I wanted to know. – Code-47 Apr 22 '20 at 15:32
  • @juharr JohnathanBarclay you can consolidate you statements. I have got the answer. – Code-47 Apr 22 '20 at 15:49

2 Answers2

3

To answer your question with confidence we must know the implementation of the DoTask and DoAnotherTask methods. Without knowing it we could just assume that they are implemented properly and follow the etiquette for async methods, which is to return a Task immediately, without blocking the calling thread. Under this assumption, the answer is: No, your code doesn't lead to thread pool starvation. This is because the ThreadPool thread employed by Task.Run has a negligible amount of work to do, which is just to create a Task object, so it will be returned back to the ThreadPool almost immediately.

It should be pointed out that although wrapping well behaved async delegates with Task.Run has negligible impact to the health of the ThreadPool, it offers no benefit either. Take a look at this semi-related question: Is Task.Run considered bad practice in an ASP .NET MVC Web Application?

Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
2

For your thread pool starvation question, if you run a task, which is already async, executes 2 new Tasks, with task.run and inside that you run 2 async methods you have per call 5 Tasks, then you await the completion of both and you are at 6 Tasks per Request.

I usually do smth like this, you still have 4 Tasks, but in the end the pool would last longer.

[HttpPost]
[Route("doSomething")]
public async Task<IActionResult> DoSomethingAsync([FromBody] Input input)
{
    // Do Something
    var t1 = taskFactory.DoTask(input);

    // Do Something Differently
    var t2 = taskFactory.DoAnotherTask(input);


    await Task.WhenAll(t1, t2);

    return Accepted();
}

Isparia
  • 682
  • 5
  • 20
  • 3
    That may be a better way to do it, but doesnt answer the OP question regarding whether thread pool starvation is at risk like his code reviewer told them it was. – pinkfloydx33 Apr 22 '20 at 15:30