Thread-safe events - is this a "clean" way?

Question

I stumbled over some code in a professional library and am uncertain if this is a clean way to handle cross-thread event calls.

The code below is in a forms application. Thread calls are made from a class that itself starts a new thread and receives messages:

private void Library_StatusChanged(object sender, AbstractTestCase.StatusChangedEventArgs e)
{
    if (this.InvokeRequired)
    {
        this.lblProgress.Invoke((MethodInvoker)delegate ()
        {
            lblProgress.Text = "Current state: " + e.Step;
            lblProgress.Refresh();
        }
        );

        this.pbProgess.Invoke((MethodInvoker)delegate ()
        {
            pbProgess.Value = e.Percentage;
            pbProgess.Refresh();
        });

        this.lstStatus.Invoke((MethodInvoker)delegate ()
        {
            lstStatus.Items.Add("    " + e.Step);
            lstStatus.Refresh();

        });

        this.Invoke((MethodInvoker)delegate ()
        {
            this.Refresh();
        });
    }
    else
    {
        lblProgress.Text = "Current state:" + e.Step;
        lblProgress.Refresh();

        pbProgess.Value = e.Percentage;
        pbProgess.Refresh();

        lstStatus.Items.Add("    " + e.Step);
        lstStatus.Refresh();

        this.Refresh();
    }

    Application.DoEvents();
}

Is this "state of the art"? In my opinion it's a little messy?!

I usually take the use of `Application.DoEvents` as a sign something isn't right. And it probably would be more efficient to have just a single `Invoke` instead of 4 separate ones. — Matt Burland, Dec 03 '15 at 15:00
The Library is single threaded (besides some rare multi-threaded parts) and thus the UI freezes. — AllDayPiano, Dec 03 '15 at 15:01
*"and thus the UI freezes"* and `Application.DoEvents` is a really crappy way to paper over that problem. — Matt Burland, Dec 03 '15 at 15:04
Also on the question of *is this state-of-the-art*, well, no, as @usr has already answered. But bear in mind if the library has been around for a while and is working, there really isn't a lot of upside to changing the whole thing because `await` is in vogue now. On the other hand, it is kind of crappy code to start with, so... — Matt Burland, Dec 03 '15 at 15:06
Remember what DoEvents does. It does not magically unwind the stack back to the thread's message loop. It *processes another message*. Suppose that message causes a library status change? We are now *recursively* calling the event handler. What stops an unbounded recursion if this keeps happening? No code here that I see. This code clearly is not using best practices, and may have been written by someone who does not understand message processing. — Eric Lippert, Dec 03 '15 at 15:42
Does that library actually expect all those different controls to be running on different UI threads (which is possible, it's even possible for them to be different processes)? If not then a single call to invoke to update all the UI at once would be sufficient. — Chris Chilvers, Dec 03 '15 at 15:47

score 5 · Accepted Answer · answered Dec 03 '15 at 15:02

5

State of the art is using await. If that is not possible here, at least simplify the code to a single Invoke call. It is not needed to invoke on each control, just invoke anywhere on the UI thread.

The InvokeRequired check should not be required because you should know on what thread the event is raised.

In any case duplicating logic such as "Current state: " + e.Step is really a bad idea and I would fail this in a code review no matter what.

The presence of Application.DoEvents is a really bad sign. Probably a misunderstanding because it only makes sense to call it on the UI thread, but why Invoke when already on the UI thread?! Looks like a contradiction.

lstStatus.Refresh(); is also a misunderstanding, probably superstitious. Controls refresh automatically (if you allow for event processing).

answered Dec 03 '15 at 15:02

usr

168,620
35
240
369

Hi usr, to answer your questions: The invokation is required because the event may not have been fired by the same thread. There's something like an asynchronous com port access that is running in another thread. It might also happen, that the UI thread is completely busy and thus the DoEvent might redraw the form?! Same for Control.Refresh() – AllDayPiano Dec 03 '15 at 15:08
1

*`lstStatus.Refresh();` is also a misunderstanding, probably superstitious.* Cargo-cult programming. – Matt Burland Dec 03 '15 at 15:08
@MattBurland yeah, the guy did it once and it hid a UI thread freeze so he now forever thinks it is the solution. – usr Dec 03 '15 at 15:09
@AllDayPiano OK, so we *always* need invoke. Delete the InvokeRequired check.; `It might also happen, that the UI thread is completely busy` is is bad design. I would not call it strictly a bug but undesirable.; DoEvents on a background thread does nothing, it must run on the/a UI thread. The Refresh call is 100% redundant given the call to DoEvents. – usr Dec 03 '15 at 15:10
So its a structural problem, right? But - however - how do you redraw a form while the CPU load exceeds the 100%? As far as I'm in programming, I'd start another thread and run the UI asynchronously. – AllDayPiano Dec 03 '15 at 15:11
Normally, the UI thread has little to do. You push all work to a worker thread and then invoke very briefly back to the UI thread. If all CPUs are at 100% (are they really?) you should lower the priority of the background threads so that you do not affect the entire OS including your app. – usr Dec 03 '15 at 15:12
`await` changes none of that but it makes this cross thread communication significantly easier to do correctly. – usr Dec 03 '15 at 15:13
As i see, my answer is right... You all said about my simple solution. – Igor Quirino Dec 03 '15 at 15:13

Igor Quirino · Answer 2 · 2015-12-03T15:07:50.483

1

When you use invoke, a statement is added in a queue to be processed by the UI thread.

Use this simple solution:

private void Library_StatusChanged(object sender, AbstractTestCase.StatusChangedEventArgs e)
{
    this.lblProgress.Invoke((MethodInvoker)delegate ()
    {
        lblProgress.Text = "Current state: " + e.Step;
    });

    this.pbProgess.Invoke((MethodInvoker)delegate ()
    {
        pbProgess.Value = e.Percentage;
    });

    this.lstStatus.Invoke((MethodInvoker)delegate ()
    {
        lstStatus.Items.Add("    " + e.Step);
    });
}

edited Dec 03 '15 at 15:07

answered Dec 03 '15 at 15:05

Igor Quirino

1,187
13
28

1

This isn't the OPs code. It's from a library. I'm assuming they aren't planning on re-writing the library. They are just asking a question about the code they have encountered. – Matt Burland Dec 03 '15 at 15:07
Jup Matt, you're right. I don't like the lib but for free is cheap, cheap is good, good is close to perfect and perfect is, what is expected :-/ – AllDayPiano Dec 03 '15 at 15:15
@AllDayPiano: I must disagree. As this code clearly demonstrates, "cheap" does *not* imply "good". – Stephen Cleary Dec 03 '15 at 19:28
Haha Stephen, you're right. But don't tell me - tell my superiors :) – AllDayPiano Dec 04 '15 at 06:18

Thread-safe events - is this a "clean" way?

2 Answers2

Linked