Task.WhenAll with LINQ select

Question

long[] orderIds={10,11,12,13};

var orders= new List<Order>();

await Task.WhenAll(orderIds.Select(async (orderId) =>
{
    var orderDetails = await GetOrderDetails(orderId);

    if (orderDetails != null)
        orders.Add(orderDetails);
}));

I am getting some wrong behavior with this, after deployement this code to server its working fine but on local sometimes it is adding all orders but sometime it missed some orders.

Can anybody please help to short out this, not sure what I am missing.

Its not thread safe, either use a [Concurrent Collection](https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent?view=netframework-4.8), or return OrderDetails to add afterwards (which would simplify what you are doing somewhat) — TheGeneral, Feb 28 '20 at 06:46
`orders.AddRange( ( await Task.WhenAll( orderIds.Select( id => GetOrderDetails(id) ) ) ).Where( o => o != null ) );` — Sir Rufo, Feb 28 '20 at 07:04
You can use ConcurentBag instead of List https://learn.microsoft.com/en-us/dotnet/api/system.collections.concurrent.concurrentbag-1?view=netframework-4.8 — pix, Feb 28 '20 at 07:28

John Wu · Accepted Answer · 2020-02-28T07:56:50.210

4

First, I recommend forcing enumeration of the tasks (e.g. an extra ToList() call) before sending them anywhere. You wouldn't want to accidentally enumerate the list more than once.

var tasks = orderIds.Select( orderId => GetOrderDetails(orderId) ).ToList();

Then await the tasks:

await Task.WhenAll( tasks );

And voila, your orders are ready.

var orders = tasks.Select( t => t.Result ).Where( o => o != null ).ToList();

BTW nothing is being mutated so you're completely thread-safe without any need for locks (assuming GetOrderDetails is thread-safe). This is one of the advantages of a functional approach to using linq and in general.

edited Feb 28 '20 at 07:56

answered Feb 28 '20 at 07:50

John Wu

50,556
8
44
80

I love your implementation, but it could be even more simplified like this: var orders = await Task.WhenAll(orderIds.Select(orderId => GetOrderDetails(orderId))) .ContinueWith(all => all.Result.Where(o => o != null).ToList()); – Arthur Grigoryan Feb 28 '20 at 09:17
1

@ArthurGrigoryan (1) You missed the point of my first sentence (2) That doesn't compile – John Wu Feb 28 '20 at 16:53
1) I can't see any point in your first sentence. "Enumerate the list more than once" I don't suppose there's a difference for the WhenAll method, or you can restart the tasks after the consumption. So no, I can't see any point there. 2) There's a typo ordersId => instead of orderId => , and it perfectly compiles with no intermediate variables and extra work. – Arthur Grigoryan Feb 29 '20 at 06:09
@ArthurGrigoryan is ContinueWith not wait to complete each task in whenall separately so it seem for each individual task will be synchronous but tasks under whenall will be asynchronous, am I right? also I just read about 1 article on not using ContinueWith, what you will say about that? https://medium.com/@joni2nja/why-you-should-not-use-continuewith-in-your-async-code-c9eaf6087e64 – us075 Mar 03 '20 at 05:30
@JohnWu - could you elaborate on "You wouldn't want to accidentally enumerate the list more than once." - why would that happen in this case? and how does calling toList prevent it. I am a C# newbie I am afraid. – user2294382 Nov 03 '21 at 15:10
Note the existence of [`WhenAll`](https://learn.microsoft.com/en-us/dotnet/api/system.threading.tasks.task.whenall?view=netcore-1.0#system-threading-tasks-task-whenall-1(system-collections-generic-ienumerable((system-threading-tasks-task((-0)))))) which would allow you to just `TResult[] result = await Task.WhenAll...etc`. – JHBonarius Jul 04 '22 at 09:26

score -1 · Answer 2 · answered Feb 28 '20 at 07:37

-1

You should use Microsoft's Reactive Framework (aka Rx) - NuGet System.Reactive and add using System.Reactive.Linq; - then you can do this:

long[] orderIds = { 10, 11, 12, 13 };

IList<Order> orders = await
    orderIds
        .ToObservable()
        .SelectMany(id => Observable.FromAsync(() => GetOrderDetails(id)))
        .ToList();

answered Feb 28 '20 at 07:37

Enigmativity

113,464
11
89
172

I don't think it's Microsoft's. And Reactive X is available for a great variety of languages, not only C#. – Arthur Grigoryan Feb 28 '20 at 08:21
1

@ArthurGrigoryan - It is Microsoft's. They created Rx. The popularity of the library is what created all of the ports to the other languages. https://channel9.msdn.com/Blogs/Charles/Getting-Started-with-Rx-Extensions-for-NET?term=Rx&pubDate=range&lang-en=true&date-range-from=2001-01-01&date-range-to=2010-12-31 – Enigmativity Feb 28 '20 at 09:36
Thanks for sharing. I didn't know that. – Arthur Grigoryan Feb 29 '20 at 06:21

score -3 · Answer 3 · answered Feb 28 '20 at 07:26

-3

I assume lock will do.

long[] orderIds={10,11,12,13};

var orders= new List<Order>();

await Task.WhenAll(orderIds.Select(async (orderId) =>
{
    var orderDetails = await GetOrderDetails(orderId);

    if (orderDetails != null)
        lock(orders) orders.Add(orderDetails);
}));

answered Feb 28 '20 at 07:26

Arthur Grigoryan

358
3
12

Locking on the list itself is usually not a good idea. You probably should have a private (or in this case a local) variable created especially for the purpose of locking. – Enigmativity Feb 28 '20 at 07:36
@Enigmativity Can explain why? I don't think you're right about it. At least Microsoft's documentations don't mention anything about it. – Arthur Grigoryan Feb 28 '20 at 08:03
@Enigmativity I suppose lock works by creating an entry in a special table which contains the address of the reference type object we provide. Until it's not garbage collected or messed up with reflection, it's fine to use whatever reference object you want. – Arthur Grigoryan Feb 28 '20 at 08:10
Regarding locking on a `List`, you may find interesting the comments of this question: [Is it safe to use a boxed value type as a locker for the lock statement?](https://stackoverflow.com/questions/56864042/is-it-safe-to-use-a-boxed-value-type-as-a-locker-for-the-lock-statement). TL:DR it is possible in theory that an instance of a `List` could do `lock(this)`. By using a dedicated private locker `object` you can be sure that your locker will not be used concurrently by anybody else. – Theodor Zoulias Feb 28 '20 at 08:31
@ArthurGrigoryan - If you use a variable that other parts of your code have access to then those parts could lock on the same variable and thus your code can't run. It's a terrible bug to diagnose. It's just always best to use a variable that is as private as possible for your code. – Enigmativity Feb 28 '20 at 08:32
@TheodorZoulias I highly doubt it, because Microsoft itself suggests "In particular, avoid using the following as lock objects: • this, as it might be used by the callers as a lock." Never use 'this' as a lock object. You're wrong too! – Arthur Grigoryan Feb 28 '20 at 08:39
@Enigmativity I couldn't agree with you more if I were using something that is visible outside of my assembly, maybe public property of my class, and not just a local variable as it is, most likely, in this case. – Arthur Grigoryan Feb 28 '20 at 08:48

Task.WhenAll with LINQ select

3 Answers3

I assume lock will do.

Linked