-1

Given this Entity Framework Core snippet

using (var scope = serviceProvider.CreateScope()) {
    var ctx = scope.ServiceProvider.GetRequiredService<IEFCoreDataContext>();
//  async operations on ctx and entity (see below for entity)
    await ctx.SaveChangesAsync();
}

And a List<> of entities I want to use it in a concurrent way. I first tried

await Task.Run(() => Parallel.ForEach(list, async entity => {
    <snippet>
});

This fails in SaveChangesAsync for concurrency issues (DataContext). Then I tried

await Task.WhenAll(
    list.Select(
        async entity => {
            <snippet>
        }
    )
);

And this succeeds. I know they do things differently (ForEach partitions the input list, ...), however I would like to understand while the Foreach version fails.

Franco Tiveron
  • 2,364
  • 18
  • 34
  • 1
    @IvanStoev Task.WhenAll does actually work with an IEnumerable, no need to materialize it (https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.whenall?view=net-5.0#System_Threading_Tasks_Task_WhenAll_System_Collections_Generic_IEnumerable_System_Threading_Tasks_Task__) – Franco Tiveron Jun 10 '21 at 07:25
  • What is the target database? Could it be [Oracle](https://stackoverflow.com/questions/29016698/can-the-oracle-managed-driver-use-async-wait-properly) by any chance? – Theodor Zoulias Jun 10 '21 at 07:55
  • database is SQL Server – Franco Tiveron Jun 10 '21 at 07:56
  • 1
    As a side note, the `Parallel.ForEach` method [is not async-friendly](https://stackoverflow.com/questions/23137393/parallel-foreach-and-async-await). Feeding the `Parallel.ForEach` with async delegates is a bug. It doesn't do what you expect it to do. – Theodor Zoulias Jun 10 '21 at 08:00
  • A possible reason that the `await Task.WhenAll(...` succeeds is that the asynchronous operations are actually performed synchronously, and so they are serialized. Could you verify that the `SaveChangesAsync` operations are indeed executed concurrently, using logging or other means? – Theodor Zoulias Jun 10 '21 at 08:06
  • In know it is not async friendly, however this doesn't make it a bug! "async doesn't work well with ForEach" is not equivalent to "async in ForEach is a bug". If it is, the compiler should forbid it. – Franco Tiveron Jun 10 '21 at 08:10
  • DbContext is not thread safe, do you know that? – Svyatoslav Danyliv Jun 10 '21 at 08:13
  • @SvyatoslavDanyliv that's why the first thing in the snippet is create a new scope, so that every concurrent operation works in its own context. – Franco Tiveron Jun 10 '21 at 08:15
  • *"This fails in SaveChangesAsync for concurrency issues (DataContext)."* Could you include in the question more info about the error? (exception type and message) – Theodor Zoulias Jun 10 '21 at 08:25
  • I see, looks correct. Anyway when i see such attempts to work with EF - something went wrong earlier. Why you just not call Task.Run and dd tasks to the list? – Svyatoslav Danyliv Jun 10 '21 at 08:26
  • "*A [software bug](https://en.wikipedia.org/wiki/Software_bug) is an error, flaw or fault in a computer program or system that causes it to produce an incorrect or unexpected result, or to behave in unintended ways.*" So whether the Parallel.ForEach+async is a bug depends on whether you expect it to launch the asynchronous operations and return without awaiting them, and so without propagating any failures, rendering ineffective any try-catch around the parallel loop. I bet that this was not your intention! – Theodor Zoulias Jun 10 '21 at 08:35
  • @TheodorZoulias In https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.parallel.foreach?view=net-5.0 there is no indication that "Feeding the Parallel.ForEach with async delegates is a bug". If we call ForEach with an int instead of an action, the compiler clearly indicates the error, but it doesn't with and async. There is certainly a bug, but not where you indicated. – Franco Tiveron Jun 10 '21 at 10:13
  • @TheodorZouliasThe error is an exception with message like "SaveChanges expected to affect x rows but instead it affected y" – Franco Tiveron Jun 10 '21 at 10:16

2 Answers2

2

I would like to understand while the Foreach version fails.

ForEach doesn't understand async lambdas. Since the ForEach parameter is of type Action, the async lambda is converted to an async void method. One of the problems with async void methods is that it's not easy for the calling code to determine when it has completed, so the ForEach loop appears to end "early" - before the async void lambdas have completed.

async void was added to the language to enable async event handlers, but this is not an event handler, so it's not correct to use async void lambdas here.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810
1

Turns out this seems to be in need of improvement on the .NET side, as for example:

  • Compiler: detect and forbid async void use (direct or as result of conversion as in this case) outside of async event handlers.
  • Documentation: clearly state in the official documentation that Parallel.ForEach should not be called with async handlers.
  • Documentation: if using async is possible but needs particular attention, show example(s) of working/correct use.

EDIT

Thanks to Theodor Zoulias's comment, it turns out that ForEach seems to be designed to work only for synchronous handlers, while a ForEachAsync is on the way. Regardless Microsoft willing, it would be a good thing if the C# compiler pops an error when an async handler is passed as ForEach input

Franco Tiveron
  • 2,364
  • 18
  • 34
  • I don't know if Microsoft would be willing to add a warning in the documentation of `Parallel.ForEach` (you could [try](https://github.com/dotnet/runtime/issues/new/choose) to make it happen if you want), but the good news is that a new API [`Parallel.ForEachAsync`](https://github.com/dotnet/runtime/issues/1946) will be probably included in the upcoming .NET 6, designed specifically for accepting acync delegates. Hopefully this new addition will make the `Parallel.ForEach` less of a pit of failure that it currently is. – Theodor Zoulias Jun 10 '21 at 22:46