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

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.

See

+1 for the question - I find myself writing Invoke callbacks for things on the main UI thread all the time. If the extension method doesn't contain too many drawbacks, this would be a huge timesaver. – Cloudberry

I'd rename it something like "SafeInvoke" – Ginaginder

@Joel: That is a better name. Updating question to reflect the suggestion. (I never actually liked the name "UpdateUI" very much.) – See

I've always made my ToolStripItem's static to get around that particular problem. – Cloudberry

You need to check for InvokeRequired && IsHandleCreated together. You cannot call BeginInvoke (or Invoke) without the handle being created first, you will get an exception. – Average

@Garo: It is illegal to check IsHandleCreated from a different thread. Control's docs list only the following methods/props as safe for crossthread calls: Invoke, InvokeRequired, BeginInvoke, EndInvoke, and CreateGraphics. This limitation drove my desire to create a standard method to do this. – See

@Greg D - He's not checking for the handle from a different thread - it will only execute when invoke is not required (e.g., you're currently on the control creating thread). – Randazzo

This method will silently ignore calls to controls that don't have handles yet. It might suppress calls that should go through (e.g., handle gets created after the handle check but before the return). – Randazzo

Yes, it does silently ignore calls to controls that don't have handles. That's by design (as indicated in the comment). It also sidesteps the nastiness that occurs when the handle's been destroyed before the asynchronous invoke has occurred. What should it do instead? My initial private implementation actually registered a handler on the control's HandleCreated event to invoke/run the desired action, but I think that's a change which requires special casing, not general purpose like this is intended to be. – See

re: the race condition, the call to SafeInvoke simply loses. If the call to SafeInvoke was actually from the GUI thread, the handle can't be created there because the handle must be created on the GUI thread. – See

Am I right in thinking if I do this.SafeInvoke on a form all subcontrols are called safely? e.g. this.SafeInvoke(() => { this.lblContent = "hello world"; this.txtEntry.Text = "Text Box"; },true); or would I need to call SafeInvoke for each control separately? – Stiletto

In a typical application you're going to create all controls on the same thread. A SafeInvoke would marshal to the same thread in all cases, so you could get away with merging all the changes in a single call. If you're working in a legacy system or a system where the threading is all messed up anyway, I'd definitely recommend independent calls just to be sure. (That's sort-of how it's designed, just like you'd use normal invoke.) Also, try to avoid forcing synchronous updates in a write-only scenario. You're slowing yourself down without a good reason. :) – See

1) Wouldn't it be more sensible to throw an exception if IsHandleCreated is false? If you did that the user would get an indication that they did something wrong. As it is now it could go unnoticed and cause confusing bugs later. – Grazynagreabe

2) What is the purpose of calling SafeInvoke in the delegates passed to Invoke/BeginInvoke instead of just calling updater directly? – Grazynagreabe

@Lii: (1) There are lots of things you could do. If you prefer to throw an exception, go for it. (2) We haven't validated all req'd preconditions yet when we get to the BeginInvoke()/Invoke(). E.g., we haven't verified that the handle exists. Calling updater directly would sidestep validation. – See

But... When in the second call to SafeInvoke (from the delegate) InvokeRequired has been called and returned true. You mentioned in the question text that in that case there is no need to call IsHandleCreated. Does this mean that the handle could have been "reset" after the Invoke, so that IsHandleCreated returns false there, even if InvokeRequired returned true before the invoke? – Grazynagreabe

In an asynchronous environment, handles may be created or destroyed asynchronously. – See

Answer 1

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.

Octennial

Agreed about EndInvoke: shouldn't be that hard to use a waithandle to call it right there, though. – Ginaginder

In fact, there's an SO question about it: stackoverflow.com/questions/532722/… – Octennial

FWIW, I recall reading on MSDN that, in this single, special, magical, particular case, it's documented to not be required. I don't have a link, though, and it isn't true until I do. :) – See

