Only call base.DisposeAsync on classes derived from FileStream (#82874)

* Only call base.DisposeAsync on classes derived from FileStream * Fix typo
dotnet · Apr 4, 2023 · 17a3ebd · 17a3ebd
1 parent 25be848
commit 17a3ebd
Show file tree

Hide file tree

Showing 5 changed files with 91 additions and 7 deletions.
diff --git 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 DerivedFileStreamAccessingPropertiesOnDispose(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 DerivedFileStreamAccessingPropertiesOnDispose : FileStream
+        {
+            private readonly string _name;
+            private bool _disposed;
+
+            public DerivedFileStreamAccessingPropertiesOnDispose(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<ObjectDisposedException>(() => Length);
+                Assert.Throws<ObjectDisposedException>(() => Position);
+#pragma warning disable CS0618 // Type or member is obsolete
+                Assert.Throws<ObjectDisposedException>(() => Handle);
+#pragma warning restore CS0618 // Type or member is obsolete
+                Assert.Throws<ObjectDisposedException>(() => 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
@@ -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.DerivedFileStreamAccessingPropertiesOnDispose(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
@@ -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<byte> 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
@@ -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
@@ -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; }