0

I have this unit test in C#

[Fact]
public async void DownloadAsync_ErrorDuringProgress_ThrowsException()
{
    using var model = SetupModel();
    SetDownload();

    async Task<DownloadStatus> Act()
    {
        return await model.DownloadAsync(VideoUrl, DestFile, (s, e) =>
        {
            e.Download.ProgressUpdated += (s, e) =>
            {
                throw new Exception("BOOM!");
            };
        });
    }

    // await Act();
    await Assert.ThrowsAsync<Exception>(Act);
}

If an exception is thrown in the event handler, then the whole method should throw an exception. (The other option is to handle/ignore the exception and return DownloadStatus.Failed, but then I have to put a generic Catch block that triggers analyzers, and the error in user code is hidden when it should be handled in the event).

For another similar event, it's working as expected, but ProgressUpdated is behaving differently. If I call "await Act();", then an exception is thrown... but it's impossible to catch that exception with ThrowsAsync! The callback is somehow in a different context or something.

It's the System.Progress class that is responsible for triggering ProgressUpdated -- which means that the whole callback piece of code is running in a separate context that doesn't handle exceptions properly.

Looking in the documentation of System.Process, I found this which may be the cause?

Any handler provided to the constructor or event handlers registered with the ProgressChanged event are invoked through a SynchronizationContext instance captured when the instance is constructed. If there is no current SynchronizationContext at the time of construction, the callbacks will be invoked on the ThreadPool.

It's also not clear whether this problem would happen at runtime or only during unit testing.

Anyone understands what's happening, and how to deal with a situation like this? I feel like there's something for me to learn here.

Edit: Here's the code where the context switching would occur

private async Task DownloadFileAsync(DownloadTaskFile fileInfo)
{
    Status = DownloadStatus.Downloading;
    using var cancelToken = new CancellationTokenSource();

    try
    {
        await _youTube.DownloadAsync(
            (IStreamInfo)fileInfo.Stream, fileInfo.Destination, new Progress<double>(ProgressHandler), cancelToken.Token).ConfigureAwait(false);

        void ProgressHandler(double percent)
        {
            fileInfo.Downloaded = (long)(fileInfo.Length * percent);
            UpdateProgress();

            if (IsCancelled)
            {
                try
                {
                    cancelToken.Cancel();
                }
                catch (ObjectDisposedException) { } // In case task is already done.
            }
        }
    }
    catch (HttpRequestException) { Status = DownloadStatus.Failed; }
    catch (TaskCanceledException) { Status = DownloadStatus.Failed; }
}

Where else did I hear about this unhandled errors problem? "async void". This event handler is not async BUT it is being run async.

Etienne Charland
  • 3,424
  • 5
  • 28
  • 58
  • Okay so there's this wall of text here but no concrete question.. What do you want? – sommmen Apr 27 '20 at 20:25
  • Make the test work. I must be able to intercept the exception. – Etienne Charland Apr 27 '20 at 23:34
  • and you're sure the event handler gets hit? could you break at `throw new exception` and make sure the code flows to there? – sommmen Apr 28 '20 at 07:24
  • and you're not swallowing any exceptions in your download function or in your event invocator? – sommmen Apr 28 '20 at 07:26
  • perhaps post the code to DownloadAsync? – sommmen Apr 28 '20 at 07:27
  • Also see this; https://stackoverflow.com/a/21269145/4122889 – sommmen Apr 28 '20 at 07:39
  • I "could" try to reproduce it in a more simple setting, but the DownloadAsync class relies on a few other classes right now. The Assert.ThrowsAsync definitely gets hit because it says that the expected exception was not thrown; and if I remove it, it says that the exception occurerd -- so it's occuring in a different scope. I'm able to handle it on ProgressUpdated?.Invoke and set DownloadStatus to Failed from there; but if not, I can't handle it from the main method. The main method returns only once the download is completed and returns the download status (success, failed, cancelled). – Etienne Charland Apr 28 '20 at 16:19
  • I've added the code where the context-switching seems to occur. This looks like a "async void" problem where an async task exception is being discarded. – Etienne Charland Apr 28 '20 at 16:27

1 Answers1

2

Progress<T> does capture the current SynchronizationContext when it is created, and it runs its event handler on that context. The handler for Progress<T> is logically an event handler, and is treated as a top-level event handler.

This means that exceptions that propagate out of the progress handler cannot be caught. They always run directly on the context.

The best fix is probably to adjust your API to take an IProgress<T>, which is the standard pattern. Progress<T> is not the only IProgress<T> implementation, and indeed it sounds like you might want an IProgress<T> implementation that executes progress handlers directly.

Stephen Cleary
  • 437,863
  • 77
  • 675
  • 810