Proposal: Implement IAsyncDisposable on various BCL types

[stephentoub]

Related to #27547.

Now that we’re adding System.IAsyncDisposable to the core libraries, we also want to implement the interface on a variety of types that currently do or have the potential to do asynchronous work as part of their disposal. This is primarily focused on System.IO types that might flush or otherwise perform I/O as part of their clean up (e.g. flushing a buffer), though it is not limited to such types.

The following types would all implement IAsyncDisposable, and all gain a public member:

public ValueTask DisposeAsync();

with it being virtual on all of the non-sealed classes:

public virtual ValueTask DisposeAsync();

• System.IO.Stream. Its DisposeAsync will do the equivalent of Task.Run(Dispose). Then, our various Stream-derived types will provide a more specialized implementation where appropriate. For example, MemoryStream’s Dispose is a nop, so we’ll make its DisposeAsync a nop as well (unless the instance is actually of a type derived from MemoryStream, in which case we’ll delegate to the base implementation). Conversely, FileStream’s Dispose does a flush, so its DisposeAsync will do an asynchronous flush.
• System.IO.BinaryWriter. Its implementation will check to see whether the instance is a concrete BinaryWriter or something derived from it. If concrete, the DisposeAsync will effectively be a copy of the synchronous Dispose, except using async equivalents on the underlying Stream, e.g. where the synchronous implementation calls Flush or Dispose/Close, the asynchronous implementation would use FlushAsync/DisposeAsync. If instead the type is derived from BinaryWriter, the implementation will simply do the equivalent of Task.Run(Dispose), so as to pick up whatever Dispose implementation the derived class is providing, and the derived class may then choose to override DisposeAsync to provide a better implementation if applicable (the core libraries don’t provide any derived types).
• System.IO.TextWriter. Its implementations will do the equivalent of Task.Run(Dispose). Derived implementations can then do something better if appropriate. For example, we’ll override on StreamWriter to asynchronously flush.
• System.Threading.Timer. Timer currently provides two Dispose methods, Dispose() and Dispose(WaitHandle), the latter of which not only stops the timer, but also signals the provided WaitHandle when the timer guarantees that no more callbacks associated with that timer will be invoked. A caller can then block on this WaitHandle to know when it’s safe to progress with any state that Timer may have interacted with. As such, we’ll provide an equivalent DisposeAsync, where the returned ValueTask will complete when the timer appropriately guarantees the same thing, allowing a caller to wait asynchronously instead of synchronously.
• System.Threading.CancellationTokenRegistration. CancellationTokenRegistration.Dispose does two things: it unregisters the callback, and then it blocks until the callback has completed if the callback is currently running. DisposeAsync will do the same thing, but allow for that waiting to be done asynchronously rather than synchronously.

Open issues:

BinaryReader/TextReader. I listed BinaryWriter and TextWriter, but not BinaryReader and TextReader. Do we want to implement IAsyncDisposable on those as well, as with their writing counterparts? It’s rare for an implementation to actually need to do asynchronous work as part of closing a reader, as they generally don't need to flush. We could add them now for completeness, or we could wait until there's a demonstrated need.

Stream.DisposeAsync. There are some unfortunate issues here. I see three main options:

1. Implement IAsyncDisposable as a public virtual.
2. Don't implement the interface on Stream, but implement it on derived types.
3. Don't implement it on any Stream, and just have callers use FlushAsync + Dispose.

(2) isn't a very good for a few reasons:

• To use using with an object, the compiler needs to be able to see statically that it implements IDisposable; similarly for IAsyncDisposable with await using. It's very common to just have a Stream reference that you get handed from somewhere, and so even if the derived implementation implements the interface, you wouldn't be able to use await using with it as a Stream.
• If a BaseStream implements the interface explicitly, then there's no good way for a DerivedStream : BaseStream to invoke the base stream's implementation in order to clean up any resources the base stream owned.
• Call sites need to type test for IAsyncDisposable to know whether they can use DisposeAsync and then fall back to using Dispose if DisposeAsync isn't available.

