Proper way to deal with exceptions in DisposeAsync

During switching to the new .NET Core 3's IAsynsDisposable, I've stumbled upon the following problem.

The core of the problem: if DisposeAsync throws an exception, this exception hides any exceptions thrown inside await using-block.

class Program 
{
    static async Task Main()
    {
        try
        {
            await using (var d = new D())
            {
                throw new ArgumentException("I'm inside using");
            }
        }
        catch (Exception e)
        {
            Console.WriteLine(e.Message); // prints I'm inside dispose
        }
    }
}

class D : IAsyncDisposable
{
    public async ValueTask DisposeAsync()
    {
        await Task.Delay(1);
        throw new Exception("I'm inside dispose");
    }
}

What is getting caught is the DisposeAsync-exception if it's thrown, and the exception from inside await using only if DisposeAsync doesn't throw.

I would however prefer it other way round: getting the exception from await using block if possible, and DisposeAsync-exception only if the await using block finished successfully.

Rationale: Imagine that my class D works with some network resources and subscribes for some notifications remote. The code inside await using can do something wrong and fail the communication channel, after that the code in Dispose which tries to gracefully close the communication (e. g., unsubscribe from the notifications) would fail, too. But the first exception gives me the real information about the problem, and the second one is just a secondary problem.

In the other case when the main part ran through and the disposal failed, the real problem is inside DisposeAsync, so the exception from DisposeAsync is the relevant one. This means that just suppressing all exceptions inside DisposeAsync shouldn't be a good idea.

I know that there is the same problem with non-async case: exception in finally overrides the exception in try, that's why it's not recommended to throw in Dispose(). But with network-accessing classes suppressing exceptions in closing methods doesn't look good at all.

It's possible to work around the problem with the following helper:

static class AsyncTools
{
    public static async Task UsingAsync<T>(this T disposable, Func<T, Task> task)
            where T : IAsyncDisposable
    {
        bool trySucceeded = false;
        try
        {
            await task(disposable);
            trySucceeded = true;
        }
        finally
        {
            if (trySucceeded)
                await disposable.DisposeAsync();
            else // must suppress exceptions
                try { await disposable.DisposeAsync(); } catch { }
        }
    }
}

and use it like

await new D().UsingAsync(d =>
{
    throw new ArgumentException("I'm inside using");
});

which is kind of ugly (and disallows things like early returns inside the using block).

Is there a good, canonical solution, with await using if possible? My search in internet didn't find even discussing this problem.

Solution

There are exceptions that you want to surface (interrupt the current request, or bring down the process), and there are exceptions that your design expects will occur sometimes and you can handle them (e.g. retry and continue).

But distinguishing between these two types is up to the ultimate caller of the code - this is the whole point of exceptions, to leave the decision up to the caller.

Sometimes the caller will place greater priority on surfacing the exception from the original code block, and sometimes the exception from the Dispose. There is no general rule for deciding which should take priority. The CLR is at least consistent (as you've noted) between the sync and non-async behaviour.

It's perhaps unfortunate that now we have AggregateException to represent multiple exceptions, it can't be retrofitted to solve this. i.e. if an exception is already in flight, and another is thrown, they are combined into an AggregateException. The catch mechanism could be modified so that if you write catch (MyException) then it will catch any AggregateException that includes an exception of type MyException. There are various other complications stemming from this idea though, and it's probably too risky to modify something so fundamental now.

You could improve your UsingAsync to support early return of a value:

public static async Task<R> UsingAsync<T, R>(this T disposable, Func<T, Task<R>> task)
        where T : IAsyncDisposable
{
    bool trySucceeded = false;
    R result;
    try
    {
        result = await task(disposable);
        trySucceeded = true;
    }
    finally
    {
        if (trySucceeded)
            await disposable.DisposeAsync();
        else // must suppress exceptions
            try { await disposable.DisposeAsync(); } catch { }
    }
    return result;
}