What is the best way to invoke a function?

Question

My program has 2 threads running, thread 1 does something to control a label in a form running on thread 2. So I have to use a delegate and invoke a function in form 1 class to access the label. My code is below and it works perfectly. However, I'm wondering if there is a shorter, better way to do this?

delegate void Change_Status_Call_Back(string status_changed);
    public void change_status(string status_changed)
    {
        if (this.label_status.InvokeRequired)
        {
            Change_Status_Call_Back obj = new Change_Status_Call_Back(change_status);
            this.Invoke(obj, new object[] { status_changed });
        }
        else
        {
            this.label_status.Text = status_changed;
        }
    }

Answer 1

After looking around, I came up with this:

// UPDATE DISPLAY items (using Invoke in case running on BW thread).
IAsyncResult h = BeginInvoke((MethodInvoker)delegate
{
    FooButton.Text = temp1;
    BarUpdown.Value = temp2;
}
);
EndInvoke(h);  // Wait for invoke to complete.
h.AsyncWaitHandle.Close();  // Explicitly close the wait handle.
                            // (Keeps handle count from growing until GC.)

Details:

• I removed if (InvokeRequired) completely. (Discovered from Peter Duniho's answer here.) Invoke() works just fine on the UI thread. In code that runs only on the UI thread, UI actions need no special treatment. In code that runs only on a non-UI thread, wrap all UI actions in an Invoke(). In code that can run on the UI thread -or- a non-UI thread, likewise wrap all UI actions in an Invoke(). Always using Invoke() adds some overhead when running on the UI thread, but: not much overhead (I hope); the actions run less often on the UI thread anyway; and by always using Invoke, you don't have to code the UI actions twice. I'm sold.
• I replaced Invoke() with BeginInvoke() .. EndInvoke() .. AsyncWaitHandle.Close(). (Found elsewhere.) Invoke() probably just does BeginInvoke() .. EndInvoke(), so that much is just inline expansion (slightly more object code; slightly faster execution). Adding AsyncWaitHandle.Close() addresses something else: When running on a non-UI thread, Invoke() leaves hundreds of handles that linger until garbage collection. (It's scary to watch Handles count grow in Task Manager.) Using BeginInvoke() .. EndInvoke() leaves lingering handles just the same. (Surprise: Using only BeginInvoke() does not leave the handles; it looks like EndInvoke() is the culprit.) Using AsyncWaitHandle.Close() to explicitly kill the dead handles eliminates the [cosmetic] problem of lingering handles. When running on the UI thread, BeginInvoke() .. EndInvoke() (like Invoke()) leaves no handles, so AsyncWaitHandle.Close() is unnecessary, but I assume it is also not costly.
• An IsDisposed test seems safe against race conditions, but I think it is not necessary. I'm worried that BackgroundWorker can Invoke() the operation; while it is pending, a click can trigger a callback on the UI thread that can Close() the form, and then the message loop executes this operation. (Not sure this can happen.)

Problem: (I will update here when something works.) I changed all my UI updates from running on a UI timer kludge to using Invoke() (as above), and now closing the form fails on a race condition about 20% of the time. If a user click stops my background worker, clicking on close after that works fine. BUT, if the user clicks directly on close, that triggers a callback on the UI thread which Close()s the form; that triggers another that flags the background worker to stop; the background worker continues, and it crashes at EndInvoke() saying "Cannot access a disposed object. Object name: 'MainWin'. at System.Windows.Forms.Control.MarshaledInvoke(Control caller, Delegate method, Object[] args, Boolean synchronous) ...". Adding if (!this.IsDisposed) {} around EndInvoke() .. AsyncWaitHandle.Close() doesn't fix it.

Option: Go back to using a forms timer: Make the BW write its changes into a dozen global "mailbox" variables. Have the timer do FooButton.Text = nextFooButtonText;, etc. Most such assignments will do almost nothing because setting a form field only updates the display if the value actually changes. (For clarity and to reduce copying objects, initialize the mailbox variables to null, and have the timer do if (nextFooButtonText != null) { FooButton.Text = nextFooButtonText; nextFooButtonText = null; }, etc.) The timer puts a new event on the UI message loop every so many milliseconds, which is more silly grinding than the Invoke()s. Updating the display on a timer callback delays each update by [up to] the timer interval. (Yuck.)

WORKING Option: Use only BeginInvoke(). Why make BW wait for each Invoke to finish? 1) temp1 and temp2 seem passed as references - if they get changed after BeginInvoke(), the new value wins. (But that's not so bad.) 2) temp1 and temp2 can go out of scope. (But aren't they safe against being released until the last reference goes away?) 3) Waiting ensures that BW only has one invoked action pending at a time - if the UI thread blocks for a while, BW can't bury it in events. (But my UI thread can't block, at least not at times when my BW is running.)

Option: Put try .. catch around the EndInvoke(). (Yuck.)

I have seen several other tricks suggested:
•Have Close cancel itself, initiate a timer, and then return so that any lingering Invoke()s finish on the UI thread; shortly after that the timer callback does a real Close (found here; from here).
•Kill the background worker thread.
•Alter Program.cs to shut down differently.

Answer 2

This question is "primarily opinion based". Still, you've touched a pet peeve of mine, so…

You should skip the InvokeRequired check altogether:

public void change_status(string status_changed)
{
    this.Invoke((MethodInvoker)(() => this.label_status.Text = status_changed));
}

The framework has to effectively check InvokeRequired anyway, because it's required to support invoking on the UI thread without deadlocking. So the check in your code is redundant. The overhead of always wrapping the method body in a delegate invocation is inconsequential in UI code like this, especially since if you're writing this code, it's probably the case that the method's not going to be called exception when InvokeRequired would be true anyway (i.e. the "fast path" is never taken anyway).

Even better is to use a more modern mechanism for dealing with cross-thread access, such as async/await or the Progress<T> class. Then you never have to write an explicit call to Invoke() at all.

Some time ago, I ranted in more depth here: MSDN’s canonical technique for using Control.Invoke is lame

Answer 3

I would do this:

public void change_status(string status_changed)
{
    this.label_status.InvokeSafely(c => c.Text = status_changed);
}

You need this extension method:

    public static void InvokeSafely(this Control control, Action<Control> action)
    {
        if (control.InvokeRequired)
        {
            control.Invoke((Action)(() => action?.Invoke(control)));
        }
        else
        {
            action?.Invoke(control);
        }
    }