I vaguely remember something about that too. – Octennial

(I don't know if that link still works or not - it's taking a long time to load for me at the minute over 3G. It at least used to say that though :) – Premature

I've had timeouts with other MS blogs this morning already, so I suspect it's an issue with MS' system today. – Ginaginder

Relevant portion from Jon's link: >> "I just got the official word from the WinForms team. It is not necessary to call Control.EndInvoke. You can call BeginInvoke in a "fire and forget" manner with impunity." << Note that was in the comments, and it's from 5/2003, but it's probably still true. – Ginaginder

Yes, but I've seen later posts that seem to contradict that. It would be great to get the final, correct answer once and for all. – Octennial

Does anybody know a WinForms PM? :) – See

You don't have to call Control.EndInvoke because the Control.BeginInvoke call resolves to a PostMessage onto the message loop of the control's thread. You can't easily tell if this message gets processed, and there's no return value. – Average

That makes sense, no doubt about it. I am inclined to believe all of you, because Control.BeginInvoke / EndInvoke really has very little in common with async delegates (other than the name). But I have heard and read contradictory claims. I'd feel better hearing it from some authority. – Octennial

@CharlieFlowers From the link that you gave, "The only documented exception to the rule that I'm aware of is in Windows Forms, where you are officially allowed to call Control.BeginInvoke without bothering to call Control.EndInvoke." – Dethrone

@LimitedAtonement: Is that really an "exception" to the "rule", or would it be more fair to say that the rule calls for calls to Delegate.BeginInvoke to be balanced by calls to Delegate.EndInvoke, but imposes no requirement upon other unrelated functions that also happen to be named BeginInvoke? Since Control.BeginInvoke bears no relation to Delegate.BeginInvoke, there's no reason rules which apply to the former should apply to the latter. – Overby

@Overby I think the rule that was being referred to is the Begin and End pattern which is documented here: msdn.microsoft.com/en-us/library/ms228963.aspx . – Dethrone

@LimitedAtonement: Right; that page talks about the BeginInvoke method inherited by delegates; it has nothing whatsoever to do with the similarly-named BeginInvoke method inherited by controls. – Overby

@Overby It's not limited to delegates but any "asynchronous operation that uses the IAsyncResult design pattern". They use FileStream as an example, and Control is just as good an example. This isn't documentation on a particular method, but it instead documents a common pattern used throughout the framework. – Dethrone

@LimitedAtonement: Okay, you're right. I was misunderstanding things a little; Control is somewhat unusual in that its BeginInvoke method takes a delegate, rather than performing some pre-defined action (like reading data from a file). I dislike the fact that the IAsyncResult does not provide any clean way to say, in essence, "I don't care about this--just call EndInvoke when it's done and ignore the result". In some contexts that may be icky, but if an asynchronous I/O operation gets stuck, there may not be any practical alternative. – Overby

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 ([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");

Aceous

Interesting. If we do this, we need to insert an appropriate call to EndInvoke(), I think, b/c we'll be stepping outside the perceived "safe zone" of ignoring Control.EndInvoke(). Also, it isn't safe to check IsHandleCreate or IsDisposed before the invoke. – See

I don't understand the benefit from the type parameter. Unless- is it for use from languages that don't support closures? – See

BeginInvokeEx will throw exception in the call to BeginInvoke if the Control handle has not yet been created. Hence, the reuse of InvokeEx is likely not possible and we need to copy the code over or make another private method used by both InvokeEx and BeginInvokeEx that accepts method delegate as argument. – Windowshop

Answer 3

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.

Unimpeachable

EndInvoke is optional in this very specific case because of an implementation detail that likely won't change due to backcompat concerns. I do not think it's because there's a missing parameter in a method signature. – See

I am agree the wording is a little bit misleading. What I wanted to say is, because the Control.BeginInvoke and Control.EndInvoke were designed not to be coupled, this prompts the user to think that Control.EndInvoke is optional. – Unimpeachable