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

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, passin开发者_开发技巧g 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();
}

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".

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.

When doing an anonymous method in your foreach, the compiler is basically generating a class which points to dmsId. So, as threads start, each is pointing at the same variable, so depending on when the threads are scheduled, you will see numbers getting duplicated or skipped.

In the for loop, you are making a copy of the integer so each thread gets its own value.

There is some good data on this problem here.

The problem is that closures close over variables, not values. This means that all delegates are getting a reference to the same variable, and the variable's value changes each time through the loop

This should fix it:

//lstDMSID is a populated List with 10 elements.
foreach(int dmsId in lstDMSID) 
{
    int tempId = dmsId;
    ThreadStart ts = delegate
    {
        //this is method that goes off ad does some isolated work with the integer
        DoThreadWork(tempId);
    };
    Thread thr = new Thread(ts);
    thr.Name = tempId.ToString();
    thr.Start();
}

In the first case, dmsId is declared within the scope of the for loop, each delegate captures its own "instance" of that variable.

In the second version, dmsId is declared for the entire scope of the foreach loop. Each delegate captures the same variable - which means you're accessing the same variable from several threads with no locking - bad stuff can happen.