MSDN’s canonical technique for using Control.Invoke is lame

While I’m on the subject of using the Control.Invoke method, I’d like to mention a pet peeve of mine. That is, the technique that everywhere on MSDN is proposed for dealing with using that method.


In particular, according to Microsoft, the preferred technique is to write one method that does two completely different things, depending on the value returned by the Control.InvokeRequired property. If the property is true, then Invoke is called using the same method as the target, and if it’s false, then whatever code was really desired to execute is in fact executed. It looks like this:


void DoSomethingMethod()
{
    if (InvokeRequired)
    {
        Invoke((MethodInvoker)DoSomethingMethod);
    }
    else
    {
        // actually do the "something"
    }
}

Now, I’m not a big fan of methods that do more than one thing in any case. It’s my opinion that a method should do one thing and do it well. There are exceptions to every rule of course, but they are few and far between. More importantly, the above pattern does not rise to the requirements of being such an exception.


To understand why, consider what happens if you call Invoke when InvokeRequired returns false. The designers of .NET could have implemented it to throw an exception if you try to call Invoke when not necessary. But there’s no obvious reason that they should have, nor did they. In fact, the Invoke method will “do the right thing” in that case, and simply invoke the target delegate directly rather than trying to marshall it onto the thread that owns the Control instance.


Note that the Invoke method can’t just always do the marshaling. It has to know whether doing so is necessary, because if it tried to marshall the invocation when it wasn’t necessary, it would wind up stuck waiting on the marshaled invocation to happen, which it never would because it’s waiting using the same thread that’s needed for the invocation.


So Invoke simply invokes the target directly, and to decide to do this it checks the same state your own code would be checking when it looks at the InvokeRequired property.


In other words, by using the MSDN-prescribed pattern, you’re duplicating the exact same effort that already is made by the .NET Framework.


My opinion is that it’s better to not have the redundant code, and to take advantage of the fact that .NET is already doing what MSDN proposes you do: just always call Invoke and let .NET sort it out. Now, you may be thinking “but if I always call Invoke using my own method as the target, won’t that cause an infinite recursion?” Yes, it would, so don’t do that. [:)]


Instead, take advantage of C#’s anonymous methods, wrapping all of your invoked logic inside one and invoke that:


void DoSomethingMethod()
{
    Invoke((MethodInvoker)delegate
    {
        // actually do the "something"
    });
}

Note: I prefer anonymous methods for this situation, but some may prefer a lambda expression instead, and especially if what you’re invoking actually returns some value, it might even be more expressive to use that. A lambda expression is a fine alternative to an anonymous method, and winds up compiled to basically the same thing.


One final thought: don’t judge Microsoft too harshly for promoting their inefficient approach so broadly. It’s unfortunate that new examples continue to be created, and that the newer versions of the documentation haven’t been changed to replace the older examples. But prior to C#/.NET 2.0, using an anonymous method in the invocation just wasn’t an option.


I wasn’t using .NET in those early days, and even then I would not have used the “one method, two behaviors” technique. Instead, I would have broken the functionality into two different methods, one that invokes the other. But the MSDN-promoted technique is less inappropriate in that context; in fact, it was as close as they could come to the benefit that anonymous methods offer of keeping all the functionality in a single method. Since I value that feature of anonymous methods so highly, I can hardly fault them for striving for that goal even when they didn’t have anonymous methods to use. [:)]

Form-closing race condition (part 2)

In my previous post, I mentioned that it turns out that there is a way for a form to be closed without the …Closing/…Closed methods/events being called. Any code (including the synchronized flag technique I described earlier) that relies on this notification will fail under that scenario.


So, what to do? Well, as I mentioned before, one solution is simply to not depend on those methods or events. For example, in the “invoking/closing race condition” scenario where I started this whole thing, it’s reasonable to just allow an exception to occur if the worker thread loses the race, catching and ignoring it. But sometimes, that’s not an option.


In those situations, it’s useful to know how to take things into your own hands. The problem is that when you pass a form instance to Application.Run, when that form is closed, the Application class interrupts the Run method’s message loop and closes all the windows owned by the thread. Without the message loop running, when the windows associated with other forms are closed, those forms never get to process their close-related methods and events.


To fix this, we simply need to do basically the same work, but change the order of operations so that when the other forms are closed, they still get to do their close-related processing. The basic technique looks like this:


while (Application.OpenForms.Count > 0)
{
    Application.OpenForms[0].Close();
}

Application.ExitThread();

The call to Application.ExitThread is necessary to cause the Application.Run method to exit.


The logic can be put in one of two places. Either in the main form’s own OnFormClosed method, or in the Program class as an event handler. Either way is fine, but you might prefer doing it as an event handler in the Program class if you would prefer for the main form to not have to know its relationship to the UI generally. This would be especially useful in scenarios where more than one form class might be used as the “main” form, but it’s a nice abstraction in any case.


Using the latter approach as our example, if we assume that the above code is already in an event handler named _CloseHandler, then in the Program.Main method instead of something like this (the default from the IDE):


Application.EnableVisualStyles();
Application.SetCompatibleTextRenderingDefault(false);
Application.Run(new Form1());

