1

I'm toying around with C# and trying to practice some WinForms with async code. I have the following code, but when I click the button and initiate doTheThingAsync and I close the window while the task is still processing then Visual Studio catches a NullReferenceException from the point where I access toolStripProgressBar1.Value.

private async Task doTheThingAsync(IProgress<int> progress)
{
    await Task.Run(() =>
    {
        for (int i = 1; i <= 1000; ++i)
        {
            Thread.Sleep(10);
            progress.Report(i);
        }
        Thread.Sleep(2000);
    });
}

private async void button1_Click(object sender, EventArgs e)
{
    var progress = new Progress<int>(i => toolStripProgressBar1.Value = i);
    await doTheThingAsync(progress);
    toolStripProgressBar1.Value = 0;
}

Not sure why this is happening, but I figure if I cancel the task before the Form closes then all will be well, so I modify it.

private async Task doTheThingAsync(IProgress<int> progress, CancellationToken ct)
{
    await Task.Run(() =>
    {
        for (int i = 1; i <= 1000; ++i)
        {
            if (ct.IsCancellationRequested) return;
            Thread.Sleep(10);
            progress.Report(i);
        }
        Thread.Sleep(2000);
    });
}

private async void button1_Click(object sender, EventArgs e)
{
    var progress = new Progress<int>(i => toolStripProgressBar1.Value = i);
    var cts = new CancellationTokenSource();
    FormClosed += (s, ev) => cts.Cancel();
    await doTheThingAsync(progress, cts.Token);
    toolStripProgressBar1.Value = 0;
}

No dice. I still get the exception. So out of desperation I try wrapping it in a try/catch.

var progress = new Progress<int>(i => {
    try {
        toolStripProgressBar1.Value = i;
    } catch {}
});

But even then the uncaught NullReferenceException persists, and I am out of ideas.

System.NullReferenceException
    HResult=0x80004003
    Message=Object reference not set to an instance of an object.
    Source=System.Windows.Forms
    StackTrace:
    at System.Windows.Forms.ToolStripProgressBar.set_Value(Int32 value)
    at Test.Form1.<>c__DisplayClass2_0.<button1_Click>b__0(Int32 i) in C:\Users\Chris\source\repos\Test\Test\Form1.cs:line 37

Edit: The NullReferenceException seems to come from the fact that ToolStripProgressBar.Value has a { get, set } that accesses a reference that is nulled due to the fact that the control has already been disposed at the time of access.

Fildor's request:

private Task doTheThingAsync(IProgress<int> progress, CancellationToken ct)
{
    return Task.Run(() =>
    {
        for (int i = 1; i <= 1000; ++i)
        {
            ct.ThrowIfCancellationRequested(); //VS breaks here with unhandled exception
            Thread.Sleep(10);
            progress.Report(i);
        }
        Thread.Sleep(2000);
    });
}

private async void button1_Click(object sender, EventArgs e)
{
    var progress = new Progress<int>(i => {
        toolStripProgressBar1.Value = i;
    });
    var cts = new CancellationTokenSource();
    FormClosed += (s, ev) => cts.Cancel();
    try
    {
        await doTheThingAsync(progress, cts.Token);
        toolStripProgressBar1.Value = 0;
    }
    catch (OperationCanceledException) { }
}

Edit: The issues with the above code is that is should have been TaskCanceledException not OperationCanceledException.

Update: This is the only way I have been able to get this to work such that I can close the form without getting an unhandled exception. Note that I am having to always check IsDisposed before accesing Value on the ToolStripProgressBar from within the IProgress action. I can't say for sure why but this seems absurd to me.

    private Task doTheThingAsync(IProgress<int> progress, CancellationToken ct)
    {
        return Task.Run(async () =>
        {
            for (int i = 1; i <= 1000; ++i)
            {
                ct.ThrowIfCancellationRequested();
                await Task.Delay(10, ct);
                progress.Report(i);
            }
            await Task.Delay(2000, ct);
        });
    }

    private async void button1_Click(object sender, EventArgs e)
    {
        var progress = new Progress<int>(i => {
            if (!toolStripProgressBar1.IsDisposed) toolStripProgressBar1.Value = i;
        });
        var cts = new CancellationTokenSource();
        //FormClosed += (s, ev) => cts.Cancel(); //no longer actually necessary...
        try
        {
            await doTheThingAsync(progress, cts.Token);
            toolStripProgressBar1.Value = 0;
        }
        catch {}
    }
