[API Proposal]: Implement IAsyncDisposable on TextReader

[Neme12]

Background and motivation

By default, when disposing a StreamReader or StreamWriter, the underlying Stream itself is disposed. Since Stream supports async disposal via IAsyncDisposable, both StreamReader and StreamWriter should implement IAsyncDisposable as well to support the case when the underlying stream supports it. Currently, only StreamWriter implements IAsyncDisposable while StreamReader doesn't, so when using StreamReader, it's only possible to dispose the underlying stream synchronously.

API Proposal

namespace System.IO;

// new
public abstract partial class TextReader : MarshalByRefObject, IDisposable, IAsyncDisposable
{
    public virtual ValueTask DisposeAsync();
}

// existing
public abstract partial class TextWriter : MarshalByRefObject, IDisposable, IAsyncDisposable
{
    public virtual ValueTask DisposeAsync();
}

// new
public partial class StreamReader : TextReader
{
    public override ValueTask DisposeAsync();
}

// existing
public partial class StreamWriter : TextWriter
{
    public override ValueTask DisposeAsync();
}

API Usage

await using var reader = new StreamReader(myAsyncStream);

Alternative Designs

No response

Risks

No response

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Background and motivation

By default, when disposing a StreamReader or StreamWriter, the underlying Stream itself is disposed. Since Stream supports async disposal via IAsyncDisposable, both StreamReader and StreamWriter should implement IAsyncDisposable as well to support the case when the underlying stream supports it. Currently, only StreamWriter implements IAsyncDisposable while StreamReader doesn't, so when using StreamReader, it's only possible to dispose the underlying stream synchronously.

API Proposal

namespace System.IO;

// new
public abstract partial class TextReader : MarshalByRefObject, IDisposable, IAsyncDisposable
{
    public virtual ValueTask DisposeAsync();
}

// existing
public abstract partial class TextWriter : MarshalByRefObject, IDisposable, IAsyncDisposable
{
    public virtual ValueTask DisposeAsync();
}

// new
public partial class StreamReader : TextReader
{
    public override ValueTask DisposeAsync();
}

// existing
public partial class StreamWriter : TextWriter
{
    public override ValueTask DisposeAsync();
}

API Usage

await using var reader = new StreamReader(myAsyncStream);

Alternative Designs

No response

Risks

No response

Author:	Neme12
Assignees:	-
Labels:	api-suggestion, area-System.IO
Milestone:	-

[adamsitnik]

Hi @Neme12

Thank you for your proposal. It sounds very reasonable to me, so I've marked it as ready for review. It should get reviewed by our API Review Board within few weeks.

[terrajobst]

Video

• Looks good as proposed

namespace System.IO;

public partial class TextReader
{
    public virtual ValueTask DisposeAsync();
}

public partial class StreamReader : TextReader
{
    public override ValueTask DisposeAsync();
}

[dersia]

I would like to take care of this

[danmoseley]

@dersia Sounds great, assigned to you. Note: main will soon be a .NET 9 branch.

[Neme12]

@dersia You might want to take care of #88246 at the same time, or at least make sure to not include the same issue when implementing StreamReader.DisposeAsync.

[dersia]

@Neme12 it took me some time to set everything up and I started the implementation.
The review team said to implement it just the way that it is implemented for StreamWriter.
Now I have just one question, the implementation of IDisposable itself is very different between StreamWriter and StreamReader, should I adjust those too or just leave them as they are?

Edit:
I think there are some potential bugs in the implementation of StreamWriter Dispose.
It calls CheckAsyncTaskInProgress which might throw and then goes on to close the stream and set _disposed to true. If CheckAsyncTaskInProgresss throws, Flush is never called and the Stream will be closed and the Writer will be disposed. This feels wrong, shouldn't dispose guarantee, that Flush is always called?
I changed it so, that CheckAsyncTaskInProgresss happens outside of the try-block, but I think changing this might have some implication, because this would then throw, if CheckAsyncTaskInProgresss throws. Would this be a breaking change?

Also the Writer is never set to _disposed = true if the stream isn't closable, which also seems wrong.

If you could clarify these points for me or tell me how to handle those changes, I can finish the work and sumbit the pull request.

// @stephentoub

[Neme12]

@dersia I'm not a member of .NET or even a contributor here, I'm just a random person who discovered an issue, you might want to ask someone else :D

[Neme12]

The review team said

You already have a PR for this? Or what review team? Just curious

[dersia]

May be @adamsitnik or @stephentoub can help? see comment #88244 (comment)

[Neme12]

The review team said to implement it just the way that it is implemented for StreamWriter.

If what you mean by that is that it should call the synchronous Close method on the stream, then there's no point in adding this API whatsoever as it would end up doing the same thing as the synchronous Dispose method :D

[Jozkee]

If CheckAsyncTaskInProgresss throws, Flush is never called and the Stream will be closed and the Writer will be disposed. This feels wrong, shouldn't dispose guarantee, that Flush is always called?

@dersia that's a good question, moving the CheckAsyncTaskInProgresss() before the try so the exception surfaces makes sense to me but I think we should avoid making changes there if we don't have a concrete case that shows that the code is wrong.

You should try to circumvent these issues if possible.

cc @stephentoub

[dersia]

