Outer Variable Trap

Outer Variable Trap

The "Outer Variable Trap" occurs when a developer expects the value of a variable to be captured by a lambda expression or anonymous delegate, when actually the variable is captured itself.

Example:

var actions = new List<Action>();
for (var i = 0; i < 10; i++)
{
    actions.Add(() => Console.Write("{0} ", i));
}
foreach (var action in actions)
{
    action();
}

Possible output #1:

0 1 2 3 4 5 6 7 8 9

Possible output #2:

10 10 10 10 10 10 10 10 10 10

If you expected output #1, you've fallen into the Outer Variable Trap. You get output #2.

Fix:

Declare an "Inner Variable" to be captured repeatedly instead of the "Outer Variable" which is captured only once.

var actions = new List<Action>();
for (var i = 0; i < 10; i++)
{
    var j = i;
    actions.Add(() => Console.Write("{0} ", j));
}
foreach (var action in actions)
{
    action();
}

For more details, see also Eric Lippert's blog.

Escape outer variable trap in two nested foreach with DictionaryT

If anybody interested, I solved the problem by declaring and initializing periodValues inside the first foreach:

foreach (var numberValue in target.Keys)
{
    var periodValues = new Dictionary<string,int>();
    {
       {"Jan", 0 },
       {"Feb", 0 },
       {"Mar", 0 }
    }
    foreach (var period in target[numberValue].Keys)
    {
        periodValues[period] = target[numberValue][period];
    }
    series[numberValue] = periodValues;
}

How can I capture the value of an outer variable inside a lambda expression?

This has more to do with lambdas than threading. A lambda captures the reference to a variable, not the variable's value. This means that when you try to use i in your code, its value will be whatever was stored in i last.

To avoid this, you should copy the variable's value to a local variable when the lambda starts. The problem is, starting a task has overhead and the first copy may be executed only after the loop finishes. The following code will also fail

for (var i = 0; i < 50; ++i) {
    Task.Factory.StartNew(() => {
        var i1=i;
        Debug.Print("Error: " + i1.ToString());
    });
}

As James Manning noted, you can add a variable local to the loop and copy the loop variable there. This way you are creating 50 different variables to hold the value of the loop variable, but at least you get the expected result. The problem is, you do get a lot of additional allocations.

for (var i = 0; i < 50; ++i) {
    var i1=i;
    Task.Factory.StartNew(() => {
        Debug.Print("Error: " + i1.ToString());
    });
}

The best solution is to pass the loop parameter as a state parameter:

for (var i = 0; i < 50; ++i) {
    Task.Factory.StartNew(o => {
        var i1=(int)o;
        Debug.Print("Error: " + i1.ToString());
    }, i);
}

Using a state parameter results in fewer allocations. Looking at the decompiled code:

• the second snippet will create 50 closures and 50 delegates
• the third snippet will create 50 boxed ints but only a single delegate

Why do these two functions not return the same value?

ff captures (binds) to the value of x at this line:

Func<double> ff = x.AsDelegate();

By contrast, fg binds to the variable x at this line:

Func<double> fg = () => x;

So, when the value of x changes, ff is unafected, but fg changes.

Access to modified closure in razor syntax?

You can safely ignore the warning.

This being said I would replace this foreach loop in your view by a display template:

@model IEnumerable<MyViewModel>
<table>
    <thead>
        <tr>
            <th>Name</th>
            <th>Telephone</th>
            <th>Skypeuser</th>
            <th></th>
        </tr>
    </thead>
    <tbody>
        @Html.DisplayForModel()
    </tbody>
</table>

and then define the corresponding display template that will automatically be rendered for each element of the collection (~/Views.Shared/DisplayTemplates/MyViewModel.cshtml):

@model MyViewModel
<tr>
    <td>
        @Html.DisplayFor(x => x.Name)
    </td>
    <td>
        @Html.DisplayFor(x => x.Telephone)
    </td>
    <td>
        @Html.DisplayFor(x => x.Skypeuser)
    </td>
    <td>
        @Html.ActionLink("Edit", "Edit", new { id = Model.ApplicantID }) |
        @Html.ActionLink("Details", "Details", new { id = Model.ApplicantID }) |
        @Html.ActionLink("Delete", "Delete", new { id = Model.ApplicantID })
    </td>
</tr>

No more warnings.

Declaring a variable inside or outside an foreach loop: which is faster/better?

Performance-wise both examples are compiled to the same IL, so there's no difference.

The second is better, because it more clearly expresses your intent if u is only used inside the loop.

Is there a way to get outer scope variables with the same name?

If foo is on the top level and declared with const, one of the only ways to access it with new Function, whose scope when run is on the top level. (Please don't actually do this):

const foo = 'outer';
function bar() {  const foo = 'inner';  const fn = new Function('return foo');  console.log(fn('foo'));}
bar();
console.log(foo); // "outer"

Integer handled as reference type when passed into a delegate

I argumentend, because int is a value type, the actual value that is passed into Console.WriteLine() gets copied

That is exactly correct. When you call WriteLine the value will be copied.

So, when are you calling WriteLine? It's not in the for loop. You're not writing anything at that point in time, you're just creating a delegate.

It's not until the foreach loop when you invoke the delegate, it's at that time that the value in the variable i is copied to the stack for the call to WriteLine.

So, what's the value of i during the foreach loop? It's 10, for each iteration of the foreach loop.

So now you're asking, "well how is i anything during the foreach loop, isn't it out of scope. Well, no, it's not. What this is demonstrating is a "closure". When an anonymous method reference a variable that variable's scope needs to last for as long as that anonymous method, which could be for any period of time. If nothing special is done at all reading the variable would be random garbage containing whatever happened to be stuck in that location in memory. C# actively makes sure that situation can't happen.

So what does it do? It creates a closure class; it's a class that will contain a number of fields representing everything that is closed over. In other words, the code will be refactored to look something like this:

public class ClosureClass
{
    public int i;

    public void DoStuff()
    {
        Console.WriteLine(i);
    }
}

class Program
{
    delegate void Writer();

    static void Main(string[] args)
    {
        var writers = new List<Writer>();
        ClosureClass closure = new ClosureClass();
        for (closure.i = 0; closure.i < 10; closure.i++)
        {
            writers.Add(closure.DoStuff);
        }

        foreach (Writer writer in writers)
        {
            writer();
        }
    }
}

Now we both have a name for our anonymous method (all anonymous methods are given a name by the compiler) and we can ensure that the variable will live for as long as the delegate that refers to the anonymous function lives.

Looking at this refactor, I hope it's clear why the result is that 10 is printed 10 times.

---

Related Topics