Race condition with .ContinueWith()

Question

I'm running in a race condition rather difficult to reproduce, and from my code analysis it seems to come from a continuation that doesn't execute (or not until its end).

Here is some pseudo-code presenting the context:

Task<Something> GetObject(string id)
{
    // some async code retrieving the object
}

List<Task> tasks = new List<Task>();
List<Something> result = new List<Something>();
foreach (var id in someList)
{
    tasks.Add(GetObject(id).ContinueWith(task => 
    {
        var something = task.Result;
        // do some basic sync stuff on something
        result.Add(something);
    }));
}

await Task.WhenAll(tasks);

It randomly happens that the result is missing some of the expected objects.

For some reason, I know that the foreach went well and there is no reason why all the required tasks would not have been added to the List<Task>.

Instrumenting or debugging hasn't given results so far as it seems to improve the race condition... I hope someone can shed some lights here. Thanks!

`result.Add(something);` isn't thread safe. May be you corrupt the list data structure by modifying it in multiple threads? Do you get any exception? — Sriram Sakthivel, Aug 04 '14 at 06:46
Yeah, exception may/maynot be thrown(depends upon what both threads were doing). But am pretty sure that is one of the problem. Use threadsafe collection or use a `lock` and forget about it. I can't see any other problem in posted code.. — Sriram Sakthivel, Aug 04 '14 at 06:57
Trashing a List<> won't always raise an exception. It might throw IndexOutOfBounds, NullReference and maybe others occasionally. — H H, Aug 04 '14 at 06:57
I'm locking all writes to the list now and will let you know how this turns out. Thanks to all! — ThomasWeiss, Aug 04 '14 at 07:00
@ThomasWeiss Don't forget to add a lock in reading part as well, That's also important! If you're enumerating you will get exception, if you're indexing you may get wrong data.! — Sriram Sakthivel, Aug 04 '14 at 07:02
Thanks for the heads-up, but the result isn't read until all tasks are finished. — ThomasWeiss, Aug 04 '14 at 07:03

score 3 · Accepted Answer · edited May 23 '17 at 12:22

3

For some reason, I know that the foreach went well and there is no reason why all the required tasks would not have been added to the List.

Well, maybe that List is not thread safe.

I don't know from your problem if the list 'someList' is short or long or could have duplicate values, otherwise consider a lock on adding and/or an Immutable list.

Next link has a nice solution: List<T> thread safety

edited May 23 '17 at 12:22

Community

1
1

answered Aug 04 '14 at 06:55

Pieter21

1,765
1
10
22

Answer would actually belong to Sriram Sakthivel who gave the initial comment! :) – ThomasWeiss Aug 04 '14 at 07:19
1

@ThomasWeiss, true ;), but when I would have just said 'List not Thread Safe' I could have been faster. And I would have been downvoted that it is not an answer. And as I just started here, I can use the reputation ;) – Pieter21 Aug 04 '14 at 07:26
Adding to `ImmutableList` itself isn't thread-safe either (because you need to mutate some field). It can be made thread-safe using [`CompareExchange()`](http://msdn.microsoft.com/en-us/library/bb297966), but I think a better option is one of the `Concurrent` collections, like [`ConcurrentBag`](http://msdn.microsoft.com/en-us/library/dd381779). – svick Aug 04 '14 at 12:51

score 3 · Answer 2 · answered Aug 04 '14 at 13:11

List is not thread safe. So if 2 threads (or more!) want to add element to it, everything can happen (per the documentation). Realistically, you will probably have some added elements that go missing.

If you want to continue using List, you will have to make your synchronization yourself (maybe using locks or more Advanced mechanisms).

You can also use another collection type that suits your need. The types from the System.Collections.Concurrent namespace are guaranteed to be thread-safe, but are of course a little more expensive than plain old List.

Last, it appears your example does not need any collection manipulation at all: Task.WhenAll() returns a Task<TResult[]>. You can rewrite your code this way:

Something[] result;
result = await Task.WhenAll(somelist.Select(id => GetObject(id));

That's much shorter, more efficient and hopefully easier to read.

+1 on your LINQ suggestion, it indeed makes sense in the example I gave and I will very probably go that way in this case. — ThomasWeiss, Aug 05 '14 at 01:39

Race condition with .ContinueWith()

2 Answers2