StreamWriter.DisposeAsync disposes the underlying Stream synchronously

[Neme12]

Description

I discovered issue this while filing #88244 and looking at the source code. By default, StreamWriter.Dispose also disposes/closes the underlying Stream. DisposeAsync should close the underlying Stream asynchronously, but currently closes it synchronously.

See

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

Lines 241 to 257 in 2a22adb

	private async ValueTask DisposeAsyncCore()
	{
	// Same logic as in Dispose(), but with async flushing.
	Debug.Assert(GetType() == typeof(StreamWriter));
	try
	{
	if (!_disposed)
	{
	await FlushAsync().ConfigureAwait(false);
	}
	}
	finally
	{
	CloseStreamFromDispose(disposing: true);
	}
	GC.SuppressFinalize(this);
	}

and

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

Lines 211 to 234 in 2a22adb

	private void CloseStreamFromDispose(bool disposing)
	{
	// Dispose of our resources if this StreamWriter is closable.
	if (_closable && !_disposed)
	{
	try
	{
	// Attempt to close the stream even if there was an IO error from Flushing.
	// Note that Stream.Close() can potentially throw here (may or may not be
	// due to the same Flush error). In this case, we still need to ensure
	// cleaning up internal resources, hence the finally block.
	if (disposing)
	{
	_stream.Close();
	}
	}
	finally
	{
	_disposed = true;
	_charLen = 0;
	base.Dispose(disposing);
	}
	}
	}

Reproduction Steps

Expected behavior

Actual behavior

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

I discovered issue this while filing #88244. By default, StreamWriter.Dispose also disposes the underlying Stream. DisposeAsync should dispose the underlying Stream asynchronously, but currently disposes it synchronously.

See

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

Lines 241 to 257 in 2a22adb

	private async ValueTask DisposeAsyncCore()
	{
	// Same logic as in Dispose(), but with async flushing.
	Debug.Assert(GetType() == typeof(StreamWriter));
	try
	{
	if (!_disposed)
	{
	await FlushAsync().ConfigureAwait(false);
	}
	}
	finally
	{
	CloseStreamFromDispose(disposing: true);
	}
	GC.SuppressFinalize(this);
	}

and

runtime/src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs

Lines 211 to 234 in 2a22adb

	private void CloseStreamFromDispose(bool disposing)
	{
	// Dispose of our resources if this StreamWriter is closable.
	if (_closable && !_disposed)
	{
	try
	{
	// Attempt to close the stream even if there was an IO error from Flushing.
	// Note that Stream.Close() can potentially throw here (may or may not be
	// due to the same Flush error). In this case, we still need to ensure
	// cleaning up internal resources, hence the finally block.
	if (disposing)
	{
	_stream.Close();
	}
	}
	finally
	{
	_disposed = true;
	_charLen = 0;
	base.Dispose(disposing);
	}
	}
	}

Reproduction Steps

Expected behavior

Actual behavior

Regression?

No response

Known Workarounds

No response

Configuration

No response

Other information

No response

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

[Neme12]

I also think it should call stream.Dispose() instead of stream.Close(). If anyone creates their own stream and implements these methods differently, I think the expectation is that when you call streamReader.Dispose(), it would also call Dispose() on the stream. But that would be a breaking change of course (although fixing this bug about calling the sync vs async Close method is technically a breaking change too).

[dersia]

I will take care of this as part of #88244

[jozkee]

I also think it should call stream.Dispose() instead of stream.Close()

I think for compatiblity reasons we should continue calling Close().

[dersia]

I also think it should call stream.Dispose() instead of stream.Close()

I think for compatiblity reasons we should continue calling Close().

yeah, stream.dispose() does not call Close on the stream, so that would be a behavioral change. I found some other issues on StreamWriter.Dispose(). Please see #88244 (comment)

[Neme12]

yeah, stream.dispose() does not call Close on the stream, so that would be a behavioral change

Right. But fixing this issue would be a behavioral change too, so if it's OK to make that behavioral change, then why shouldn't it be OK to make this other small behavioral change too.

[jozkee]

@Neme12 because changing DisposeAsync is not as impactful as changing Dispose which has been around for longer.

[NinoFloris]

We have an example in Npgsql https://github.com/npgsql/Npgsql/blob/main/src/Npgsql/NpgsqlRawCopyStream.cs#L353-L376 of a stream that should ideally take advantage of async if it's there.

NpgsqlRawCopyStream is the base stream used in our StreamReader/Writer derived types (same file, see NpgsqlCopyTextReader/Writer)

It seems its made very hard to have StreamWriter call DisposeAsync on the underlying stream, many methods do GetType() == typeof(StreamWriter) checks and CloseStreamDuringDispose etc are all private, the * truly * async FlushAsync implementation is also private.

I would welcome any fixes or workarounds to get StreamReader/Writer to respect DisposeAsync.