Why would an iterator (.Net) be unreliable in this code

Question

I have an example that I can get to break every time I use an iterator, but it works fine with a for loop. All code uses variables local to the executing method. I am stumped. There is either a fact about iterators that I am not aware of, or there is an honest to goodness bug in .Net. I'm betting on the former. Pleae help.

This code reliably work every time. It loops through (let's say 10) all elements one at a time and starts a new thread, passing the integer to the new thread as an argument in a method. It starts 10 threads, one for each item. 1,2,3,4,5,6,7,8,9,10 - this always works.

WORKING CODE:

//lstDMSID is a populated List<int> with 10 elements.
for(int i=0; i<lstDMSID.Count; i++) 
{
    int dmsId = lstDMSID[i];
    ThreadStart ts = delegate
    {
        // Perform some isolated work with the integer
        DoThreadWork(dmsId);  
    };
    Thread thr = new Thread(ts);
    thr.Name = dmsId.ToString();
    thr.Start();
 }

And this code actually repeat elements. It iterates through (let's say 10) all elements one at a time and starts a new thread. It starts 10 threads, but it does not reliably get all 10 integers. I am seeing it start 1,2,3,3,6,7,7,8,9,10. I am losing numbers.

BUSTED CODE:

//lstDMSID is a populated List<int> with 10 elements.
foreach(int dmsId in lstDMSID) 
{
    ThreadStart ts = delegate
    {
        // Perform some isolated work with the integer
         DoThreadWork(dmsId);
    };
    Thread thr = new Thread(ts);
    thr.Name = dmsId.ToString();
    thr.Start();
}

Answer 1

The problem is based on the closure generated for your scope...

The same problem would happen in your for loop, were you to rewrite it like so (BAD CODE!):

// ...Closure now happens at this scope...
for(int i=0;i<lstDMSID.Count;i++) 
{
    ThreadStart ts = delegate
    {
        DoThreadWork(lstDMSID[i]); // Eliminate the temporary, and it breaks!
    };
    Thread thr = new Thread(ts);
    thr.Name = dmsId.ToString();
    thr.Start();
}

The issue is, when you close over a variable in a delegate (in your case, dmsId), the closure happens at the scope where the variable is being declared. When you use a for or a foreach loop, the closure happens in the scope of the for/foreach statement, which is one level too high.

Introducing a temporary variable inside the foreach loop will correct the issue:

foreach(int dmsId in lstDMSID) 
{
    int temp = dmsId; // Add temporary
    ThreadStart ts = delegate
    {
        DoThreadWork(temp); // close over temporary, and it's fixed
     };
     Thread thr = new Thread(ts);
     thr.Name = dmsId.ToString();
     thr.Start();
}

For a more detailed discussion of what is happening, I'd recommend reading Eric Lippert's blog post: "Closing over the loop variable considered harmful".

Answer 2

This is because of the scope of the variable you're using in the closure.

Eric Lippert has a nice blog post explaining this in detail, and I think that others (Jon Skeet?) have blogged about it, too.