Task null using Task.Run and Parallel.For

Question

I have two services that ultimately both update the same object, so we have a test to ensure that the writes to that object complete (Under the hood we have retry policies on each).

9 times out of 10, one or more of the theories will fail, with the task.ShouldNotBeNull(); always being the assertion to fail. What am i getting wrong with the async code in this sample? Why would the task be null?

    [Theory]
    [InlineData(1)]
    [InlineData(5)]
    [InlineData(10)]
    [InlineData(20)]
    public async Task ConcurrencyIssueTest(int iterations)
    {
        var orderResult = await _driver.PlaceOrder();

        var tasksA = new List<Task<ApiResponse<string>>>();
        var tasksB = new List<Task<ApiResponse<string>>>();

        await Task.Run(() => Parallel.For(1, iterations,
            x =>
            {
                tasksA.Add(_Api.TaskA(orderResult.OrderId));
                tasksB.Add(_Api.TaskB(orderResult.OrderId));
            }));

        //Check all tasks return successful           
        foreach (var task in tasksA)
        {
            task.ShouldNotBeNull();

            var result = task.GetAwaiter().GetResult();

            result.ShouldNotBeNull();
            result.StatusCode.ShouldBe(HttpStatusCode.OK);
        }

         foreach (var task in tasksB)
        {
            task.ShouldNotBeNull();

            var result = task.GetAwaiter().GetResult();

            result.ShouldNotBeNull();
            result.StatusCode.ShouldBe(HttpStatusCode.OK);
        }

    }
}

What is the purpose of awaiting a task that just blocks on a parallel for loop? — p3tch, Aug 20 '18 at 15:05

Liam · Accepted Answer · 2018-08-21T07:51:40.917

There's no need for Tasks and Parrallel looping here. I'm presuming that your _api calls are IO bound? You want something more like this:

var tasksA = new List<Task<ApiResponse<string>>>();
var tasksB = new List<Task<ApiResponse<string>>>();

//fire off all the async tasks
foreach(var it in iterations){
   tasksA.Add(_Api.TaskA(orderResult.OrderId));
   tasksB.Add(_Api.TaskB(orderResult.OrderId));
}

//await the results
await Task.WhenAll(tasksA).ConfigureAwait(false);

foreach (var task in tasksA)
{
    //no need to get GetAwaiter(), you've awaited above.
    task.Result;
} 

//to get the most out of the async only await them just before you need them
await Task.WhenAll(tasksB).ConfigureAwait(false);

foreach (var task2 in tasksB)
{
     task2.Result;
}

this will fire all your api calls async then block while the results return. You Parallel for and tasks are just using additional thread pool threads to zero benefit.

If _api is CPU bound you could get benefit from Task.Run but I'm guessing these are web api or something. So the Task.Run is doing nothing but using an additional thread.

Thank you, that's roughly what i had first time i wrote the code, but a colleague suggested i use the Parallel.For. — MrBliz, Aug 20 '18 at 15:20
It depends on if you process is CPU or IO bound. APIs are typically IO bound, when this is the case `Task`s add no benefit. — Liam, Aug 20 '18 at 15:21

d.moncada · Answer 2 · 2018-08-20T15:19:16.963

1

As others have suggested, remove the Parallel, and await on all tasks to finish before asserting them.

I would also recommend to remove .Result from each task, and await them instead.

public async Task ConcurrencyIssueTest(int iterations)
{
    var orderResult = await _driver.PlaceOrder();

    var taskA = _Api.TaskA(orderResult.OrderId);
    var taskB = _Api.TaskB(orderResult.OrderId);

    await Task.WhenAll(taskA, taskB);

    var taskAResult = await taskA;

    taskAResult.ShouldNotBeNull();
    taskAResult.StatusCode.ShouldBe(HttpStatusCode.OK);

    var taskBResult = await taskB;

    taskBResult.ShouldNotBeNull();
    taskBResult.StatusCode.ShouldBe(HttpStatusCode.OK);
}

edited Aug 20 '18 at 15:19

answered Aug 20 '18 at 15:15

d.moncada

16,900
5
53
82

1

why await the tasks twice? `Task.WhenAll` will ensure that they've returned. The `await taskA` is essentially identical to `taskA.Result` – Liam Aug 20 '18 at 15:16
1

the await the second time is simply just getting the result from them. it is typically recommend over .Result() https://stackoverflow.com/questions/24623120/await-on-a-completed-task-same-as-task-result – d.moncada Aug 20 '18 at 15:19
*Recommended* is a bit strong. It's different. But the benefits are marginal. [*There are two reasons why I **prefer**.....*](https://stackoverflow.com/a/24657079/542251) – Liam Aug 20 '18 at 15:20

Task null using Task.Run and Parallel.For

2 Answers2