StreamWriter might not call Flush

[dersia]

Description

While working on #88244 I found that there might be a possible bug in StreamWriters implementation when disposing.

Dispose(bool disposing) and DisposeAyncCore both call CheckAsyncTaskInProgress() which might throw. In these cases Flush(Async) is not called, which is fine, however, because this is all wrapped in a try-finally-block, both call Close() on the underlying Stream and set _disposing to true, which then does not allow for a retry on Dispose nor Flush since the Writer is "disposed" at this point. I think this is a bug and if CheckAsyncTaskInProgress() throws it shouldn't be caught but it should throw. Same for Flush, as a user I should know when Flush throws and it shouldn't silently close the stream.

This would however be a breaking change and therefor I am not sure, if this can be considered. However if this is going to be changed, then StreamReader should be fixed to, since I just mirrored the implementation of StreamWriter onto StreamReader in #88244

Reproduction Steps

It is hard to create a repro, since this can be the case if the underlying stream is slow and we are writing a lot of data to it. So this repro makes some assumptions

var sr = new StreamReader(new MemoryStream());
//do not await the write
sr.WriteAsync("abc");
//dispose right away
sr.Dispose();
// assume flush failed
sr.Flush();

Expected behavior

Exception to be thrown if the WriteTask is still ongoing or if Flushing failed.

Actual behavior

Silently closes the stream without any issues being reported

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

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

---

Description

While working on #88244 I found that there might be a possible bug in StreamWriters implementation when disposing.

Dispose(bool disposing) and DisposeAyncCore both call CheckAsyncTaskInProgress() which might throw. In these cases Flush(Async) is not called, which is fine, however, because this is all wrapped in a try-finally-block, both call Close() on the underlying Stream and set _disposing to true, which then does not allow for a retry on Dispose nor Flush since the Writer is "disposed" at this point. I think this is a bug and if CheckAsyncTaskInProgress() throws it shouldn't be caught but it should throw. Same for Flush, as a user I should know when Flush throws and it shouldn't silently close the stream.

This would however be a breaking change and therefor I am not sure, if this can be considered. However if this is going to be changed, then StreamReader should be fixed to, since I just mirrored the implementation of StreamWriter onto StreamReader in #88244

Reproduction Steps

It is hard to create a repro, since this can be the case if the underlying stream is slow and we are writing a lot of data to it. So this repro makes some assumptions

var sr = new StreamReader(new MemoryStream());
//do not await the write
sr.WriteAsync("abc");
//dispose right away
sr.Dispose();
// assume flush failed
sr.Flush();

Expected behavior

Exception to be thrown if the WriteTask is still ongoing or if Flushing failed.

Actual behavior

Silently closes the stream without any issues being reported

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

Author:	dersia
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[stephentoub]

Quoting from #88244 (comment):

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.