8

We came across a bug in our product and reduced it to the following problem. Given a list and call to the ForEach-Extension method with an async lambda, what is the expected order of the output:

public static async Task Main()
{
    var strings = new List<string> { "B", "C", "D" };
    Console.WriteLine("A");
    strings.ForEach(async s => { await AsyncMethod(s); } );
    Console.WriteLine("E");
}

private static async Task AsyncMethod(string s)
{
    await Task.Run(() => { Console.WriteLine(s); });
}

We expected it to be always A,B,C,D,E. But sometimes it's A,B,C,E,D or A,B,E,D,C

We thought that these two lines would be equivalent:

strings.ForEach(async s => { await AsyncMethod(s); });

foreach (var s in strings) await AsyncMethod(s);

Can somebody explain where the difference is? How are these async lambdas executed and why are they not awaited?

Clarification: The problem is not the order of B,C and D. The problem is that E comes before the loop is finished

tomfroehle
  • 602
  • 5
  • 16

3 Answers3

15
foreach (var s in strings) await AsyncMethod(s);

You're misunderstanding how this works. These are the steps that are taken, sequentially:

  1. Handle "B" asynchronously.
  2. Wait for (1).
  3. Handle "C" asynchronously.
  4. Wait for (3).
  5. Handle "D" asynchronously.
  6. Wait for (5).

The await is part of each iteration. The next iteration won't start until the current one is finished.

Due to not handling the tasks asynchronously, these sequential tasks will finish in the order that they were started.


strings.ForEach(async s => { await AsyncMethod(s); });

This, on the other hand, works differently:

  1. Handle "B" asynchronously.
  2. Handle "C" asynchronously.
  3. Handle "D" asynchronously.

The ForEach starts the tasks, but does not immediately await them. Due to the nature of asynchronous handling, these concurrent tasks can be completed in a different order each time you run the code.

As there is nothing that awaits the tasks that were spawned by the ForEach, the "E" task is started immediately. BCDE are all being handled asynchronously and can be completed in any arbitrary order.


You can redesign your foreach example to match your ForEach example:

foreach (var s in strings) 
{
    AsyncMethod(s);
}

Now, the handling is the same as in the ForEach:

  1. Handle "B" asynchronously.
  2. Handle "C" asynchronously.
  3. Handle "D" asynchronously.

However, if you want to ensure that the E task is only started when BCD have all been completed, you simply await the BCD tasks together by keeping them in a collection:

foreach (var s in strings) 
{
    myTaskList.Add(AsyncMethod(s));
}

await Task.WhenAll(myTaskList);
  1. Handle "B" asynchronously and add its task to the list.
  2. Handle "C" asynchronously and add its task to the list.
  3. Handle "D" asynchronously and add its task to the list.
  4. Wait for all tasks in the list before doing anything else.
Flater
  • 12,908
  • 4
  • 39
  • 62
  • The second snippet you've showed doesn't wait for all of the tasks, as you claim it does. – Servy Feb 23 '18 at 16:06
  • Your second example is incorrect, there is nothing that waits for 1, 2, 3. The method immediately continues past the `ForEach` since it returns an `async void`. – JSteward Feb 23 '18 at 16:06
  • @Servy: I was already correcting it, you are right. I got too focused on explaining why the `foreach` awaited every single iteration, that I lost sight over the lack of awaiting in the `ForEach` :) – Flater Feb 23 '18 at 16:07
  • @JSteward: See comment above. – Flater Feb 23 '18 at 16:07
  • 1
    Great answer! The pitfalls can sometimes be overlooked with similar read-able code(i.e. `.ForEach` vs `foreach`). – Jon Douglas Mar 18 '19 at 16:45
5

ForEach does not support an async delegate since it takes an Action<T>. That reduces your method to un-awaitable async void. ForEach was never intended for a Func<Task> or any other async variant. Calling .Wait on AsyncMethod will cause a deadlock with a single threaded synchronization context. You have a few choices though:

JSteward
  • 6,833
  • 2
  • 21
  • 30
  • I think this is the key point for me. You can put async delegates in ForEach but they are simply called and not awaited. – tomfroehle Mar 02 '18 at 23:35
1

The issue is caused by not waiting for the task to complete on each string of the list.

      Console.WriteLine("A");
      strings.ForEach( s => { AsyncMethod(s).Wait(); });

      Console.WriteLine("E");

The above solution is working properly. Hope it helps!

Indrit Kello
  • 1,293
  • 8
  • 19
  • 6
    `ForEach` is not intended for use with `async` methods and calling `.Wait()` can result in a deadlock. – JSteward Feb 23 '18 at 15:54