Weird InvokeRequired issue

Question

I have a UserControl with a TreeView control called mTreeView on it. I can get data updates from multiple different threads, and these cause the TreeView to be updated. To do this, I've devised the following pattern: all data update event handlers must acquire a lock and then check for InvokeRequired; if so, do the work by calling Invoke. Here's the relevant code:

  public partial class TreeViewControl : UserControl
  {  
    object mLock = new object();
    void LockAndInvoke(Control c, Action a)
    {
      lock (mLock)
      {
        if (c.InvokeRequired)
        {
          c.Invoke(a);
        }
        else
        {
          a();
        }
      }
    }

    public void DataChanged(object sender, NewDataEventArgs e)
    {
      LockAndInvoke(mTreeView, () =>
        {
          // get the data
          mTreeView.BeginUpdate();
          // perform update
          mTreeView.EndUpdate();
        });
    }    
  }

My problem is, sometimes, upon startup, I will get an InvalidOperationException on mTreeView.BeginUpdate(), saying mTreeView is being updated from a thread different than the one it was created. I go back in the call stack to my LockAndInvoke, and lo and behold, c.InvokeRequired is true but the else branch was taken! It's as if InvokeRequired had been set to true on a different thread after the else branch was taken.

Is there anything wrong with my approach, and what can I do to prevent this?

EDIT: my colleague tells me that the problem is that InvokeRequired is false until the control is created, so this is why it happens on startup. He's not sure what to do about it though. Any ideas?

Answer 1

It is a standard threading race. You are starting the thread too soon, before the TreeView is created. So your code sees InvokeRequired as false and fails when a split second later the native control gets created. Fix this by only starting the thread when the form's Load event fires, the first event that guarantees that all the control handles are valid.

Some mis-conceptions in the code btw. Using lock is unnecessary, both InvokeRequired and Begin/Invoke are thread-safe. And InvokeRequired is an anti-pattern. You almost always know that the method is going to be called by a worker thread. So use InvokeRequired only to throw an exception when it is false. Which would have allowed diagnosing this problem early.

Answer 2

When you marshal back to the UI thread, it's one thread--it can do only one thing at at time. You don't need any locks when you call Invoke.

The problem with Invoke is that it blocks the calling thread. That calling thread usually doesn't care about what get's completed on the UI thread. In that case I recommend using BeginInvoke to marshal the action back to the UI thread asynchronously. There are circumstances where the background thread can be blocked on Invoke while the UI thread can be waiting for the background thread to complete something and you end up with a deadlock: For example:

private bool b;
public void EventHandler(object sender, EventArgs e)
{
  while(b) Thread.Sleep(1); // give up time to any other waiting threads
  if(InvokeRequired)
  {
    b = true;
    Invoke((MethodInvoker)(()=>EventHandler(sender, e)), null);
    b = false;
  }
}

... the above will deadlock on the while loop while because Invoke won't return until the call to EventHandler returns and EventHandler won't return until b is false...

Note my use of a bool to stop certain sections of code from running. This is very similar to lock. So, yes, you can end up having a deadlock by using lock.

Simply do this:

public void DataChanged(object sender, NewDataEventArgs e)
{
      if(InvokeRequired)
      {
          BeginInvoke((MethodInvoker)(()=>DataChanged(sender, e)), null);
          return;
      }
      // get the data
      mTreeView.BeginUpdate();
      // perform update
      mTreeView.EndUpdate();
}

This simply re-invokes the DataChanged method asynchronously on the UI thread.

Answer 3

The pattern as you have shown it above looks 100% fine to me (albeit with some extra unnecessary locking, however I can't see how this would cause the problem you have described).

As David W points out, the only difference between what you are doing and this extension method is that you directly access mTreeView on the UI thread instead of passing it in as an argument to your action, however this will only make a difference if the value of mTreeView changes, and in any case you would have to try fairly hard to get this to cause the problem you have described.

Which means that the problem must be something else.

The only thing that I can think of is that you may have created mTreeView on a thread other than the UI thread - if this is the case then accessing the tree view will be 100% safe, however if you try and add that tree view to a form which was created on a different thread then it will go bang with an exception similar to the one that you describe.