Is it appropriate to extend Control to provide con

Question

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.

Answer 1

This is not actually an answer but answers some comments for the accepted answer.

For standard IAsyncResult patterns, the BeginXXX method contains AsyncCallback parameter, so if you want to say "I don't care about this--just call EndInvoke when it's done and ignore the result", you can do something like this (this is for Action but should be able to be adjusted for other delegate types):

    ...
    public static void BeginInvokeEx(this Action a){
        a.BeginInvoke(a.EndInvoke, a);
    }
    ...
    // Don't worry about EndInvoke
    // it will be called when finish
    new Action(() => {}).BeginInvokeEx(); 

(Unfortunately I don't have a solution not to have a helper function without declaring a variable each time when use this pattern).

But for Control.BeginInvoke we do not have AsyncCallBack, so there is no easy way to express this with Control.EndInvoke guaranteed to be called. The way it has been designed prompts the fact that Control.EndInvoke is optional.

Answer 2

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 (!@this.IsHandleCreated)
        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");

Answer 3

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.