Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Apr 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
56 changes: 56 additions & 0 deletions src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
}
}
}
13 changes: 11 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ internal DerivedFileStreamStrategy(FileStream fileStream, FileStreamStrategy str
{
_fileStream = fileStream;
_strategy = strategy;
IsDerived = true;
}

public override bool CanRead => _strategy.CanRead;
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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; }
Expand Down