2

I'm refactoring some old WinForms code to use async/await, and I've hit a code analysis warning that I'm not sure how to fix properly.

The situation is that I have a method that is called from a non-UI thread which needs to update the UI. The old code uses the time-honoured technique of calling Control.BeginInvoke() to run the code on the UI thread.

The problem is that the method I want to invoke is now async and attempting to use Control.BeginInvoke() yields the warning VSTHRD101: "Avoid using async lambda for a void returning delegate type, because any exceptions not handled by the delegate will crash the process."

The following sample WinForms application demonstrates the problem. It's a simple form containing only a Button called "testBtn" and a Label called "label1".

The call to ControlBeginInvoke() inside the notRunningOnUiThread() method causes the warning as commented:

using System;
using System.Threading.Tasks;
using System.Windows.Forms;

#pragma warning disable CA2007 // Consider calling ConfigureAwait on the awaited task (disabled for brevity in this sample code)

namespace Net5WinFormsApp
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
            CheckForIllegalCrossThreadCalls = true; // For test purposes to prove that mustRunOnUiThreadAsync() runs on the UI thread.
        }

        // This method is only to simulate what's happening in the real code. 
        // Assume that you cannot change this.
        void testBtn_Click(object sender, EventArgs e)
        {
            _ = Task.Run(() => notRunningOnUiThread(this)); // Not my real code, but this simulates it.
        }

        // NOTE: In the real code this method is not a member of the Control,
        // but it does have access to the Control to update.
        void notRunningOnUiThread(Control ui) // Called from a non-UI thread and wants to call a method that updates the UI.
        {
            // The next line causes warning VSTHRD101:
            // "Avoid using async lambda for a void returning delegate type,
            // because any exceptions not handled by the delegate will crash the process."

            ui.BeginInvoke(new Action(async () => await mustRunOnUiThreadAsync()));
        }

        async Task mustRunOnUiThreadAsync()
        {
            label1.Text = "Before update";
            await Task.Delay(1000).ConfigureAwait(true);
            label1.Text = "After update";
        }
    }
}

If I suppress the warning, everything seems to run OK even if there are exceptions in mustRunOnUiThreadAsync() (if there's an exception, it's reported immediately and not from the finalizer for a Task).

My question is: Is it safe to suppress this warning? And if not, what's a good way to fix it?

The fundamental thing I want to solve is how to safely call an async UI method on the UI thread from a non-UI thread in a WinForms application, when only the ui Form is available and the current synchronisation context is not available.

