The foreach identifier and closures

Question

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.

Factorial

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). – Thermolysis

possible duplicate of Access to Modified Closure (2) – Myrtismyrtle

@Thermolysis It doesn’t require threading. Deferring the execution of the delegates until after the loop would be enough to get this behavior. – Daubigny

Answer 1

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)

Renunciation

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. – Tetroxide

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.) – Submerged

@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. – Renunciation

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) – Polyhistor

@Polyhistor aye, I've been correcting these as we go, but it was a common stumbling block, so : there's quite a few to go! – Renunciation

Answer 2

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();
        }
    }
}

Fontanel

Thanks - I appended the question to say it's not really about threads. – Factorial

It was also in one of the talks that you have in video on your site csharpindepth.com/Talks.aspx – Harp

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. – Fontanel

Nice to know the videos are getting watched though :) – Fontanel

Even understanding that the variable exists outside the for loop this behavior is confusing to me. For instance in your example of closure behavior, https://mcmap.net/q/20114/-what-are-39-closures-39-in-net, the variable exists outside the closure yet binds correctly. Why is this different? – Concretion

@JamesMcMahon: It isn't different. The single variable is being captured in that case too. Basically, you need to work out which variable is being captured, and where else it is used. – Fontanel

Oh I get you. The writeline is displaying the value at the time, but the next time the variable is modified it would be modified in the closure. The behavior is just confusing to me because I expect closures to be the value that they were created with. – Concretion

@JamesMcMahon: And that's the point. It's the variable that is captured by the closure, not the value at the point of creation. – Fontanel

Answer 3

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.

Sixtasixteen

I think that goes for value and reference types. – Benedictine

Answer 4

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.

Etti

Answer 5

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();
}

Tillage

Answer 6

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?

Allx

Answer 7

Foo f2 = f;

points to the same reference as

f 

So nothing lost and nothing gained ...

Afterpiece

The point is that the compiler performs some magic here to lift the local variable into the scope of the implicitly created closure. The question is if the compiler understands to do this for the loop variable. The compiler is known for a few weaknesses regarding lifting so this is a good question. – Carbaugh

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). – Benedictine

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. – Carbaugh

leppie: (cont'd) and since others have pointed out that code 1 is actually not enough, this example illustrates perfectly why the name "magic" is well-earned. Intuitively, the behaviour should be the same. Why isn't it? Magic. – Carbaugh

It's not the code, it's the semantics, and they are clearly defined. – Benedictine

@konrad: my original point makes that clear now (variable is mutated). All you to imagine is that everything is wrapped in a class, and so they say goes, closures are the poor man's classes or was that classes are the poor man's closures? ;) – Benedictine

sorry for all the typos :( 1. have to imagine 2. so the saying goes. – Benedictine

@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? – Fontanel

@Jon Skeet Do you mean "any sufficiently advanced technology is indistinguishable from magic" en.wikipedia.org/wiki/Clarke%27s_three_laws :) – Cornflakes

It does not point to a reference. It is a reference. It points to the same object, but it is a different reference. – Welbie