Should DisposeAsync throw background task exceptions, or leave it to the client to observe explicitly?

Question

I don't think this question is a duplicate of "Proper way to deal with exceptions in DisposeAsync".

Let's say my class that implements IAsynsDisposable because it has a long-running background task, and DisposeAsync terminates that task. A familiar pattern might be the Completion property, e.g. ChannelReader<T>.Completion (despite ChannelReader doesn't implement IAsynsDisposable).

Is it considered a good practice to propagate the Completion task's exceptions outside DisposeAsync?

Here is a complete example that can be copied/pasted into a dotnet new console project. Note await this.Completion inside DisposeAsync:

try
{
    await using var service = new BackgroundService(TimeSpan.FromSeconds(2));
    await Task.Delay(TimeSpan.FromSeconds(3));
}
catch (Exception ex)
{
    Console.WriteLine(ex);
    Console.ReadLine();
}

class BackgroundService: IAsyncDisposable
{
    public Task Completion { get; }

    private CancellationTokenSource _diposalCts = new();

    public BackgroundService(TimeSpan timeSpan)
    {
        this.Completion = Run(timeSpan);
    }

    public async ValueTask DisposeAsync()
    {
        _diposalCts.Cancel();
        try
        {
            await this.Completion;
        }
        finally
        {
            _diposalCts.Dispose();
        }
    }

    private async Task Run(TimeSpan timeSpan)
    {
        try
        {
            await Task.Delay(timeSpan, _diposalCts.Token);
            throw new InvalidOperationException("Boo!");
        }
        catch (OperationCanceledException)
        {
        }
    }
}

Alternatively, I can observe service.Completion explicitly in the client code (and ignore its exceptions inside DiposeAsync to avoid them being potentially thrown twice), like below:

try
{
    await using var service = new BackgroundService(TimeSpan.FromSeconds(2));
    await Task.Delay(TimeSpan.FromSeconds(3));
    await service.Completion;
}
catch (Exception ex)
{
    Console.WriteLine(ex);
    Console.ReadLine();
}

class BackgroundService: IAsyncDisposable
{
    public Task Completion { get; }

    private CancellationTokenSource _diposalCts = new();

    public BackgroundService(TimeSpan timeSpan)
    {
        this.Completion = Run(timeSpan);
    }

    public async ValueTask DisposeAsync()
    {
        _diposalCts.Cancel();
        try
        {
            await this.Completion;
        }
        catch
        {
            // the client should observe this.Completion
        }
        finally
        {
            _diposalCts.Dispose();
        }
    }

    private async Task Run(TimeSpan timeSpan)
    {
        try
        {
            await Task.Delay(timeSpan, _diposalCts.Token);
            throw new InvalidOperationException("Boo!");
        }
        catch (OperationCanceledException)
        {
        }
    }
}

Is there a concensus about which option is better?

In first approach, you want wait for background task to complete, if using scope has finished, the background task will be cancelled if it won't finish by that time. In the second case, you will await background task. THose two approaches are very different functionally, so I would not recommend one over another, I'd say whatever works for you and what is your use-case. — Michał Turczyn, Jan 18 '22 at 11:00
@MichałTurczyn, by "very different", do you mean that if I expose `service.Completion`, then maybe I should not even implement `IAsyncDisposable` and let the client track the completion status and any errors? Tks for the discussion! :) — noseratio, Jan 18 '22 at 11:28
I would say so. If client invokes background task, then yeah. If I were a client, i would implement Disposable pattern and complete (or not) background tasks that I invoked. — Michał Turczyn, Jan 18 '22 at 11:32
Yes, your question is close to the referenced one. IMHO, it should be up to you and your requirements. It could be incapsulated to have clearer code, have less control in logic. Or it could be processed in logic with less clarity but have more control. More text in the accepted answer https://stackoverflow.com/a/59185614/6344916. — PIoneer_2, Jan 18 '22 at 12:12
@noseratio QQ: What do you want to do with that exception? You have already disposed your long-running task when you are observing the exception. — Peter Csala, Jan 21 '22 at 08:27
@PeterCsala, I'd say I'd have disposed the resources used by the task when I called `AsyncDispose`, but I'd probably still be interested in how the task itself had ended? So at this point I like more the second approach, where `DisposeAsync` should await the task but ignore its exceptions (before the cleanup), while the client code should observe the `Completion` property. — noseratio, Jan 21 '22 at 08:41
@noseratio For me the same. It is more intuitive to receive the exception via the `Completion`. But the main drawback is that you can't enforce the consumer of your class to call `await service.Completion;` , which can lead to swallowing exception. The consumer might think that the operation has succeeded even though it's not. — Peter Csala, Jan 21 '22 at 09:25

noseratio · Answer 1 · 2022-01-20T01:16:07.507

For now, I've settled on a reusable helper class LongRunningAsyncDisposable (here's a gist, warning: barely tested yet), which allows:

to start a background task;
stop this task (via a cancellation token) by calling IAsyncDisposable.DisposeAsync at any time, in a thread-safe, concurrency-friendly way;
configure whether DisposeAsync should re-throw the task's exceptions (DisposeAsync will await the task's completion either way, before doing a cleanup);
observe the task's status, result and exceptions at any time via LongRunningAsyncDisposable.Completion property.

Should DisposeAsync throw background task exceptions, or leave it to the client to observe explicitly?

1 Answers1