(Note that I can't simply await mustRunOnUiThreadAsync() in notRunningOnUiThread(). That would cause a "Cross-thread operation not valid" InvalidOperationException to be thrown.)

I suppose one possible solution is to write a non-async method that calls the async method, using the advice from this answer. But is that the best approach for this particular case?


[EDIT]

I should have mentioned: In my actual code the notRunningOnUiThread() is not a member of the Control that needs to update the UI. Because notRunningOnUiThread() isn't running on a UI thread, I can't use System.Threading.SynchronizationContext since it is the wrong context. I do have the Control available for calling Control.BeginInvoke() however.

The actual situation is that I am servicing an RPC request which must update some UI using a method that happens to be async.

Matthew Watson
  • 104,400
  • 10
  • 158
  • 276
  • 1
    _"Is it safe to suppress this warning?"_ - Out of curiousity, what happens if you make `mustRunOnUiThreadAsync()` throw an exception? Does the warning live up to its name? –  Aug 03 '21 at 09:18
  • The [Progress](https://learn.microsoft.com/en-us/dotnet/api/system.progress-1) Class can also raise an event and the event handler can be async. This event is of course raised in the UI Thread (if the Progress object is created in the UI Thread). No need for that weird `BeginInvoke()` call, just pass a Progress delegate to `notRunningOnUiThread()` . BTW, `CheckForIllegalCrossThreadCalls = true` is the default, as is `ConfigureAwait(true)`. – Jimi Aug 03 '21 at 09:30
  • @MickyD I mentioned this in the question. If `mustRunOnUiThreadAsync()` throws an exception, it's caught by the normal windows message loop code (which you would expect, because it's running in the UI thread via the message pump). – Matthew Watson Aug 03 '21 at 09:32
  • @Jimi Actually CheckForIllegalCrossThreadCalls is false by default for WinForms applications (unless you're running under the debugger). – Matthew Watson Aug 03 '21 at 09:34
  • Well, I assumed you were debugging that code :) – Jimi Aug 03 '21 at 09:38
  • @Jimi `Progress` uses `System.Threading.SynchronizationContext` to synchronise the callback. I didn't mention this in my question (I'll go edit it in a minute) but I don't have `System.Threading.SynchronizationContext` available where `notRunningOnUiThread()` is called - all I have at that point is a WinForms `Control`. – Matthew Watson Aug 03 '21 at 09:41
  • You're running your Task from `testBtn_Click`. That's where you create the Progress object. E.g., `var progress = new Progress(); progress.ProgressChanged += ProgressChangedEvent; _ = Task.Run(() => notRunningOnUiThread(progress));`. Redefine `void notRunningOnUiThread(IProgress progress) { ... progress.Report(null); }`. The event handler can be `private async void ProgressChangedEvent(object sender, int? e) { ... await [Something] ... }` -- But you can create a Progress object anywhere else. – Jimi Aug 03 '21 at 09:44
  • @jimi I've updated my question with the appropriate information. A `Progress` has to be created from a UI context otherwise it can't capture the correct `System.Threading.SynchronizationContext` for synchronisation. – Matthew Watson Aug 03 '21 at 09:47
  • That's what I just said and what the *sample code* is about :) Aren't you running the Task from a Button click handler? Code in that handler is executed in the UI Thread. You can create a Progresss object there (as described), or anywhere else in your Form. Then pass it to the method that Task.Run() executes. – Jimi Aug 03 '21 at 09:48
  • @Jimi `notRunningOnUiThread()` is called from RPC in my actual code, so it's not being run from a UI context. This is the fundamental issue. If I had a UI context available this would be easy to solve - but all I have is a `Control` object on which I can call `BeginInvoke()`. – Matthew Watson Aug 03 '21 at 09:54
  • So, do you mean that the code you posted has nothing to do with the actual scenario and `notRunningOnUiThread()` cannot be modified because is nothing like that code implies? I think you should post an example that is quite closer to reality. – Jimi Aug 03 '21 at 10:10
  • @Jimi This is the simplest simulation I can post. The implementation of `notRunningOnUiThread()` can be modified. – Matthew Watson Aug 03 '21 at 10:21
  • I don't think I understand. You can call your method like this: `_ = Task.Run(() => notRunningOnUiThread(this));` but not like this: `_ = Task.Run(() => notRunningOnUiThread(progress));`? When both `this` and the `IProgress`delegate come from the same exact *place* (and SynchronizatinContext)? Assuming `this` is the Form instance and not a random `this` of something else you didn't mention. – Jimi Aug 03 '21 at 10:29
  • @Jimi Sorry if I'm not being clear. My situation is that `notRunningOnUiThread()` is called from an RPC call outside of any `Control`. I only put it in the form implementation for convenience (otherwise the repro would require multiple classes). In the actual code it has a parameter for the `Control` for which to update the UI, and that's all it's got access to. I wanted to post an easily-compilable sample for test purposes. – Matthew Watson Aug 03 '21 at 10:38
  • Since that method can receive the instance of a Control, cannot it receive the instance of a `Progress` object instead? It's still a Field declared in a Form class, as a Control. If not, why that is? This is the missing part that makes the actual context a bit blurry. – Jimi Aug 03 '21 at 10:44
  • @Jimi You must create a `Progress` from a UI thread. Since we are starting from a non-UI thread, this is not possible. The non-UI thread does have a reference to a `Form`, but it's still not running from a UI thread so the context is not available. – Matthew Watson Aug 03 '21 at 11:06
  • As a side note, the `.ConfigureAwait(true)` configuration does essentially nothing, and it can be easily confused with the `.ConfigureAwait(false)` by someone who reads the code in fast pace. My suggestion would be to remove it. – Theodor Zoulias Aug 03 '21 at 12:10
  • @TheodorZoulias If you remove it, you get a Code Analysis warning, `CA2007: Consider calling ConfigureAwait on the awaited task`. Microsoft want us to always explicitly choose which to use - [see here for details](https://learn.microsoft.com/en-gb/dotnet/fundamentals/code-analysis/quality-rules/ca2007?view=vs-2019#how-to-fix-violations). – Matthew Watson Aug 03 '21 at 12:14
  • Oh God! Are these analyzers intended for general application development, or they are targeting extensions for Visual Studio or something? I couldn't emphasize enough how pointless and unproductive it seems to me to add `.ConfigureAwait(true)` before every `await`! – Theodor Zoulias Aug 03 '21 at 12:22

2 Answers2

1

One idea could be to convert the mustRunOnUiThreadAsync from async Task to async void. Conceptually you could say that this method is the "event handler" of an "event" that is originated from the background thread, and so you are not violating Microsoft's guidelines about async void usage essentially. Example:

async void mustRunOnUiThread()
{
    label1.Text = "Before update";
    await Task.Delay(1000).ConfigureAwait(true);
    label1.Text = "After update";
}

...and in the notRunningOnUiThread method:

ui.BeginInvoke(new Action(() => mustRunOnUiThread()));

This modification will not change the behavior of your current code in any way, except from making the VSTHRD101 warning go away.

As a side note you could also consider converting the testBtn_Click handler to async void, so that the notRunningOnUiThread is not launched as a fire-and-forget Task:

async void testBtn_Click(object sender, EventArgs e)
{
    await Task.Run(() => notRunningOnUiThread(this));
}
Theodor Zoulias
  • 34,835
  • 7
  • 69
  • 104
  • This does work, but of course we now get a new warning on the async void method: `VSTHRD100: Avoid "async void" methods, because any exceptions not handled by the method will crash the process.` so it's really only moved the warning around. It's effectively the same warning of course: Possible unobserved exception. But this does at least make the code look more like normal event handling code when using async handlers. – Matthew Watson Aug 03 '21 at 11:16
  • @MatthewWatson ha ha! These analyzers are very smart! You could try to [trick them](https://github.com/Microsoft/vs-threading/issues/469 "VSTHRD100 fails to report async void event handlers") by adding fake (unused) parameters `object sender, EventArgs e` in the `mustRunOnUiThread` method. – Theodor Zoulias Aug 03 '21 at 11:25
  • This is the ~same thing I just described in comments, except you have an async void in a method call here (not a tragedy). Just declare your `Progress` as `Progress progress = null;` then create the object in the Form Constructor or Form.Load: `progress = new Progress(); progress.ProgressChanged += ProgressChangedEvent;`. Setup the event handler and, in `void notRunningOnUiThread()`, add `((IProgress)progress).Report(null);` and all is good. The async event handler can await whatever you want. No warnings. – Jimi Aug 03 '21 at 11:27
  • 1
    I will mark this as the answer in a bit. This transformation has allowed me to understand what's happening to the extent that I believe it's OK to suppress the warning for this particular situation. All the code is running in the UI context which will therefore handle any exceptions (there's no danger of an unobserved exception) so it's safe to suppress the message. – Matthew Watson Aug 03 '21 at 11:27
  • @Jimi I don't have a way to change the code in the UI. I must engineer a way to call the UI methods when I don't have access to anything other than the Form itself and the entry method is being called from a non UI thread, that's the crux of the issue. (I can't change the code in the form - it's third-party.) – Matthew Watson Aug 03 '21 at 11:31
  • Well, that's the missing piece I was asking about. -- The async void method Theodor suggested is not the same as `BeginInvoke(new Action(async () => await mustRunOnUiThreadAsync()));`. – Jimi Aug 03 '21 at 11:45
1

Thanks to Theodor Zoulias's answer, I think that I now understand the warning sufficiently to know that I can safely suppress it.

This is because the entirety of the code being run by the delegate is run in the context of the UI message loop (because of the .BeginInvoke() call) which will report any exceptions.

So this is what I end up with:

using System;
using System.Threading.Tasks;
using System.Windows.Forms;

namespace Net5WinFormsApp
{
    public partial class Form1 : Form
    {
        public Form1()
        {
            InitializeComponent();
            CheckForIllegalCrossThreadCalls = true; // For test purposes to prove that mustRunOnUiThreadAsync() runs on the UI thread.
        }

        void testBtn_Click(object sender, EventArgs e)
        {
            _ = Task.Run(() => notRunningOnUiThread(this)); // Not my real code, but this simulates it.
        }

        static void notRunningOnUiThread(Form1 ui) // Called from a non-UI thread and wants to call a method that updates the UI.
        {
            #pragma warning disable VSTHRD101 // Avoid unsupported async delegates
            ui.BeginInvoke(new Action(async () => await ui.MustRunOnUiThreadAsync().ConfigureAwait(false)));
            #pragma warning restore VSTHRD101 // Avoid unsupported async delegates
        }

        public async Task MustRunOnUiThreadAsync()
        {
            label1.Text = "Before update";
            await Task.Delay(1000).ConfigureAwait(true);
            label1.Text = "After update";
            throw new InvalidOperationException("TEST"); // Check that this is reported.
        }
    }
}

Alternatively, you can change the method to async void:

static void notRunningOnUiThread(Form1 ui) // Called from a non-UI thread and wants to call a method that updates the UI.
{
    ui.BeginInvoke(new Action(ui.MustRunOnUiThread));
}

#pragma warning disable VSTHRD100 // Avoid async void methods
public async void MustRunOnUiThread()
{
    label1.Text = "Before update";
    await Task.Delay(1000).ConfigureAwait(true);
    label1.Text = "After update";
    throw new InvalidOperationException("TEST"); // Check that this is reported.
}
#pragma warning restore VSTHRD100 // Avoid async void methods

This moves the warning to MustRunOnUiThread() so it doesn't help much, although that warning is probably more widely understood compared to VSTHRD101.

Note that the following DOES NOT WORK because it doesn't observe any exception (and it doesn't give a Code Analysis warning either!):

static void notRunningOnUiThread(Form1 ui) // Called from a non-UI thread and wants to call a method that updates the UI.
{
    ui.BeginInvoke(new Action(() => ui.MustRunOnUiThreadAsync().ConfigureAwait(false))); // BAD CODE! Misses exceptions.
}

public async Task MustRunOnUiThreadAsync()
{
    label1.Text = "Before update";
    await Task.Delay(1000).ConfigureAwait(true);
    label1.Text = "After update";
    throw new InvalidOperationException("TEST"); // Check that this is reported.
}

So to summarise:

It's safe to suppress the warning for that particular case because the code that might thrown an exception is run in the context of the UI's message loop, which will report any exceptions.

Matthew Watson
  • 104,400
  • 10
  • 158
  • 276