C# thread safe closure inside foreach

Question

I wrote a code block, but I am not sure it is thread safe.

List<Task> tasks = new List<Task>();
foreach (KeyValuePair<string, string> kvp in result)
{
    var t = new Task(async () => 
    {
        int retries = 0;
        bool success = false;
        try
        {
            while (retries <= _maxRetries && !success)
            {
                await doSomething(kvp.Value);
                success = true;
            }
        }
        catch (Exception e)
        {
            retries++;
        }

        if (retries == _maxRetries)
        {
            //TODO: need to do smth about it
        }
    });
    tasks.Add(t);
    t.Start();
}
await Task.WhenAll(tasks);

Can I rely on the fact that when the compiler sets the task he uses a safe value, meaning that as long as Im in the loop and the task haven't stated yet, the values set are ok?

Because, I think that after the first retry of the while loop, the kvp object won't be as he was when the task ran at first time.

If its in-fact not thread-safe, which I think it really isn't, how can it be fixed?

Which version of .NET is this? – Lasse V. Karlsen Apr 13 '16 at 10:24 — Lasse V. Karlsen, Apr 13 '16 at 10:24
@LasseV.Karlsen im using .net Framework 4.5 – Ori Refael Apr 13 '16 at 10:25 — Ori Refael, Apr 13 '16 at 10:25

Douglas · Accepted Answer · 2016-04-13T10:32:32.403

If you're on C# 5, then your code is fine; the semantics of foreach have been changed such that the loop variable is logically scoped to each iteration. Per Eric Lippert:

In C# 5, the loop variable of a foreach will be logically inside the loop, and therefore closures will close over a fresh copy of the variable each time.

If you're on C# 4 or earlier, then you should copy kvp to a local variable for the closure.

Your code has an unrelated bug: A task initialized through the Task constructor will not wait for the completion of the asynchronous function delegate that is passed to it as argument. Instead, you should use Task.Run for such delegates.

There is a simpler (safe) way of achieving this:

var tasks = result.Select(kvp => Task.Run(async () =>
{
    int retries = 0;
    bool success = false;
    // ...
})).ToList();

await Task.WhenAll(tasks);

won't the local variable just use as a reference to the normal kvp? — Ori Refael, Apr 13 '16 at 10:26
@OriRefael: Yes, but the captured local variable will not be updated on subsequent iterations of the loop with references to *other* `KeyValuePair` instances. — Douglas, Apr 13 '16 at 10:27

score 1 · Answer 2 · answered Apr 13 '16 at 10:23

1

I guess you have to save kvp in local variable inside your task like this:

 var t = new Task(async () => 
    {
        var localKvp = kvp;
        ....
    }

This way it will catch local variable for each task

answered Apr 13 '16 at 10:23

Mitklantekutli

410
1
3
16

but it will just store a refrence to it because its an object, so it wont do anything.. – Ori Refael Apr 13 '16 at 10:24
@OriRefael Wrong, it will create a new *variable* which will be captured by the delegate instead of the loop variable. This new variable will be treated as "new" each time the loop iterates, so in effect you will have one such variable per iteration, whereas the loop variable will only exist once, for all the iterations. This was changed in latter C# versions. – Lasse V. Karlsen Apr 13 '16 at 10:26
it will be unique reference for each task that references to its kvp object – Mitklantekutli Apr 13 '16 at 10:26

C# thread safe closure inside foreach

2 Answers2