Is it appropriate to extend Control to provide consistently safe Invoke/BeginInvoke functionality?

In the course of my maintenance for an older application that badly violated the cross-thread update rules in winforms, I created the following extension method as a way to quickly fix illegal calls when I've discovered them:

/// <summary>
/// Execute a method on the control's owning thread.
/// </summary>
/// <param name="uiElement">The control that is being updated.</param>
/// <param name="updater">The method that updates uiElement.</param>
/// <param name="forceSynchronous">True to force synchronous execution of 
/// updater.  False to allow asynchronous execution if the call is marshalled
/// from a non-GUI thread.  If the method is called on the GUI thread,
/// execution is always synchronous.</param>
public static void SafeInvoke(this Control uiElement, Action updater, bool forceSynchronous)
{
    if (uiElement == null)
    {
        throw new ArgumentNullException("uiElement");
    }

    if (uiElement.InvokeRequired)
    {
        if (forceSynchronous)
        {
            uiElement.Invoke((Action)delegate { SafeInvoke(uiElement, updater, forceSynchronous); });
        }
        else
        {
            uiElement.BeginInvoke((Action)delegate { SafeInvoke(uiElement, updater, forceSynchronous); });
        }
    }
    else
    {
        if (!uiElement.IsHandleCreated)
        {
            // Do nothing if the handle isn't created already.  The user's responsible
            // for ensuring that the handle they give us exists.
            return;
        }

        if (uiElement.IsDisposed)
        {
            throw new ObjectDisposedException("Control is already disposed.");
        }

        updater();
    }
}

Sample usage:

this.lblTimeDisplay.SafeInvoke(() => this.lblTimeDisplay.Text = this.task.Duration.ToString(), false);

I like how I can leverage closures to read, also, though forceSynchronous needs to be true in that case:

string taskName = string.Empty;
this.txtTaskName.SafeInvoke(() => taskName = this.txtTaskName.Text, true);

I don't question the usefulness of this method for fixing up illegal calls in legacy code, but what about new code?

Is it good design to use this method to update UI in a piece of new software when you may not know what thread is attempting to update the ui, or should new Winforms code generally contain a specific, dedicated method with the appropriate Invoke()-related plumbing for all such UI updates? (I'll try to use the other appropriate background processing techniques first, of course, e.g. BackgroundWorker.)

Interestingly this won't work for ToolStripItems. I just recently discovered that they derive directly from Component instead of from Control. Instead, the containing ToolStrip's invoke should be used.

Followup to comments:

Some comments suggest that:

if (uiElement.InvokeRequired)

should be:

if (uiElement.InvokeRequired && uiElement.IsHandleCreated)

Consider the following msdn documentation:

This means that InvokeRequired can return false if Invoke is not required (the call occurs on the same thread), or if the control was created on a different thread but the control's handle has not yet been created.

In the case where the control's handle has not yet been created, you should not simply call properties, methods, or events on the control. This might cause the control's handle to be created on the background thread, isolating the control on a thread without a message pump and making the application unstable.

You can protect against this case by also checking the value of IsHandleCreated when InvokeRequired returns false on a background thread.

If the control was created on a different thread but the control's handle has not yet been created, InvokeRequired returns false. This means that if InvokeRequired returns true, IsHandleCreated will always be true. Testing it again is redundant and incorrect.

Solution 1:

You should create Begin and End extension methods as well. And if you use generics, you can make the call look a little nicer.

public static class ControlExtensions
{
  public static void InvokeEx<T>(this T @this, Action<T> action)
    where T : Control
  {
    if (@this.InvokeRequired)
    {
      @this.Invoke(action, new object[] { @this });
    }
    else
    {
      if ([email protected])
        return;
      if (@this.IsDisposed)
        throw new ObjectDisposedException("@this is disposed.");

      action(@this);
    }
  }

  public static IAsyncResult BeginInvokeEx<T>(this T @this, Action<T> action)
    where T : Control
  {
    return @this.BeginInvoke((Action)delegate { @this.InvokeEx(action); });
  }

  public static void EndInvokeEx<T>(this T @this, IAsyncResult result)
    where T : Control
  {
    @this.EndInvoke(result);
  }
}

Now your calls get a little shorter and cleaner:

this.lblTimeDisplay.InvokeEx(l => l.Text = this.task.Duration.ToString());

var result = this.BeginInvokeEx(f => f.Text = "Different Title");
// ... wait
this.EndInvokeEx(result);

And with regards to Components, just invoke on the form or container itself.

this.InvokeEx(f => f.toolStripItem1.Text = "Hello World");

Solution 2:

I like the general idea, but I do see one problem. It is important to process EndInvokes, or you can have resource leaks. I know a lot of people don't believe this, but it really is true.

Here's one link talking about it. There are others as well.

But the main response I have is: Yes, I think you've got a nice idea here.