(1) is what I've implemented, but it has its own problems. First, when we add DisposeAsync to Stream, it has to invoke the existing Dispose, as otherwise when code started using await using with a stream instead of using, it wouldn't actually clean anything up until the owner of that stream released a new version that overrode the new method. Second, since Dispose could be doing anything, including I/O, we don't want to synchronously block the caller who just asked to do work asynchronously, so DisposeAsync really needs to queue the call to Dispose. Now, it's very common for a derived Stream's Dispose(bool) override to do some cleanup and then call base.Dispose(disposing) in order to do whatever further cleanup work its base has (which may be an intermediary stream rather than Stream itself). However, if you do that with DisposeAsync and get down to the base Stream.DisposeAsync method, you'll end up queueing a work item to invoke Dispose... this shouldn't hurt anything functionally, as Dispose is meant to be idempotent, but it's unnecessary work. We could say "if you derive directly from Stream, don't bother calling to base.Dispose(disposing)", but that doesn't work, either. Consider a type like FileStream. FileStreamhas its own cleanup to do, so it needs to overrideDisposeAsync. However, what about an existing MySpecializedFileStreamthat derives fromFileStreamand overridesDisposeto cleanup additional stuff and then call toFileStream's implementation. That FileStream-derived type won't have overridden DisposeAsyncyet, which meansFileStream's DisposeAsyncactually needs to type test whether the instance is a concreteFileStreamor something derived, and if derived, it should just usebase.DisposeAsync()instead of its async logic. That then makes thebaseinvocation problem transitive, as whenMySpecializedFileStreamgoes to overrideDisposeAsync, if it calls base.DisposeAsync(), it'll end up going all the way down to Stream.DisposeAsync, which will queue a call to Dispose`.

This makes me wonder whether we should just do (3). But not implementing IAsyncDisposable on a type like Stream makes me question IAsyncDisposable.

Similar problems apply to TextWriter as well.

A few notes:

• Shouldn’t every type that implements IDisposable also implement IAsyncDisposable? No. The vast majority of IDisposable types do not perform asynchronous work as part of their disposal, with most dispose routines primarily focused on releasing native resources (often via calling Dispose on SafeHandles) and other such synchronous operations. We only want to implement IAsyncDisposable when we know that the type has the strong potential to do asynchronous I/O that would otherwise force its synchronous Dispose to block or spin waiting for those operations to complete.
• Don’t we need a DisposeAsync(bool disposing)? No. The synchronous pattern has this so that both Dispose and a finalizer can share a dispose implementation, with the former passing true and the latter passing false, and then the implementation generally not disposing other managed state in the case of it being a finalizer. With DisposeAsync, the benefit of the method is to the caller being able to await for the disposal to complete rather than being blocked implicitly; this is not relevant to a finalizer, which will not wait for any such work, synchronously or asynchronously.

[GSPP]

What's the guidance on throwing exceptions? Async disposal will often involve IO.

[stephentoub]

What's the guidance on throwing exceptions? Async disposal will often involve IO.

It should be the same as for Dispose, for the same reasons plus consistency with Dispose. Dispose also often involves I/O.

[muratg]

How about Brotli and Deflate compression?

We have asks like this: aspnet/BasicMiddleware#247 and also lots of other issues (like threadpool exhaustion, and other subtle bugs)

[muratg]

cc @Tratcher

[stephentoub]

How about Brotli and Deflate compression?

That's covered by Stream and "Then, our various Stream-derived types will provide a more specialized implementation where appropriate".

[benaadams]

Stream.DisposeAsync. There are some unfortunate issues here...

Add extension method?

public static ValueTask DisposeAsync(this Stream stream)
{
    if (stream is IAsyncDisposable ad)
    {
        return ad.DisposeAsync();
    }

    return Task.Run(() => stream.Dispose());
} 

