Fix StreamWriter keeps working after being disposed if leaveOpen: true is used#125189
Fix StreamWriter keeps working after being disposed if leaveOpen: true is used#125189ViveliDuCh wants to merge 3 commits intodotnet:mainfrom
leaveOpen: true is used#125189Conversation
…roring the StreamReader.Dispose pattern. Add tests for Dispose and DisposeAsync with leaveOpen: true, plus a StreamReader parity test.
There was a problem hiding this comment.
Pull request overview
Fixes a StreamWriter disposal bug where leaveOpen: true prevented the writer from transitioning to a disposed state, allowing post-Dispose() / DisposeAsync() writes to proceed instead of throwing ObjectDisposedException. This aligns StreamWriter behavior with StreamReader and expected .NET disposed-object semantics.
Changes:
- Update
StreamWriter.CloseStreamFromDisposeso_disposedis set (and internal state is cleared) regardless ofleaveOpen, while_closableonly gates closing the underlying stream. - Add regression tests for sync
Dispose()+leaveOpen: trueand asyncDisposeAsync()+leaveOpen: true. - Add a parity test confirming
StreamReaderthrows after disposal whenleaveOpen: true.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
src/libraries/System.Private.CoreLib/src/System/IO/StreamWriter.cs |
Ensures StreamWriter becomes disposed even when leaveOpen: true, preventing further writes after disposal. |
src/libraries/System.Runtime/tests/System.IO.Tests/StreamWriter/StreamWriter.DisposeAsync.cs |
Adds regression coverage for DisposeAsync() with leaveOpen: true throwing on subsequent writes. |
src/libraries/System.Runtime/tests/System.IO.Tests/StreamWriter/StreamWriter.CloseTests.cs |
Adds regression coverage for Dispose() with leaveOpen: true throwing on subsequent operations. |
src/libraries/System.Runtime/tests/System.IO.Tests/StreamReader/StreamReaderTests.cs |
Adds a test asserting StreamReader throws after disposal when leaveOpen: true (behavior parity reference). |
src/libraries/System.Runtime/tests/System.IO.Tests/StreamWriter/StreamWriter.CloseTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.IO.Tests/StreamReader/StreamReaderTests.cs
Outdated
Show resolved
Hide resolved
stephentoub
left a comment
There was a problem hiding this comment.
This is likely to break someone. I think it's ok to make, as we say that use-after-dispose is erroneous. But we might want to consider marking it as a breaking change.
|
@dotnet/compat This fix changes |
Console.Out is a process-wide singleton wrapping stdout. Disposing it is always a mistake like disposing StreamWriter.Null. The fact that it survived dispose calls before was an accident (caused by the bug we're fixing). The NonClosableStreamWriter is making explicit what was previously implicit. It's 6 lines of logic and follows the exact same pattern as NullStreamWriter and NullStreamReader, which exist for the same reason.
e37729e to
548b3e6
Compare
Fixes #89646
Description
StreamWritercreated withleaveOpen: truesilently continues accepting writes afterDispose()/DisposeAsync()instead of throwingObjectDisposedException.StreamReaderwith the same flag correctly throws.The root cause is in
CloseStreamFromDispose: the_disposed = trueassignment was gated behindif (_closable && !_disposed), where _closable is false whenleaveOpen: true. This meant_disposedwas never set to true, soThrowIfDisposed()never fired.Fix
Restructured
CloseStreamFromDisposeso_disposed = true,_charLen = 0, andbase.Dispose(disposing)execute unconditionally in thefinallyblock, while _closable now only gates_stream.Close(). This mirrors the pattern already used by StreamReader inStreamReader.Dispose(bool):private void CloseStreamFromDispose(bool disposing) { - if (_closable && !_disposed) + if (!_disposed) { try { - if (disposing) + if (_closable && disposing) { _stream.Close(); } } finally { _disposed = true; _charLen = 0; base.Dispose(disposing); } } }NullStreamWriter(backingStreamWriter.Null) is not affected — it overrides bothDispose(bool)andDisposeAsync()as no-ops and never callsCloseStreamFromDispose.Testing
Added tests to existing test files (no new files, no csproj changes needed):
CloseTests.AfterDisposeThrows_LeaveOpenTrue: sync Dispose withleaveOpen: true.StreamWriterTests.DisposeAsync_LeaveOpenTrue_ThrowsAfterDispose: async DisposeAsync withleaveOpen: trueStreamReaderTests.ObjectDisposedExceptionDisposedStream_LeaveOpenTrue: confirms StreamReader parity.Full
System.IO.Testssuite: 1712 tests, 0 regressions (1 pre-existing unrelated failure inMemoryStream_CapacityBoundaryChecks).