Proper way to deal with exceptions in AsyncDispose

Question

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.

Answer 1

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;
}

Answer 2

Maybe you already understand why this happens, but it's worth spelling out. This behaviour isn't specific to await using. It would happen with a plain using block too. So while I say Dispose() here, it all applies to DisposeAsync() too.

A using block is just syntactical sugar for a try/finally block, as the remarks section of the documentation says. What you see happens because the finally block always runs, even after an exception. So if an exception happens, and there is no catch block, the exception is put on hold until the finally block runs, and then the exception is thrown. But if an exception happens in finally, you will never see the old exception.

You can see this with this example:

try {
    throw new Exception("Inside try");
} finally {
    throw new Exception("Inside finally");
}

It doesn't matter whether Dispose() or DisposeAsync() is called inside the finally. The behaviour is the same.

My first thought is: don't throw in Dispose(). But after reviewing some of Microsoft's own code, I think it depends.

Take a look at their implementation of FileStream, for example. Both the synchronous Dispose() method, and DisposeAsync() can actually throw exceptions. The synchronous Dispose() does ignore some exceptions intentionally, but not all.

But I think it's important to take into account the nature of your class. In a FileStream, for example, Dispose() will flush the buffer to the file system. That is a very important task and you need to know if that failed. You can't just ignore that.

However, in other types of objects, when you call Dispose(), you truly have no use for the object anymore. Calling Dispose() really just means "this object is dead to me". Maybe it cleans up some allocated memory, but failing doesn't affect the operation of your application in any way. In that case, you might decide to ignore the exception inside your Dispose().

But in any case, if you want to distinguish between an exception inside the using or an exception that came from Dispose(), then you need a try/catch block both inside and outside of your using block:

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

Or you could just not use using. Write out a try/catch/finally block yourself, where you catch any exception in finally:

var d = new D();
try
{
    throw new ArgumentException("I'm inside try");
}
catch (Exception e)
{
    Console.WriteLine(e.Message); // prints I'm inside try
}
finally
{
    try
    {
        if (D != null) await D.DisposeAsync();
    }
    catch (Exception e)
    {
        Console.WriteLine(e.Message); // prints I'm inside dispose
    }
}