81

In the two following snippets, is the first one safe or must you do the second one?

By safe I mean is each thread guaranteed to call the method on the Foo from the same loop iteration in which the thread was created?

Or must you copy the reference to a new variable "local" to each iteration of the loop?

var threads = new List<Thread>(); foreach (Foo f in ListOfFoo) { Thread thread = new Thread(() => f.DoSomething()); threads.Add(thread); thread.Start(); }

-

var threads = new List<Thread>(); foreach (Foo f in ListOfFoo) { Foo f2 = f; Thread thread = new Thread(() => f2.DoSomething()); threads.Add(thread); thread.Start(); }

Update: As pointed out in Jon Skeet's answer, this doesn't have anything specifically to do with threading.

Improve this question
3
  • Actually I feel it has to do with threading as if you weren't using threading, you would call the right delegate. In Jon Skeet's sample without threading, the problem is that there are 2 loops. Here's there's only one, so there should be no issue...unless you don't know exactly when the code will be executed (meaning if you use threading - Marc Gravell's answer shows that perfectly). Commented Dec 21, 2011 at 9:28
  • possible duplicate of Access to Modified Closure (2) Commented Nov 2, 2013 at 7:07
  • @user276648 It doesn’t require threading. Deferring the execution of the delegates until after the loop would be enough to get this behavior. Commented Jan 20, 2016 at 16:04

7 Answers 7

Reset to default
105

Edit: this all changes in C# 5, with a change to where the variable is defined (in the eyes of the compiler). From C# 5 onwards, they are the same.

Before C#5

The second is safe; the first isn't.

With foreach, the variable is declared outside the loop - i.e.

Foo f; while(iterator.MoveNext()) { f = iterator.Current; // do something with f }

This means that there is only 1 f in terms of the closure scope, and the threads might very likely get confused - calling the method multiple times on some instances and not at all on others. You can fix this with a second variable declaration inside the loop:

foreach(Foo f in ...) { Foo tmp = f; // do something with tmp }

This then has a separate tmp in each closure scope, so there is no risk of this issue.

Here's a simple proof of the problem:

 static void Main() { int[] data = { 0, 1, 2, 3, 4, 5, 6, 7, 8, 9 }; foreach (int i in data) { new Thread(() => Console.WriteLine(i)).Start(); } Console.ReadLine(); }

Outputs (at random):

1 3 4 4 5 7 7 8 9 9

Add a temp variable and it works:

 foreach (int i in data) { int j = i; new Thread(() => Console.WriteLine(j)).Start(); }

(each number once, but of course the order isn't guaranteed)

Improve this answer
Sign up to request clarification or add additional context in comments.

5 Comments

Holy cow ... that old post saved me a lot of headache. I always expected the foreach variable to be scoped inside the loop. That was one major WTF experience.
Actually that was considered a bug in foreach-loop and fixed in the compiler. (Unlike for-loop where the variable has single instance for the entire loop.)
@Orlangur I've had direct conversations with Eric, Mads and Anders over this for years. The compiler followed the spec so was right. The spec made a choice. Simply: that choice was changed.
This answer applies up to C# 4, but not for later versions: "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." (Eric Lippert)
@Douglas aye, I've been correcting these as we go, but it was a common stumbling block, so : there's quite a few to go!
37

Pop Catalin and Marc Gravell's answers are correct. All I want to add is a link to my article about closures (which talks about both Java and C#). Just thought it might add a bit of value.

EDIT: I think it's worth giving an example which doesn't have the unpredictability of threading. Here's a short but complete program showing both approaches. The "bad action" list prints out 10 ten times; the "good action" list counts from 0 to 9.

using System; using System.Collections.Generic; class Test { static void Main() { List<Action> badActions = new List<Action>(); List<Action> goodActions = new List<Action>(); for (int i=0; i < 10; i++) { int copy = i; badActions.Add(() => Console.WriteLine(i)); goodActions.Add(() => Console.WriteLine(copy)); } Console.WriteLine("Bad actions:"); foreach (Action action in badActions) { action(); } Console.WriteLine("Good actions:"); foreach (Action action in goodActions) { action(); } } }
Improve this answer

8 Comments

Thanks - I appended the question to say it's not really about threads.
It was also in one of the talks that you have in video on your site csharpindepth.com/Talks.aspx
Yes, I seem to remember I used a threading version there, and one of the feedback suggestions was to avoid threads - it's clearer to use an example like the one above.
Nice to know the videos are getting watched though :)
Even understanding that the variable exists outside the for loop this behavior is confusing to me. For instance in your example of closure behavior, stackoverflow.com/a/428624/20774, the variable exists outside the closure yet binds correctly. Why is this different?
|
17

Your need to use option 2, creating a closure around a changing variable will use the value of the variable when the variable is used and not at closure creation time.

The implementation of anonymous methods in C# and its consequences (part 1)

The implementation of anonymous methods in C# and its consequences (part 2)

The implementation of anonymous methods in C# and its consequences (part 3)

Edit: to make it clear, in C# closures are "lexical closures" meaning they don't capture a variable's value but the variable itself. That means that when creating a closure to a changing variable the closure is actually a reference to the variable not a copy of it's value.

Edit2: added links to all blog posts if anyone is interested in reading about compiler internals.

Improve this answer

1 Comment

I think that goes for value and reference types.
3

This is an interesting question and it seems like we have seen people answer in all various ways. I was under the impression that the second way would be the only safe way. I whipped a real quick proof:

class Foo { private int _id; public Foo(int id) { _id = id; } public void DoSomething() { Console.WriteLine(string.Format("Thread: {0} Id: {1}", Thread.CurrentThread.ManagedThreadId, this._id)); } } class Program { static void Main(string[] args) { var ListOfFoo = new List<Foo>(); ListOfFoo.Add(new Foo(1)); ListOfFoo.Add(new Foo(2)); ListOfFoo.Add(new Foo(3)); ListOfFoo.Add(new Foo(4)); var threads = new List<Thread>(); foreach (Foo f in ListOfFoo) { Thread thread = new Thread(() => f.DoSomething()); threads.Add(thread); thread.Start(); } } }

if you run this you will see option 1 is definetly not safe.

Improve this answer

Comments

1

In your case, you can avoid the problem without using the copying trick by mapping your ListOfFoo to a sequence of threads:

var threads = ListOfFoo.Select(foo => new Thread(() => foo.DoSomething())); foreach (var t in threads) { t.Start(); }
Improve this answer

Comments

0

Both are safe as of C# version 5 (.NET framework 4.5). See this question for details: Has foreach's use of variables been changed in C# 5?

Improve this answer

Comments

-5 
Foo f2 = f;

points to the same reference as 

f

So nothing lost and nothing gained ...

Improve this answer

10 Comments

It's not magic. It simply captures the environment. The problem here and with for loops, is that the capture variable gets mutated (re-assigned).
leppie: the compiler generates code for you, and it's not easy to see in general what code this is exactly. This is the definition of compiler magic if ever there was one.
@leppie: I'm with Konrad here. The lengths the compiler goes to feel like magic, and although the semantics are clearly defined they're not well understood. What's the old saying about anything not well understood being comparable to magic?
@Jon Skeet Do you mean "any sufficiently advanced technology is indistinguishable from magic" en.wikipedia.org/wiki/Clarke%27s_three_laws :)
It does not point to a reference. It is a reference. It points to the same object, but it is a different reference.
|

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.