Chris_F
  • 4,991
  • 5
  • 33
  • 63
  • 1
    Look at the exception pop-up window. Is this a "first-chance exception"? If so, VS has broken at the point that the exception is thrown, regardless of the fact that it might be caught later. So if you hit "Continue", it might well end up inside that catch statement. That said, it would be better to check for cancellation inside the `Progress` callback I think -- that'll be thread-safe, and should avoid the problem entirely – canton7 May 05 '22 at 10:12
  • Unrelated: Please do not use `Thread.Sleep()` in here. If you absolutely need to Delay use `await Task.Delay` – Fildor May 05 '22 at 10:17
  • 1
    @Fildor I'm assuming that's because OP put together a good [mcve] -- those aren't always realistic, but this does nicely demonstrate the problem without unnecessary code – canton7 May 05 '22 at 10:18
  • 1
    Ah, from the stack trace it looks like it's the `toolStripProgressBar1.Value = 0;` line at the end of the method which is throwing, not the one in the `Progress` callback? You didn't wrap that one in a try/catch – canton7 May 05 '22 at 10:19
  • @canton7 Yep, on second look, it seems to be the case. That just was an immediate splinter in the eye. – Fildor May 05 '22 at 10:20
  • 1
    Note that it's common to use `ct.ThrowIfCancellationRequested()` to throw an `OperationCanceledException` if cancellation has occurred, then the caller wraps a `try { ... } catch (OperationCanceledException) { }` around the stuff that should be cancelled. This means you can do `try { await doTheThingAsync(progress, cts.Token); toolStripProgressBar1.Value = 0; } ...`, and the last access of `toolStripProgressBar1.Value` only happens if cancellation hasn't happened – canton7 May 05 '22 at 10:21
  • @canton7 Now I'm just getting an uncaught `OperationCanceledException` instead despite the try/catch. I've become far to comfortable with JavaScript promises. I really need an example. – Chris_F May 05 '22 at 10:33
  • @Chris_F, you should wrap the `await doTheThingAsync(...)` line with `try/catch` to handle `OperationCanceledException`: `try{await doTheThingAsync(...);}catch(OperationCanceledException){/*do nothing*/}` – Serg May 05 '22 at 10:47
  • @Serg I did that. It did not work. – Chris_F May 05 '22 at 10:51
  • Add your new code to the question. – Fildor May 05 '22 at 10:51
  • Updated question. I can't find any solution other than testing `IsDisposed` before every access of `Value`. – Chris_F May 05 '22 at 11:48
  • 1
    Also note that it should have been `TaskCanceledException` not `OperationCanceledException`. – Chris_F May 05 '22 at 11:52
  • Why not prevent the form to close in case the asyn method is still working? This will be very simple. You can still add a button to cancel in order to let the user to close even if the method is still executing – Marco Beninca May 05 '22 at 12:20
  • @Chris_F `TaskCanceledException` inherits from `OperationCanceledException`, so catching `OperationCanceledException` is fine. Note also that in some weird cases (and I don't remember what exactly) you'll get an `OperationCanceledException` even if you were expecting a `TaskCanceledException`, so always trying to catch the base class `OperationCanceledException` is safer – canton7 May 05 '22 at 12:58
  • Hmm, looks like `CancellationToken.ThrowIfCancelationRequested` [throws an `OperationCanceledException`](https://source.dot.net/#System.Private.CoreLib/CancellationToken.cs,359). So no, don't try and catch `TaskCanceledException` – canton7 May 05 '22 at 12:59
  • You'll probably have the same problem if you try to change the `toolStripProgressBar1.Value` inside the async event handler of a button click, after an `await Task.Delay(5000);` or something. See [this](https://stackoverflow.com/questions/54128681/what-happens-when-the-user-closes-a-form-performing-an-async-task-in-an-async-ev "What happens when the user closes a form performing an async task in an async event handler?") for example. – Theodor Zoulias May 05 '22 at 14:11
  • if (!toolStripProgressBar1.IsDisposed) toolStripProgressBar1.Value = i; – Hans Passant May 05 '22 at 14:58
  • @HansPassant I think that's a Band-Aid. The issue is that 'IProgress' is being invoked after the Form has already been disposed. It just happens that `ToolStripProgressBar` is a control type that blows up when accessing it post-disposal, but accessing *any* control would be undefined behavior even if it doesn't throw and exception. I don't see anywhere in documentation or example where you are told to test `IsDisposed` before every control access from within an IProgress or async function. – Chris_F May 05 '22 at 16:00
  • Threading race bugs often require band-aids. You can certainly make it more convoluted, just use the FormClosing event to prevent the form from closing while the task is still running. Same idea as https://stackoverflow.com/a/1732361/17034 – Hans Passant May 05 '22 at 16:12

1 Answers1

0

Edit Changed from label to report result to a ToolStripProgressBar which as stated from others needs to check _cts.IsCancellationRequested

Here is a code sample to try out which will not throw an exception on closing the form/app while a do nothing async method is running.

Starts off in a button named StartButton, first checks if the CancellationTokenSource needs to be recreated if used already, performs a do nothing method using await Task.Delay(500, ct) rather than Thread.Sleep which has already been pointed out to avoid.

In the do nothing method AsyncMethod we test if there is a cancel request which goes back to the start button code which handles the request in a try/catch.

Cancellation is done in a button named CancelButton.

Progress is done via setting value property of a ToolStripProgressBar.

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

namespace AsyncSimple
{
    public partial class Form1 : Form
    {
        private CancellationTokenSource _cts = new ();
        public Form1()
        {
            InitializeComponent();
        }

        private async void StartButton_Click(object sender, EventArgs e)
        {
            if (_cts.IsCancellationRequested)
            {
                _cts.Dispose();
                _cts = new CancellationTokenSource();
            }

            var progressIndicator = new Progress<int>(ReportProgress);

            try
            {
                await AsyncMethod(progressIndicator, _cts.Token);
            }
            catch (OperationCanceledException)
            {
                // do nothing
            }
        }

        private void CancelButton_Click(object sender, EventArgs egEventArgs)
        {
            _cts.Cancel();
        }
        private static async Task AsyncMethod(IProgress<int> progress, CancellationToken ct)
        {

            for (int index = 1; index <= 100; index++)
            {
                //Simulate an async call that takes some time to complete
                await Task.Delay(500, ct);

                progress?.Report(index);

            }

        }

        private void ReportProgress(int value)
        {
            if (_cts.IsCancellationRequested)
            {
                return;
            }
            else
            {
                toolStripProgressBar1.Value = value;
            }
        }
    }
}
Karen Payne
  • 4,341
  • 2
  • 14
  • 31
  • @canton7 agree and took the check out – Karen Payne May 05 '22 at 11:19
  • This doesn't subscribe to the `FormClosed` event to trigger cancellation, and that's the crux of the issue. Your `StatusLabel.Text = "Cancelled"` line will fail in this case, because `StatusLabel` will become null. That's the issue OP's hitting – canton7 May 05 '22 at 11:22
  • Your code produces the same problem. If you want to test this for yourself I recommend you actually include a ToolStripProgressBar in the formula since I think this is critical. – Chris_F May 05 '22 at 11:26
  • For instance, this issue does not occur if the `ToolStripProgressBar` is replaced with a `ProgressBar`. – Chris_F May 05 '22 at 11:58
  • Let's see what I can come up with in a few minutes – Karen Payne May 05 '22 at 12:02