How to avoid captured variables?

Question

I'm having a problem with

foreach(var category in categories)
{
    foreach(var word in words)
    {
        var waitCallback = new WaitCallback(state =>
        {
            DoSomething(word, category);
        });

        ThreadPool.QueueUserWorkItem(waitCallback);
    }
}

When the DoSomething gets executed, it receives the latest value for each captured variable instead of the value I desired. I can imagine a solution for this, but it imagine you guys can come up with better solutions

Parliament

possible duplicate of How does a lambda in C# bind to the enumerator in a foreach? – Morrismorrison

I don't agree that it's a duplicate. That post asks why the behavior happens; this post asks what the best way to write this code to avoid the problem is. – Mckeehan

Answer 1

The canonical way to solve this is to copy the values into temporary variables which are declared inside the loop.

foreach(var category in categories)
{
    var catCopy = category;
    foreach(var word in words)
    {
        var wordCopy = word;
        var waitCallback = new WaitCallback(state =>
        {
            DoSomething(wordCopy, catCopy);
        });

        ThreadPool.QueueUserWorkItem(waitCallback);
    }
}

Modulate

Excellent thanks. Have been scratching my head on this one for about an hour. So each loop instantiates a new local variable, each of which is captured independently of the others - got it. – Eurasian

Answer 2

Refactor it to:

foreach(var category in categories) {
  foreach(var word in words) {
    DoSomethingAsync(word, category);
  }
}

...

private void DoSomethingAsync(string word, string category) {
  var waitCallback = new WaitCallback(state => DoSomething(word, category));
  ThreadPool.QueueUserWorkItem(waitCallback);
}

This is simple and easy to understand. It states the developer's intent without cluttering the code with extra variables (as in the default way to solve this problem).

Bresee

@mquander: Might be a matter of taste, but it's basically the same principle. In the case of the function, the copy is made implicitly through the function parameters. – Modulate

Answer 3

For reference, I imagine the following would solve my problem:

foreach(var category in categories)
{
    foreach(var word in words)
    {
        var waitCallback = new WaitCallback(state =>
        {
            var kv = (KeyValuePair<string, string>)state;
            DoSomething(kv.Key, kv.Value);
        });

        var state2 = new KeyValuePair<string, string>(word, category);
        ThreadPool.QueueUserWorkItem(waitCallback, state2);
    }
}

Parliament

Yes, you can also use a Tuple<string, string> instead of the KeyValuePair. – Felipe

@Henk which requires .NET 4.0 – Parliament

which I now consider the default: unless stated otherwise. – Felipe

Answer 4

I would write the whole thing like this, which dodges the problem and leaves absolutely no question about what is going on:

var callbacks = words.SelectMany(w => categories.Select(c =>
    new WaitCallback(state => {
        DoSomething(w, c);
    })
));

foreach (var callback in callbacks)
    ThreadPool.QueueUserWorkItem(callback);

Mckeehan