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

possible duplicate of [How does a lambda in C# bind to the enumerator in a foreach?](http://stackoverflow.com/questions/2413706/how-does-a-lambda-in-c-bind-to-the-enumerator-in-a-foreach) — JSBձոգչ, Apr 19 '11 at 13:36
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. — mqp, Apr 19 '11 at 13:47

score 14 · Accepted Answer · answered Apr 19 '11 at 13:34

14

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

answered Apr 19 '11 at 13:34

Heinzi

167,459
57
363
519

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. – redcalx Nov 05 '11 at 15:48

Jordão · Answer 2 · 2011-04-19T13:55:42.153

4

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

edited Apr 19 '11 at 13:55

answered Apr 19 '11 at 13:41

Jordão

55,340
13
112
144

@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. – Heinzi Oct 31 '12 at 21:42

score 1 · Answer 3 · answered Apr 19 '11 at 13:36

1

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

answered Apr 19 '11 at 13:36

Jader Dias

88,211
155
421
625

1

Yes, you can also use a `Tuple` instead of the KeyValuePair. – H H Apr 19 '11 at 13:38
@Henk which requires .NET 4.0 – Jader Dias Apr 19 '11 at 13:43
1

which I now consider the default: unless stated otherwise. – H H Apr 19 '11 at 13:47

score 1 · Answer 4 · answered Apr 19 '11 at 13:39

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

How to avoid captured variables?

4 Answers4

Linked