@Jozkee for now I have just mirrored what StreamWriter does.
I think we need another issue to look into the possible bugs and I think it makes sense to add CloseAsync to stream, because of the base implementation of Close on stream.

[stephentoub]

CheckAsyncTaskInProgresss throws, Flush is never called and the Stream will be closed and the Writer will be disposed. This feels wrong, shouldn't dispose guarantee, that Flush is always called? I changed it so, that CheckAsyncTaskInProgresss happens outside of the try-block, but I think changing this might have some implication, because this would then throw, if CheckAsyncTaskInProgresss throws. Would this be a breaking change?

CheckAsyncTaskInProgress will only throw if the StreamWriter is misused; behavior here is undefined. If it is misused and you're in this situation, there really isn't a right answer for what to do. If you move it to before the try block, then most likely it still won't be flushed and instead of determinstically closing the handle in response to Dispose, it'll just get non-deterministically closed when the handle is eventually finalized, still without the data being flushed first.

I don't see a good reason to change anything here.

[jeffhandley]

I updated #89477 (comment) with a note that we should hold off on merging this feature in until .NET 9.

@dersia / @Neme12 -- As part of my consideration for that conclusion, I watched the API Review video and read through this issue/comments. I want to make sure I'm not overlooking a scenario where this would be highly valuable. Can you share more about your scenarios where an async dispose during read will be helpful (beyond having API consistency with writing)? What type of async functionality would you like to have inside an async disposal? Thanks!

[Neme12]

@jeffhandley It's not about consistency with writing. It's when you have a Stream that itself supports async disposal - any scenario in which you'd have such a stream (and it must have been already agreed upon that there are scenarios for that since Stream already implements IAsyncDisposable) - the StreamReader disposes the Stream itself in its own disposal. You want to avoid having to dispose such a stream synchronously because the StreamReader only supports synchronous disposal.

[stephentoub]

any scenario in which you'd have such a stream (and it must have been already agreed upon that there are scenarios for that since Stream already implements IAsyncDisposable)

Generally such async I/O happens as part of writing, which is why this was added to TextWriter and StreamWriter as well. Stream.Dispose for streams from which you're only reading typically doesn't do I/O.

Can you share what scenario you have where you're using a TextReader to read from Stream that will do I/O as part of its disposal?

[Neme12]

If you have a Stream reading from an HTTP response, it might have to do IO during closing even if you're only reading from it.

[stephentoub]

If you have a Stream reading from an HTTP response, it might have to do IO during closing even if you're only reading from it.

Thanks. Is that a hypothetical or could you put us to some examples in code where that happens? It'd be helpful to see.

[Neme12]

I mean I assume that when I get a stream via HttpClient.GetStreamAsync() that its DisposeAsync disconnects the socket asynchronously since Socket has a DisconnectAsync method.

using var client = new HttpClient();
await using var stream = await client.GetStreamAsync(url);

[stephentoub]

I mean I assume that when I get a stream via HttpClient.GetStreamAsync() that its DisposeAsync disconnects the socket asynchronously since Socket has a DisconnectAsync method.

It does not.

[Neme12]

Why doesn't it do so?

[stephentoub]

Why doesn't it do so?

Because Disconnect{Async} is about being able to reuse the same socket handle again for another connection. It's not relevant here.

[Neme12]

Why does it then have a reuseSocket parameter if you're only supposed to use it when you want to pass in true?

[stephentoub]

You generally don't. It's basically just Shutdown at that point (in fact the Unix implementation just calls Shutdown). The .NET API has it because the winsock DisconnectEx has it.

[stephentoub]

Regardless, from your comments / questions this all still sounds theoretical and that you don't actually have a known case where asynchronous work would be performed in a DisposeAsync on a Stream you're using underneath TextReader?

[Neme12]

I guess I don't then if the DisposeAsync is actually synchronous in the example I provided.

[AndyBevan]

@stephentoub - I was looking at picking this up to implement but I see you removed the "good first issue" and "help wanted" labels.

Is this issue no longer looking for help on it?

[stephentoub]

Thanks, @AndyBevan. I removed the label because I'm still not convinced this is a necessary addition. Do you have an example case where it would be beneficial?

[AndyBevan]

Hi @stephentoub - No I don't think I do have an example, I was looking at this as something to help out on but after re-reviewing the thread it sounds like this code change is no longer needed - I'll look for something else. Thanks for getting back to me.

[Neme12]

Regardless, from your comments / questions this all still sounds theoretical and that you don't actually have a known case where asynchronous work would be performed in a DisposeAsync on a Stream you're using underneath TextReader?

@stephentoub I'm sorry, I assumed the disposal of the stream would be asynchronous in the example I provided:

using var client = new HttpClient();
await using var stream = await client.GetStreamAsync(url);

I should have tried looking into the implementation details before opening this issue. However, I'd still argue there is value in this, since whether or not the returned stream in this case supports asynchronous disposal is indeed an implementation detail that I cannot infer by looking at the API, and that could in theory change in the future. The same is true for many other streams that come from various APIs. Stream is an abstraction. If I don't know whether a certain stream has async disposal or not, the safer thing to do is to always use DisposeAsync - at worst, it will just return synchronously and everything will be fine. If it's asynchronous however and I call the synchronous Dispose method, I could be doing sync-over-async, which is a big no-no.