You’d have something like this:


Application.EnableVisualStyles();
Application.SetCompatibleTextRenderingDefault(false);

Form formShow = new Form1();

formShow.FormClosed += _CloseHandler;
formShow.Show();

Application.Run();

And that’s all there is to it. [:)]

Form-closing race condition (part 1)

Okay, I know last post I said next I would be writing about more network stuff. But I’ve been away from the blog, and in returning to it I had to revisit an issue that came up when I was doing the GUI stuff. The issue is a race condition between a thread that may need to invoke code on the GUI thread, and the GUI thread itself.


Some brief background: in Windows, GUI objects are tied to the thread on which they were created. There are a variety of ways to deal with this, but in the .NET Forms API, it’s addressed using the Control.Invoke or Control.BeginInvoke method. These methods take a delegate and ensure that it’s executed on the same thread that created the Control instance used to call the method. Invoke is synchronous, not returning until the delegate has been executed, while BeginInvoke returns right away, with the delegate being executed at some later time.


This allows some thread other than the GUI thread to execute code that is only permitted to be executed on the GUI thread. Most commonly, this would be used to allow some worker thread to update the user interface, but another use is to implement an easy form of synchronization.


If you’re reading this, you probably already knew all that. But if you don’t, you probably will want to make sure you understand the above, or review the related concepts on MSDN.


So, what’s the race condition I’m talking about? Consider a form that starts a worker thread, where the form can be closed by the user before the worker thread is done. A natural thing for such a form to do when being closed is to cause the worker thread to interrupt its work and quit. Now consider such a situation, but in addition to all the above the worker thread notifies the form upon completion via an invoked delegate.


Invoking a delegate requires a valid window handle, but a form only has a valid window handle after it’s been displayed but before it’s been closed. If the thread is terminating because the form is being closed, that’s simple enough to deal with: just have the form set a flag, checked before invoking the delegate, before telling the thread to terminate, or simply check the Control.IsDisposed property before trying to invoke the delegate and make sure the thread isn’t instructed to terminate until the form is safely disposed.


But, what if the thread was already terminating on its own, at the same instant that the form is being closed by the user? Racers, start your engines! [:)]


When that happens, there’s a possibility that the thread that is in the process of closing the form will dispose it, but do so just after the worker thread has checked the flag (special-purpose or IsDisposed property) and just before the worker thread actually tries to call Invoke.


If you’ve dealt with race conditions before, you already know what’s coming. One way to address this is to synchronize access to the flag, so that it’s assured to not be changed by one thread while it’s being used by another. It would be nice if we could do this with the IsDisposed property, but we don’t have control over the code that modifies that, so to use this approach would require a new flag. We can set it in the OnFormClosed method and check it before invoking the delegate of interest:


bool _fClosed = false;
object _objLock = new object();

protected override void OnFormClosed(FormClosedEventArgs e)
{
    lock (_objLock)
    {
        _fClosed = true;
    }

    base.OnFormClosed(e);
}

void SomeMethod()
{
    lock (_objLock)
    {
        if (!_fClosed)
        {
            BeginInvoke((MethodInvoker) delegate { });
        }
    }
}

One thing you might notice is that the above code uses BeginInvoke instead of Invoke. With this technique, using Invoke would be a major no-no. Why? Because Invoke introduces a circular lock dependency, which could lead to a deadlock condition. That is, any code executing on the GUI thread has essentially locked the GUI thread, taking the lock that any code trying to call Invoke will want. At the same time, we’ve introduced an explicit lock (_objLock).


If the worker thread gets the explicit lock while at the same time that the GUI thread starts to execute the OnFormClosed method, then we’ll wind up with two different threads, each holding the lock that the other thread needs in order to continue.


Using BeginInvoke gets around this problem by simply queuing the delegate for execution, avoiding the need for the worker thread to ever need to get the lock that’s implicit in the behavior of the GUI thread.


But wait! There’s still a problem. It turns out that there’s a way for a form to be closed without the OnFormClosing or OnFormClosed methods ever being called. Personally, this seems like a design flaw to me. But that’s the way .NET works and we have to live with it. This means that we could conceivably wind up in the method trying to call Invoke (or BeginInvoke) with a disposed form that hasn’t ever set the special-purpose _fClosed flag.


How can this be? Well, it turns out that by default, a .NET Forms application is set up such that there’s one main form, and when that form closes, it terminates the message pump and closes all of the other windows owned by that thread. Because of the way it does that, the other forms never get a chance to execute their …Closing/…Closed methods and events.


One way to deal with this, as well as the original race condition issue, is to just give up and let .NET do it. Wrap the call to Invoke with a try/catch block and let it fail if the form has been disposed before the code trying to call Invoke gets a chance to.


This is actually a fine way to deal with the race condition, I think. The fact is, the overhead of the exception isn’t going to matter, and it’s actually about the simplest way to implement a fix.


But, there’s another technique that when used in conjunction with the explicit synchronized flag will ensure that the synchronized flag technique is reliable. It’s kind of interesting in its own right, and so in my next post I’ll describe that approach.