Thread Safety and Task.Factory

Question

I'm quite new in parallel programing
i'd like to do some work with tasks
every task is initiated with param to do some simple work with id
but it seems the params all mixed up..

i'm sure i'm missing some key element in thread safety
can you help me understand what i'm doing wrong ?

i don't need any return value from the tasks, i just need them to finish their work.

static void Main(string[] args)
{
    int NumberOfTasks = 10;

    Task[] tasks = new Task[NumberOfTasks];
    for (int i = 0; i < NumberOfTasks; i++)
    {
       tasks[i] = Task.Factory.StartNew(() => DoSafeWork(i));
    }
    Task.WaitAll(tasks);

    Console.WriteLine("Done !");
    Console.ReadKey();
}

private static void DoSafeWork(int i)
{
    Console.WriteLine("working on Task {0} ", i.ToString());
}

Current output (possible):

working on Task 3    
working on Task 6    
working on Task 10    
working on Task 10    
working on Task 10    
working on Task 10    
working on Task 10    
working on Task 10    
working on Task 10    
working on Task 10    
Done !

possible duplicate of [C# - The foreach identifier and closures](http://stackoverflow.com/questions/512166/c-sharp-the-foreach-identifier-and-closures) — Euphoric, Dec 17 '14 at 12:57
@Euphoric It's not, since the behavior differs between `foreach` and `for` — i3arnon, Dec 17 '14 at 18:51
@l3arnon Yeah, it was fixed in newer C# version. But the problem is still same. — Euphoric, Dec 17 '14 at 18:52

score 2 · Accepted Answer · edited May 23 '17 at 11:49

2

The problem lies in the fact that the loop variable is defined outside the loop. Note that the source of the problem is actually the closure and has nothing to do with threading.

Just create a local copy of your loop variable like this, so that the closure captures the copy:

for (int i = 0; i < NumberOfTasks; i++)
{
    var localCopy = i;
    tasks[i] = Task.Factory.StartNew(() => DoSafeWork(localCopy));
}

See https://stackoverflow.com/a/512265/219187 for a detailed description.

edited May 23 '17 at 11:49

Community

1
1

answered Dec 17 '14 at 12:59

theDmi

17,546
6
71
138

that shouldn't be a problem while using value types like int32 – fubo Dec 17 '14 at 13:08
1

@fubo That's a common assumption, but this is not true. See http://stackoverflow.com/a/5438331/219187 for an in-depth explanation from Mr. Skeet himself. – theDmi Dec 17 '14 at 13:16

score 0 · Answer 2 · answered Dec 17 '14 at 18:58

The problem, as theDmi said is that you are closing over the index and so all the tasks reference the same index while the for loop changes it.

While you could solve that by copying the current index value to a local variable a more elegant solution would be to use LINQ:

var tasks = Enumerable.Range(0, NumberOfTasks).Select(i => Task.Factory.StartNew(() => DoSafeWork(i)));
Task.WaitAll(tasks);

Also, if you're not constrained to an older version of .Net you should be using Task.Run, Task.WhenAll and await instead:

var tasks = Enumerable.Range(0, NumberOfTasks).Select(i => Task.Run(() => DoSafeWork(i)));
await Task.WhenAll(tasks);

Thread Safety and Task.Factory

2 Answers2