BeginInvoke causes error due to skipped null check

Question

I encountered a to me not understandable error caused by using Dispatcher.BeginInvoke within a multi-threaded application.

My program contains a List of objects through which I loop using multiple threads and perform some calculations. I have simplified (and slightly modified) my code structure to the very bare essentials, so it will hopefully be easier to understand:

public class Foo
{
    public void DoCalc()
    {
        //do calculations
    }
}

public class Foo2
{
    public Foo foo = new Foo();
    public Ellipse ellipse;
}

public partial class MainWindow : Window
{
    List<Foo2> myList = new List<Foo2>();
    List<Tuple<int, int>> indicesList = new List<Tuple<int, int>>();

    public MainWindow()
    {
        InitializeComponent();

        for (int i = 0; i < 10; ++i)
            myList.Add(new Foo2 { ellipse = new Ellipse() });
        for (int i = 10; i < 20; ++i)
            myList.Add(new Foo2());
        indicesList.Add(new Tuple<int, int>(0, 9));
        indicesList.Add(new Tuple<int, int>(10, 19));
    }

    private void OnStart(object sender, RoutedEventArgs e)
    {
        foreach (var t in indicesList)
            ThreadPool.QueueUserWorkItem(new WaitCallback(Loop), t);
    }

    private void Loop(object o)
    {
        Tuple<int, int> indices = o as Tuple<int, int>;

        for(int i = indices.Item1; i <= indices.Item2; ++i)
        {
            myList[i].foo.DoCalc();
            if (myList[i].ellipse == null)
                continue;

            Application.Current.Dispatcher.BeginInvoke(DispatcherPriority.Normal, new Action(() => myList[i].ellipse.Fill = Brushes.Black));
        }
    }
}

The first half of items in myList has ellipse point to actual objects, while the second half points to null. Within Loop I check at every iteration if ellipse is null and skip that iteration if necessary. What is weird is that in the second thread where all ellipses point to null, the program still ends up calling the BeginInvoke action on the first item (index 10), causing the program to crash, due to a null reference exception.

If I put a breakpoint on the line myList[i].foo.DoCalc(); and go through the program slowly step by step, no error occurs. Also when I change BeginInvoke to Invoke no error occurs.

As I understand, BeginInvoke works asynchronously by sending a request to the Dispatcher to be performed at some point, while Invoke blocks the calling thread until the Dispatcher has performed the request. However, since I neither access the same elements in the two loops, nor do I change anything about the list itself, so I don't understand how multithreading or the asynchronous nature of BeginInvoke could in any way interfere with what I am doing here.

Unfortunately, I have only found BeginInvoke errors connected with Lists when adding or removing items on SO, however, nothing where an error occurs when all threads seem to access different items at all times. If my code has some fundamental issues that I do simply not understand (which might cause my program to actually not function), then please clear this up for me or send me a link to an answer, I couldn't find. I have been stuck with this issue for a whole day now!

score 2 · Answer 1 · answered Feb 08 '16 at 17:55

2

This could have something to do with closures...

Try this:

  var current = myList[i].foo.DoCalc();
            if (current.ellipse == null)
                continue;    
            Application.Current.Dispatcher.BeginInvoke(DispatcherPriority.Normal, 
             new Action(() => current.ellipse.Fill = Brushes.Black));

answered Feb 08 '16 at 17:55

JWP

6,672
3
50
74

Thanks for the answer! I have actually tried this on my real program and the error still occurred, it does however, not on this simple example! – philkark Feb 08 '16 at 18:00
You might actually be right with this... sorry, I declared the reference (var current) OUTSIDE the for loop, which might cause the same issue as Scott has pointed out. – philkark Feb 08 '16 at 18:05
@phil13131 yep, that would cause the same issue. The declration must be inside the loop for each delegate to get it's own reference to a object instead of sharing them. – Scott Chamberlain Feb 08 '16 at 18:07

score 2 · Accepted Answer · edited May 23 '17 at 12:23

2

You are running in to variable capture. The i that gets used in the invoke call will likely be indices.Item2 + 1 not the i value that it had at the time you did the BeginInvoke. You must copy i in to a local variable that is created new each loop itteration.

private void Loop(object o)
{
    Tuple<int, int> indices = o as Tuple<int, int>;

    for(int i = indices.Item1; i <= indices.Item2; ++i)
    {
        myList[i].foo.DoCalc();
        if (myList[i].ellipse == null)
            continue;

        int iLocal = i;
        Application.Current.Dispatcher.BeginInvoke(DispatcherPriority.Normal, new Action(() => myList[iLocal].ellipse.Fill = Brushes.Black));
    }
}

foreach prior to C# 5 had the same issue, see here for more info.

edited May 23 '17 at 12:23

Community

1
1

answered Feb 08 '16 at 17:56

Scott Chamberlain

124,994
33
282
431

That is a very interesting issue and it seems to solve the problem, thanks!...however, I do not understand where the issue originates... – philkark Feb 08 '16 at 17:59
the variable `i` is not a "local copy" when you do `new Action(() => myList[i].ellipse.Fill = Brushes.Black)`, it is still the `i` from the `for` loop declration. So when `BeginInvoke` finally gets around to running the value of `i` has changed to be a new value. You must make a new variable that is declared inside of the loop to change this behavior, this makes each `() =>` get it's own variable instead of sharing one. – Scott Chamberlain Feb 08 '16 at 18:00
Is the issue because at the end of the for loop, I call ++i, leading to the last instance actually be increased by 1? – philkark Feb 08 '16 at 18:02
Correct, however even then you could have problems during the loop. for example if you have a loop that runs with `i = 0`, before the `BeginInvoke` starts you get another loop done that hits the `continue;` with `i = 1`, now the `BeginInvoke` finally runs, the `i` in `() => myList[i]` will be `1` not `0`. – Scott Chamberlain Feb 08 '16 at 18:05
Or say your loop runs 3 times before the `BeginInvoke` runs. You will get `() => myList[2]` ran 3 times instead of getting `() => myList[0]`, `() => myList[1]`, and `() => myList[2]` once. – Scott Chamberlain Feb 08 '16 at 18:06
Okay, thank you very much. I will change it to a local variable then, but actually use John Peters approach, so that I do not block `List` unnecessarily often for other threads. Still, your further explanation were more thorough, so I accept your answer. – philkark Feb 08 '16 at 18:10

BeginInvoke causes error due to skipped null check

2 Answers2