From a19a9e7cb74d4e2fc4f407540500177b6176194e Mon Sep 17 00:00:00 2001 From: David Cantu Date: Wed, 1 Mar 2023 19:57:45 -0600 Subject: [PATCH 1/2] Only call base.DisposeAsync on classes derived from FileStream --- .../tests/FileStream/Dispose.cs | 56 +++++++++++++++++++ .../tests/FileStream/DisposeAsync.cs | 24 ++++++-- .../src/System/IO/FileStream.cs | 13 ++++- .../Strategies/DerivedFileStreamStrategy.cs | 3 +- .../IO/Strategies/FileStreamStrategy.cs | 2 + 5 files changed, 91 insertions(+), 7 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs index a6c649b5118f0..4eeee20cf982e 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs @@ -213,6 +213,14 @@ public void DisposeFlushesWriteBuffer() } } + [Fact] + public void DerivedFileStream_PropertiesDontThrow_OnDispose() + { + var fs = new DerivedFileStreamAccesingPropertiesOnDispose(GetTestFilePath(), FileMode.Create); + fs.Dispose(); + fs.VerifyAfterDispose(); + } + public class DerivedFileStreamWithFinalizer : FileStream { public static int DisposeTrueCalled = 0; @@ -289,6 +297,54 @@ protected override void Dispose(bool disposing) } } + public sealed class DerivedFileStreamAccesingPropertiesOnDispose : FileStream + { + private readonly string _name; + private bool _disposed; + + public DerivedFileStreamAccesingPropertiesOnDispose(string path, FileMode mode) : base(path, mode, FileAccess.ReadWrite) + { + _name = path; + } + + protected override void Dispose(bool disposing) + { + if (!_disposed) + { + Assert.Equal(_name, Name); + Assert.False(IsAsync); + Assert.True(CanRead); + Assert.True(CanSeek); + Assert.True(CanWrite); + Assert.False(CanTimeout); + Assert.Equal(0, Length); + Assert.Equal(0, Position); +#pragma warning disable CS0618 // Type or member is obsolete + Assert.NotEqual(nint.Zero, Handle); +#pragma warning restore CS0618 // Type or member is obsolete + Assert.NotNull(SafeFileHandle); + _disposed = true; + } + + base.Dispose(disposing); + } + + public void VerifyAfterDispose() + { + Assert.True(_disposed, "This method must be called only after the object has been disposed."); + Assert.Throws(() => Length); + Assert.Throws(() => Position); +#pragma warning disable CS0618 // Type or member is obsolete + Assert.Throws(() => Handle); +#pragma warning restore CS0618 // Type or member is obsolete + Assert.Throws(() => SafeFileHandle); + Assert.False(CanRead); + Assert.False(CanSeek); + Assert.False(CanWrite); + Assert.False(CanTimeout); + } + } + [ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))] public void FinalizeFlushesWriteBuffer() => VerifyFlushedBufferOnFinalization( diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/DisposeAsync.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/DisposeAsync.cs index 8069173113cff..3d7f3dc6cad78 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/DisposeAsync.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/DisposeAsync.cs @@ -41,23 +41,39 @@ public async Task DisposeAsyncFlushes() } [Fact] - public async Task DerivedFileStreamDisposeUsedForDisposeAsync() + public async Task DerivedFileStreamDisposeAndCloseUsedForDisposeAsync() { - var fs = new OverridesDisposeFileStream(GetTestFilePath(), FileMode.Create); + var fs = new DerivedFileStream(GetTestFilePath(), FileMode.Create); Assert.False(fs.DisposeInvoked); + Assert.False(fs.CloseInvoked); await fs.DisposeAsync(); Assert.True(fs.DisposeInvoked); + Assert.True(fs.CloseInvoked); } - private sealed class OverridesDisposeFileStream : FileStream + [Fact] + public async Task DerivedFileStream_PropertiesDontThrow_OnDisposeAsync() { + var fs = new FileStream_Dispose.DerivedFileStreamAccesingPropertiesOnDispose(GetTestFilePath(), FileMode.Create); + await fs.DisposeAsync(); + fs.VerifyAfterDispose(); + } + + private sealed class DerivedFileStream : FileStream + { + public bool CloseInvoked; public bool DisposeInvoked; - public OverridesDisposeFileStream(string path, FileMode mode) : base(path, mode) { } + public DerivedFileStream(string path, FileMode mode) : base(path, mode) { } protected override void Dispose(bool disposing) { DisposeInvoked = true; base.Dispose(disposing); } + public override void Close() + { + CloseInvoked = true; + base.Close(); + } } } } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs index 9fa6984f59e4b..d7e940f62e8b4 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs @@ -520,8 +520,14 @@ public override long Position public override async ValueTask DisposeAsync() { await _strategy.DisposeAsync().ConfigureAwait(false); - Dispose(false); - GC.SuppressFinalize(this); + + // For compatibility, derived classes must only call base.DisposeAsync(), + // otherwise we would end up calling Dispose twice (one from base.DisposeAsync() and one from here). + if (!_strategy.IsDerived) + { + Dispose(false); + GC.SuppressFinalize(this); + } } public override void CopyTo(Stream destination, int bufferSize) @@ -622,6 +628,9 @@ internal Task BaseWriteAsync(byte[] buffer, int offset, int count, CancellationT internal ValueTask BaseWriteAsync(ReadOnlyMemory buffer, CancellationToken cancellationToken = default) => base.WriteAsync(buffer, cancellationToken); + internal ValueTask BaseDisposeAsync() + => base.DisposeAsync(); + internal Task BaseCopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) => base.CopyToAsync(destination, bufferSize, cancellationToken); diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/DerivedFileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/DerivedFileStreamStrategy.cs index a68ff794f394c..5e5aea65d4d38 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/DerivedFileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/DerivedFileStreamStrategy.cs @@ -21,6 +21,7 @@ internal DerivedFileStreamStrategy(FileStream fileStream, FileStreamStrategy str { _fileStream = fileStream; _strategy = strategy; + IsDerived = true; } public override bool CanRead => _strategy.CanRead; @@ -146,7 +147,7 @@ public override Task FlushAsync(CancellationToken cancellationToken) public override Task CopyToAsync(Stream destination, int bufferSize, CancellationToken cancellationToken) => _fileStream.BaseCopyToAsync(destination, bufferSize, cancellationToken); - public override ValueTask DisposeAsync() => _strategy.DisposeAsync(); + public override ValueTask DisposeAsync() => _fileStream.BaseDisposeAsync(); protected sealed override void Dispose(bool disposing) => _strategy.DisposeInternal(disposing); } diff --git a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamStrategy.cs b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamStrategy.cs index de021bc67b368..ece6428364081 100644 --- a/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamStrategy.cs +++ b/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/FileStreamStrategy.cs @@ -9,6 +9,8 @@ internal abstract class FileStreamStrategy : Stream { internal abstract bool IsAsync { get; } + internal bool IsDerived { get; init; } + internal abstract string Name { get; } internal abstract SafeFileHandle SafeFileHandle { get; } From c7cadc961ba8809ca0572d05421e6162941a4839 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?David=20Cant=C3=BA?= Date: Wed, 22 Mar 2023 16:55:50 -0500 Subject: [PATCH 2/2] Fix typo --- .../System.IO.FileSystem/tests/FileStream/Dispose.cs | 6 +++--- .../System.IO.FileSystem/tests/FileStream/DisposeAsync.cs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs index 4eeee20cf982e..3b947b65681e6 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs @@ -216,7 +216,7 @@ public void DisposeFlushesWriteBuffer() [Fact] public void DerivedFileStream_PropertiesDontThrow_OnDispose() { - var fs = new DerivedFileStreamAccesingPropertiesOnDispose(GetTestFilePath(), FileMode.Create); + var fs = new DerivedFileStreamAccessingPropertiesOnDispose(GetTestFilePath(), FileMode.Create); fs.Dispose(); fs.VerifyAfterDispose(); } @@ -297,12 +297,12 @@ protected override void Dispose(bool disposing) } } - public sealed class DerivedFileStreamAccesingPropertiesOnDispose : FileStream + public sealed class DerivedFileStreamAccessingPropertiesOnDispose : FileStream { private readonly string _name; private bool _disposed; - public DerivedFileStreamAccesingPropertiesOnDispose(string path, FileMode mode) : base(path, mode, FileAccess.ReadWrite) + public DerivedFileStreamAccessingPropertiesOnDispose(string path, FileMode mode) : base(path, mode, FileAccess.ReadWrite) { _name = path; } diff --git a/src/libraries/System.IO.FileSystem/tests/FileStream/DisposeAsync.cs b/src/libraries/System.IO.FileSystem/tests/FileStream/DisposeAsync.cs index 3d7f3dc6cad78..49c79c49987e4 100644 --- a/src/libraries/System.IO.FileSystem/tests/FileStream/DisposeAsync.cs +++ b/src/libraries/System.IO.FileSystem/tests/FileStream/DisposeAsync.cs @@ -54,7 +54,7 @@ public async Task DerivedFileStreamDisposeAndCloseUsedForDisposeAsync() [Fact] public async Task DerivedFileStream_PropertiesDontThrow_OnDisposeAsync() { - var fs = new FileStream_Dispose.DerivedFileStreamAccesingPropertiesOnDispose(GetTestFilePath(), FileMode.Create); + var fs = new FileStream_Dispose.DerivedFileStreamAccessingPropertiesOnDispose(GetTestFilePath(), FileMode.Create); await fs.DisposeAsync(); fs.VerifyAfterDispose(); }