Is this the correct way to call Dispatcher.BeginInvoke/lambda in a loop over an array or list of data items

Question

I inherited some code which used BeginInkoke to add tabs to a TabControl which looked like this:

foreach (DitaNestedContent content in root.Content)
{
    CrlList.Dispatcher.BeginInvoke(DispatcherPriority.Normal, new Action<TabControl>((tabControl) =>
    {
        TabItem aTab = new TabItem();
        if (content.Paths != null)
        {
            PublicationsListUserControl crlTree = new PublicationsListUserControl(content.Path, filename);
            crlTree.MinWidth = 5;
            aTab.Content = crlTree;
        }

        aTab.Header = content.Name;
        tabControl.Items.Add(aTab);
}), CrlList);

}

This worked until I rebuilt the project after which the correct number of tabs were still created but each one contained the last tab's content (only). I reasoned that timings had changed and the previous code was working merely by accident and that the first BeginInvoke was now only being started after the loop had completed and that the content was therefore equal to the last value by the time it ran.

So I decided to rewrite the code but I was astonished at what finally seemed to work:

List<String> contentPaths = new List<string>();
foreach (DitaNestedContent content in root.Content)
{
    contentPaths.Add(String.Copy(content.Path));
}

for (Int32 i = 0; i < root.Content.Count; ++i)
{
    CrlList.Dispatcher.BeginInvoke(DispatcherPriority.Normal, new Action<TabControl>((tabControl) =>
    {
        if (i >= root.Content.Count) { i = 0; } 

        TabItem aTab = new TabItem();
        if (contentPaths[i] != null)
        {
            String contentPath = contentPaths[i];
            PublicationsListUserControl crlTree = new PublicationsListUserControl(contentPath, filename);
            crlTree.MinWidth = 5;
            aTab.Content = crlTree;
        }

        aTab.Header = root.Content[i].Name;
        tabControl.Items.Add(aTab);

        ++i;

    }), CrlList);
}

Basically instead of using the current content to invoke the PublicationsListUserControl constructor, I use i inside the lambda to recalculate which root.Content I should be using.

I would have thought (and the guy who wrote the code before me obviously thought) that the values of the variables used would have been calculated and stored for use by the lambda when it was created, not when BeginInvoke began working.

Is this the correct to reliably use BeginInvoke with a lambda in a loop? Or am I way off the mark?

---

UPDATE

foreach variables can only be captured by C# 5.0 see here:

http://ericlippert.com/2009/11/12/closing-over-the-loop-variable-considered-harmful-part-one/ http://csharp.2000things.com/2014/09/19/1186-capturing-a-foreach-iteration-variable-in-a-lambda-expression/

Answer 1

What may be happening here is that you've moved down a compiler version from the codes author, prior to C#5 the variable defined in the foreach body (Your content variable) was defined outside of the loop and took the last applicable value, after C#5 that variable is defined inside the loop and is captured appropriately in lambdas.

http://blogs.msdn.com/b/ericlippert/archive/2009/11/12/closing-over-the-loop-variable-considered-harmful.aspx

Answer 2

Is there something about the enumeration of root.Content that is slow? I.e. each new value retrieved takes some time? If not, then there's really no good reason to perform a new invoke for each item in the enumeration; the code really should just put the loop in the invoked method. I.e.:

CrlList.Dispatcher.BeginInvoke(DispatcherPriority.Normal,
    (TabControl tabControl) =>
    {
        foreach (DitaNestedContent content in root.Content)
        {
            TabItem aTab = new TabItem();
            if (content.Paths != null)
            {
                PublicationsListUserControl crlTree =
                    new PublicationsListUserControl(content.Path, filename);
                crlTree.MinWidth = 5;
                aTab.Content = crlTree;
            }

            aTab.Header = content.Name;
            tabControl.Items.Add(aTab);
        }
    }), CrlList);

If there really is a good reason to perform the foreach outside of the invoke, then you need to look harder to determine exactly why the original code wasn't behaving the way you wanted. Your original theory was incorrect, and the new code is more broken than whatever was wrong with the original code.

A foreach loop variable is essentially created anew with each loop iteration, so it's safe to capture the variable itself, while a for loop variable is created once for the entire loop and so is not safe to capture. In other words, your theory that the content variable was being shared by each invoked method instance is wrong; they each get their own private copy of the variable and so it should not matter when the invoked method is actually executed.

Your attempt to work around the single for loop variable, by capturing that variable and the incrementing it in the invoked anonymous method certainly can work in some situations. But you have both a race and a thread-safety issue, in that the loop calling the BeginInvoke() method is modifying that same variable that is also being use in the UI thread where the invoked method executes. If one or more of the invocations gets to execute before the loop is complete, then the loop itself will increment the variable as well as the invoked method, causing you to bypass some indexes, repeat others, and worst of all, possibly have the index change right in the middle of processing a single element.

The normal way to address capturing in a for loop is to have a block-local variable into which the loop variable is copied, and use that variable instead of the loop variable. E.g.:

for (int i = 0; i < max; i++)
{
    int localIndex = i;

    Dispatcher.Current.BeginInvoke(() => /* do something with localIndex, not i */);
}

But you really should not need to use a for loop. As I said, the foreach is specifically the safe scenario when it comes to variable capturing, and so whatever the wrong behavior you were seeing, it's not a capturing bug.

(Or, if it is, you posted some code other than the code actually executing. Consider posting a good code example instead, which is complete, concise, and reliably reproduces the problem you're having).