views:

224

answers:

9

I know that when manipulating UI controls from any non-UI thread, you must marshal your calls to the UI thread to avoid issues. The general consensus is that you should use test InvokeRequired, and if true, use .Invoke to perform the marshaling.

This leads to a lot of code that looks like this:

private void UpdateSummary(string text)
{
    if (this.InvokeRequired)
    {
        this.Invoke(new Action(() => UpdateSummary(text)));
    }
    else
    {
        summary.Text = text;
    }
}

My question is this: can I leave out the InvokeRequired test and just call Invoke, like so:

private void UpdateSummary(string text)
{
    this.Invoke(new Action(() => summary.Text = text));
}

Is there a problem with doing this? If so, is there a better way to keep the InvokeRequired test while not having to copy and paste this pattern all over the place?

+5  A: 

Calling Invoke from the UI thread is somewhat inefficient.

Instead, you can create an InvokeIfNeeded extension method that takes an Action parameter. (this would also allow you to remove new Action(...) from the callsite)

SLaks
What makes `Invoke` from the UI thread inefficient?
Gabe
@Gabe: It interacts with the message queue.
SLaks
SLaks: `Invoke` is synchronous, so wouldn't it deadlock if it tried to interact with the message queue on the UI thread?
Gabe
@Gabe: No. It checks whether it's on the UI thread, and, if so, synchronously empties the queue. (In other words, it's a reentrant call)
SLaks
SLaks: You're saying that `Invoke` causes your action to get executed after any other queued invokes, as opposed to before the other queued invokes. How does changing the order make it inefficient?
Gabe
@Gabe: Not calling `Invoke` will not run any queued invokes. Also, the queue probably involves locks.
SLaks
SLaks: But those queued invokes would run on the UI thread anyway, just a bit later.
Gabe
+16  A: 

Well how about this:

public static class ContolHelpers
{
    public static void InvokeIfRequired(this Control control, Action action)
    {
        if (control.InvokeRequired)
        {
            control.Invoke(action);
        }
        else
        {
            action();
        }
    }
}

Use it like this:

private void UpdateSummary(string text)
{
    summary.InvokeIfRequired(() => summary.Text = text);
}
John Gietzen
That's exactly what I meant.
SLaks
@SLaks: Well, you beat me by 3 seconds, but I got teh codez. :)
John Gietzen
I've done exactly the same thing except I also added `public static T InvokeIfRequired(this Control control, Func<T> function)` as well for when I want to return a value.
Davy8
That's better - thanks =)
Erik Forbes
This pattern can be used in other situations aswell. At my company, we use `Funs.CatchLog(Action a, string name)` to catch and log "forgotten" exceptions. `Funs.Measure(Action a, string name)` measures how much time was spent in an action and so on...
lbruder
+1  A: 

I am not convinced that Control.Invoke is the best choice for updating the UI. I cannot say for sure in your case because I do not know the circumstances under in which UpdateSummary in called. However, if you are calling it periodically as a mechanism for displaying progress information (which is the impression I get from the code snippet) then there is usually a better option. That option is to have the UI thread poll for the status instead of having the worker thread push it.

The reasons why the polling approach should be considered in this case is because:

  • It breaks the tight coupling between the UI and worker threads that Control.Invoke imposes.
  • It puts the responsibility of updating the UI thread on the UI thread where it should belong anyway.
  • The UI thread gets to dictate when and how often the update should take place.
  • There is no risk of the UI message pump being overrun as would be the case with the marshaling techniques initiated by the worker thread.
  • The worker thread does not have to wait for an acknowledgement that the update was performed before proceeding with its next steps (ie. you get more throughput on both the UI and worker threads).

So consider creating a System.Windows.Forms.Timer that periodically checks for the text to be displayed on the Control instead of initiating the push from the worker thread. Again, without knowing your exact requirements I am not willing to say this definitely the direction you need to go, but in most many cases it is better than the Control.Invoke option.

Obviously this approach eliminates the necessity of the InvokedRequired check entirely. Nevermind, the fact that it simplies all other aspects of the UI / worker thread interaction.

Brian Gideon
I don't think that this 'simplies all aspects of the UI / worker thread interaction'. In fact, now you have to add synchronization around the `GetCurrentStatus` function on the worker. So, depending on how complex the worker already is, you could be introducing a wide array of threading issues.
John Gietzen
@John: Maybe, though the general pattern is to package the progress/status information in an immutable class and publish it by writing to a shared variable as a single point of interation. The only thing required here would be the `volatile` keyword. That seems a lot simplier than `Control.Invoke`ing in multiple places and having to consider the impacts it causes on *both* threads. I did edit my answer to use a less imposing tone regarding that point though.
Brian Gideon
Roger that, +1.
John Gietzen
+1  A: 

