Access to Modified Closure (2)

Question

This is an extension of question from Access to Modified Closure. I just want to verify if the following is actually safe enough for production use.

List<string> lists = new List<string>();
//Code to retrieve lists from DB    
foreach (string list in lists)
{
    Button btn = new Button();
    btn.Click += new EventHandler(delegate { MessageBox.Show(list); });
}

I only run through the above once per startup. For now it seems to work alright. As Jon has mentioned about counterintuitive result in some case. So what do I need to watch out here? Will it be ok if the list is run through more than once?

Lapidary

Congratulations, you're now part of the Resharper documentation. confluence.jetbrains.net/display/ReSharper/… – Jarlathus

This one was tricky but the above explanation made it clear for me: This may appear to be correct but, in actual fact, only the last value of str variable will be used whenever any button is clicked. The reason for this is that foreach unrolls into a while loop, but the iteration variable is defined outside this loop. This means that by the time you show the message box, the value of str may have already been iterated to the last value in the strings collection. – Sympathizer

Answer 1

Prior to C# 5, you need to re-declare a variable inside the foreach - otherwise it is shared, and all your handlers will use the last string:

foreach (string list in lists)
{
    string tmp = list;
    Button btn = new Button();
    btn.Click += new EventHandler(delegate { MessageBox.Show(tmp); });
}

Significantly, note that from C# 5 onwards, this has changed, and specifically in the case of foreach, you do not need to do this any more: the code in the question would work as expected.

To show this not working without this change, consider the following:

string[] names = { "Fred", "Barney", "Betty", "Wilma" };
using (Form form = new Form())
{
    foreach (string name in names)
    {
        Button btn = new Button();
        btn.Text = name;
        btn.Click += delegate
        {
            MessageBox.Show(form, name);
        };
        btn.Dock = DockStyle.Top;
        form.Controls.Add(btn);
    }
    Application.Run(form);
}

Run the above prior to C# 5, and although each button shows a different name, clicking the buttons shows "Wilma" four times.

This is because the language spec (ECMA 334 v4, 15.8.4) (before C# 5) defines:

foreach (V v in x) embedded-statement is then expanded to:

{
    E e = ((C)(x)).GetEnumerator();
    try {
        V v;
         while (e.MoveNext()) {
            v = (V)(T)e.Current;
             embedded-statement
        }
    }
    finally {
        … // Dispose e
    }
}

Note that the variable v (which is your list) is declared outside of the loop. So by the rules of captured variables, all iterations of the list will share the captured variable holder.

From C# 5 onwards, this is changed: the iteration variable (v) is scoped inside the loop. I don't have a specification reference, but it basically becomes:

{
    E e = ((C)(x)).GetEnumerator();
    try {
        while (e.MoveNext()) {
            V v = (V)(T)e.Current;
            embedded-statement
        }
    }
    finally {
        … // Dispose e
    }
}

---

Re unsubscribing; if you actively want to unsubscribe an anonymous handler, the trick is to capture the handler itself:

EventHandler foo = delegate {...code...};
obj.SomeEvent += foo;
...
obj.SomeEvent -= foo;

Likewise, if you want a once-only event-handler (such as Load etc):

EventHandler bar = null; // necessary for "definite assignment"
bar = delegate {
  // ... code
  obj.SomeEvent -= bar;
};
obj.SomeEvent += bar;

This is now self-unsubscribing ;-p

Conscientious

If that's the case, the temporary variable will stay in memory until the app closes, as to serve the delegate, and it won't be advisable to do that for very huge loops if the variable takes up a lot of memory. Am I right? – Lapidary

It will stay in memory for the length of time that there are things (buttons) with the event. There is a way of un-subscribing once-only delegates, which I'll add to the post. – Conscientious

But to qualify on your point: yes, captured variables can indeed increase the scope of a variable. You need to be careful not to capture things you weren't expecting... – Conscientious

Could you please update your answer with respect to change in C# 5.0 spec? Just to make it a great wiki documentation regarding foreach loops in C#. There are already some good answers regarding change in C# 5.0 compiler treating foreach loops bit.ly/WzBV3L, but they aren't a wiki-like resources. – Cecrops

But I assume the for loop keeps its variable scoping in 5.0 and still needs the local variable workaround, is that correct? – Strychnic

@Strychnic yes, for is unchanged in 5.0 – Conscientious