Would be overridden if derived type implements it and is referred to directly (like FileStream); or can be cast to interface if higher base type is used (like Stream) via the extension method (for example FileStream) through the interface cast.

[stephentoub]

Add extension method?

Even if await using is made to work with extension methods (using today does not), how does that work when a StreamA derived from StreamB implements IAsyncDisposable.DisposeAsync and then needs to call to StreamB's base implementation if it implements it? Seems like you could easily end up in an infinite loop or stack dive.

We spoke in the meeting today about an alternative where we expose a protected virtual FlushFinalAsync; the base Stream.DisposeAsync would be non-virtual, and would await FlushFinalAsync then call Dispose(), the idea being that FlushFinalAsync would do all of the asynchronous work, such that the synchronous Dispose wouldn't have to do any I/O. At first that seemed like a good idea, but as I've gone through to implement it, I'm not sure it's actually any better than the previous solution. FlushFinalAsync will end up flushing such that subsequent flushes should be a nop, but if it doesn't dispose the stream, then the subsequent call to Dispose will have to invoke flush again (so two flushes instead of two disposes), and if it does dispose the stream, then it's no different than the two disposes. Plus, having the method as protected causes issues for wrappers like Streamwriter, whose equivalent FlushFinalAsync wouldn't have access to the protected virtual on Stream, and thus would be forced to use DisposeAsync in its FlushFinalAsync, which would then cause problems for its Dispose that would try to flush a disposed stream.

This all makes me wonder whether the best solution is just a small variation of what I currently have up in the PR: have the base public virtual DisposeAsync on Stream just synchronously invoke Dispose. Yes, in some cases it'll end up doing I/O as part of DisposeAsync, but it'll generally be only a small amount of I/O, especially if the consuming code has already flushed (in which case it'll generally be no I/O)… plus it's not really any worse than the only option code has today. And a derived implementation can then just delegate to base; it already needs to be able to handle Dispose being called any number of times, so we'll incur potentially an extra Dispose invocation, but that should return immediately with minimal fanfare, and by having the invocation be synchronous, we don't have to pay for a queued work item in that case.

[GSPP]

If there is any additional flushing done it should not force disk access. Rather, it should just empty internal .NET buffers. This is something to keep in mind.

[stephentoub]

If there is any additional flushing done it should not force disk access.

That would of course be the case in any implementations we can control.

[pentp]

Synchronously invoking Dispose in Stream.DisposeAsync looks like the best alternative.
If one day C# will support address-of for functions to detect overrides (dotnet/csharplang#191 (comment)) then it could be possible to skip the Dispose call even for derived classes.

[ahsonkhan]

It doesn't look like we updated the ref assembly to match the type definition (i.e. types now implement IAsyncDisposable but the ref doesn't show it).

For example dotnet/corefx#33410 exposed the DisposeAsync method but didn't update the type signature

• https://github.com/dotnet/coreclr/blob/57fd77e6f8f7f2c37cc5c3b36df3ea4f302e143b/src/System.Private.CoreLib/shared/System/IO/BinaryWriter.cs#L16
• https://github.com/dotnet/corefx/blob/eec001d96a68376c0e504eb7635c8edec196f90f/src/System.Runtime.Extensions/ref/System.Runtime.Extensions.cs#L1314

Is this intentional? It doesn't look like we expose IAsyncDisposable publicly in any contract either.

[stephentoub]

It doesn't look like we expose IAsyncDisposable publicly in any contract either.

Yes, we do:
https://github.com/dotnet/corefx/blob/90e11f77895a5c5363dd818b5c89c6f811eef2e6/src/System.Runtime/ref/System.Runtime.cs#L1394-L1397

Is this intentional?

No, that's an oversight on my part. Will fix. Apparently I added the DisposeAsync methods in the refs but didn't actually mark the types as implementing the interface.