My preferred approach for view-only controls is to have all of the control state encapsulated in a class which can be updated without ever going through any inconsistent states (a simple way to do this is to put all things that need to be updated together into an immutable class, and create a new instance of the class whenever an update is needed). Then have a method which will Interlocked.Exchange an updateNeeded flag and, if there isn't an update pending but IsHandleCreated is true, then BeginInvoke the update procedure. The update procedure should clear the updateNeeded flag as the first thing it does, before doing any updates (if someone tries to update the control at that point, another request will be BeginInvoked). Note that you must be prepared to catch and swallow an exception (I think IllegalOperation) if the control gets disposed just as you're preparing to update it.

Incidentally, if a control hasn't yet been joined to a thread (by being added to a visible window, or having the window it's on become visible), it's legal to update it directly but not legal to use BeginInvoke or Invoke on it.

supercat
+2  A: 

I realize that there's already one answer that's pretty much spot on, but I wanted to also post my take on it (which I also posted here).

Mine is a little different in that it can slightly more safely handle null controls and can return results when necessary. Both of these have come in handy for me when trying to Invoke showing a MessageBox on a parent form that might be null, and returning the DialogResult of showing that MessageBox.


using System;
using System.Windows.Forms;

/// <summary>
/// Extension methods acting on Control objects.
/// </summary>
internal static class ControlExtensionMethods
{
    /// <summary>
    /// Invokes the given action on the given control's UI thread, if invocation is needed.
    /// </summary>
    /// <param name="control">Control on whose UI thread to possibly invoke.</param>
    /// <param name="action">Action to be invoked on the given control.</param>
    public static void MaybeInvoke(this Control control, Action action)
    {
        if (control != null && control.InvokeRequired)
        {
            control.Invoke(action);
        }
        else
        {
            action();
        }
    }

    /// <summary>
    /// Maybe Invoke a Func that returns a value.
    /// </summary>
    /// <typeparam name="T">Return type of func.</typeparam>
    /// <param name="control">Control on which to maybe invoke.</param>
    /// <param name="func">Function returning a value, to invoke.</param>
    /// <returns>The result of the call to func.</returns>
    public static T MaybeInvoke<T>(this Control control, Func<T> func)
    {
        if (control != null && control.InvokeRequired)
        {
            return (T)(control.Invoke(func));
        }
        else
        {
            return func();
        }
    }
}

Usage:

myForm.MaybeInvoke(() => this.Text = "Hello world");

// Sometimes the control might be null, but that's okay.
var dialogResult = this.Parent.MaybeInvoke(() => MessageBox.Show(this, "Yes or no?", "Choice", MessageBoxButtons.YesNo));
Mark Rushakoff
A: 

Keep the InvokeRequired.

Jas
A: 

That's a good task for Aspect-Oriented Programming:

http://www.sharpcrafters.com/solutions/multithreading

Rauhotz
A: 

It is easier to use BackgroudWorker, if possible, for making the UI responsive and use ReportProgress to update the UI because it runs on the same thread as the UI, hence you will not need InvokeRequired.

sh_kamalh
+2  A: 

I've been reading about the arguments back and forth on adding a logic check to find out if the invoke should be used IFF when not on the UI thread and not on the UI thread itself. I wrote a class that examines the time to execute (via Stopwatch) of various methods to get a rough estimate of the efficiency of one method over another.

The results may be surprising to some of you (these tests were run via the Form.Shown event):

     // notice that we are updating the form's title bar 10,000 times
     // directly on the UI thread
     TimedAction.Go
     (
        "Direct on UI Thread",
        () =>
        {
           for (int i = 0; i < 10000; i++)
           {
              this.Text = "1234567890";
           }
        }
     );

     // notice that we are invoking the update of the title bar
     // (UI thread -> [invoke] -> UI thread)
     TimedAction.Go
     (
        "Invoke on UI Thread",
        () =>
        {
           this.Invoke
           (
              new Action
              (
                 () =>
                 {
                    for (int i = 0; i < 10000; i++)
                    {
                       this.Text = "1234567890";
                    }
                 }
              )
           );
        }
     );

     // the following is invoking each UPDATE on the UI thread from the UI thread
     // (10,000 invokes)
     TimedAction.Go
     (
        "Separate Invoke on UI Thread",
        () =>
        {
           for (int i = 0; i < 10000; i++)
           {
              this.Invoke
              (
                 new Action
                 (
                    () =>
                    {
                       this.Text = "1234567890";
                    }
                 )
              );
           }
        }
     );

Results are as follows:

  • TimedAction::Go()+0 - Debug: [DEBUG] Stopwatch [Direct on UI Thread]: 300ms
  • TimedAction::Go()+0 - Debug: [DEBUG] Stopwatch [Invoke on UI Thread]: 299ms
  • TimedAction::Go()+0 - Debug: [DEBUG] Stopwatch [Separate Invoke on UI Thread]: 649ms

My conclusion is that you can safely invoke at any time, regardless whether you are on the UI thread or a worker thread, without significant overhead of looping back via the message pump. However, performing most of the work ON the UI thread instead of making many calls to the UI thread (via Invoke()) is advantageous and improve efficiency greatly.

Michael