Unexpected behaviour for ThreadPool.QueueUserWorkItem

Question

Please check the code sample below:

public class Sample
{
    public int counter { get; set; }
    public string ID;
    public void RunCount()
    {
        for (int i = 0; i < counter; i++)
        {
            Thread.Sleep(1000);

            Console.WriteLine(this.ID + " : " + i.ToString());
        }
    }
}

class Test
{
    static void Main()
    {
        Sample[] arrSample = new Sample[4];

        for (int i = 0; i < arrSample.Length; i++)
        {
            arrSample[i] = new Sample();
            arrSample[i].ID = "Sample-" + i.ToString();
            arrSample[i].counter = 10;
        }

        foreach (Sample s in arrSample)
        {
            ThreadPool.QueueUserWorkItem(callback => s.RunCount());
        }

        Console.ReadKey();
    }

}

The expected output for this sample should be something like :

Sample-0 : 0 
Sample-1 : 0 
Sample-2 : 0 
Sample-3 : 0 
Sample-0 : 1 
Sample-1 : 1 
Sample-2 : 1 
Sample-3 : 1
.
. 
.

However, when you run this code, it would show something like this instead:

Sample-3 : 0 
Sample-3 : 0 
Sample-3 : 0 
Sample-3 : 1 
Sample-3 : 1 
Sample-3 : 0 
Sample-3 : 2 
Sample-3 : 2
Sample-3 : 1 
Sample-3 : 1
.
. 
.

I can understand that the order in which the threads are executing might differ and hence the count isnt increasing in round robin fashion. However, I fail to understand, why all the IDs are being displayed as Sample-3, while the execution is clearly happening independent of each other.

Arent different objects being used with different threads?

Another day, another person not knowing how to use closures properly. No wonder Java is so reluctant to add them... — leppie, Jan 20 '11 at 07:54
What leppie is saying is that you capture the variable `s` instead of its value. By the time the threads execute, the iteration is done and `s` is left with its last value. — Zarat, Jan 20 '11 at 07:59
@leppie thanks for sarcasm.. i hope you are feeling better by now.. Now, can we have a demonstration of your uberGeekNess with a better explaination ! :) — Danish Khan, Jan 20 '11 at 08:00
@Danish: I was not sarcastic. Not understanding you; closed over variables is IMO the second most common misunderstood area in C# after floating point equality. — leppie, Jan 20 '11 at 08:03
@leppie Well, some ppl tend to feel better by demonstrating their supremacy.. that comment can be taken as one of the examples.. it didnt help me get an answer to my problem or learn anything new.. — Danish Khan, Jan 20 '11 at 08:19
@Danish: You are mistaken. I did in fact answer, but Ani supplied exactly the same answer. So I deleted my answer as his answer was 2 minutes before mine. As for learning something new, the word `closure` should have been enough to keep you busy for a few hours or days, if you made the effort of trying to understand it. — leppie, Jan 20 '11 at 08:26
@Leppie Its alright sir.. lets not get into a verbal tussle, I am sorry for my comments. :) Now, may be you can explain what `floating point equality` issue corresponds to.. ;). Just Kidding!! — Danish Khan, Jan 20 '11 at 08:36

score 11 · Accepted Answer · edited May 23 '17 at 11:44

This is the old modified closure problem. You might want to look at: Threadpools - possible thread execution order problem for a similar question, and Eric Lippert's blog post Closing over the loop variable considered harmful for an understanding of the issue.

Essentially, the lambda expression you've got there is capturing the variable s rather than the value of the variable at the point the lambda is declared. Consequently, subsequent changes made to the value of the variable are visible to the delegate. The instance of Sample on which the RunCount method will run will depend on the instance referred to by the variable s (its value) at the point the delegate actually executes.

Additionally, since the delegate(s) (the compiler actually reuses the same delegate instance) are being asynchronously executed, it isn't guaranteed what these values will be at the point of each execution. What you are currently seeing is that the foreach loop completes on the main-thread before any of the delegate-invocations (to be expected - it takes time to schedule tasks on the thread-pool). So all the work-items end up seing the 'final' value of the loop-variable. But this isn't guaranteed by any means; try inserting a reasonable-duration Thread.Sleep inside the loop, and you will see a different output.

The usual fix is to:

Introduce another variable inside the loop-body.
Assign that variable to the current value of the loop-variable.

Capture the 'copy' variable instead of the loop-variable inside the lambda.

foreach (Sample s in arrSample)
{
    Sample sCopy = s;
    ThreadPool.QueueUserWorkItem(callback => sCopy.RunCount());
}

Now each work-item "owns" a particular value of the loop variable.

Another option in this case is to dodge the issue completely by not capturing anything:

ThreadPool.QueueUserWorkItem(obj => ((Sample)obj).RunCount(), s);

thanks @ani, that makes sense. No wonder I got flamed! I should've known this.. :) — Danish Khan, Jan 20 '11 at 08:04
FWIW, the C# team recognized they made it too easy to make this kind of mistake, and [introduced a breaking change in C# 5 to fix it](https://stackoverflow.com/a/8899347/120955). So this issue won't exist in later versions of C#. — StriplingWarrior, Mar 06 '23 at 23:38

Unexpected behaviour for ThreadPool.QueueUserWorkItem

1 Answers1

Linked