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?

Answer 1

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

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

---

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