2

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?

Improve this question
2
  • Which version of .NET is this? Commented Apr 13, 2016 at 10:24
  • @LasseV.Karlsen im using .net Framework 4.5 Commented Apr 13, 2016 at 10:25

2 Answers 2

Reset to default
3

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);
Improve this answer
Sign up to request clarification or add additional context in comments.

2 Comments

won't the local variable just use as a reference to the normal kvp?
@OriRefael: Yes, but the captured local variable will not be updated on subsequent iterations of the loop with references to other KeyValuePair instances.
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

Improve this answer

3 Comments

but it will just store a refrence to it because its an object, so it wont do anything..
@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.
it will be unique reference for each task that references to its kvp object

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.