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.

Sign up for GitHub

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

Add missing GC.SuppressFinalize #64997

Merged
merged 7 commits into from
Feb 23, 2022
Merged

Add missing GC.SuppressFinalize #64997

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1fce48a
add tests (they are currently failing)
adamsitnik Feb 8, 2022
17ab6a5
remove redundant call to base.Dispose
adamsitnik Feb 8, 2022
63ea3e6
fix the bug
adamsitnik Feb 8, 2022
706d768
add test for derived FileStream with a finalizer
adamsitnik Feb 9, 2022
281b2a7
add test for derived FileStream without a finalizer
adamsitnik Feb 9, 2022
e6e8326
restore finalizer to the FileStream
adamsitnik Feb 9, 2022
1444fe5
address code review feedback
adamsitnik Feb 11, 2022
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
232 changes: 223 additions & 9 deletions src/libraries/System.IO.FileSystem/tests/FileStream/Dispose.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,21 +2,21 @@
// The .NET Foundation licenses this file to you under the MIT license.

using Microsoft.Win32.SafeHandles;
using System;
using System.Diagnostics;
using System.IO;
using Microsoft.DotNet.RemoteExecutor;
using Xunit;
using System.Collections.Generic;
using System.Threading.Tasks;

namespace System.IO.Tests
{
[Collection(nameof(DisableParallelization))] // make sure no other tests are calling GC.Collect()
public class FileStream_Dispose : FileSystemTest
{
[Fact]
public void DisposeClosesHandle()
{
SafeFileHandle handle;
using(FileStream fs = new FileStream(GetTestFilePath(), FileMode.Create))
using (FileStream fs = new FileStream(GetTestFilePath(), FileMode.Create))
{
handle = fs.SafeFileHandle;
}
Expand Down Expand Up @@ -199,7 +199,7 @@ public void Finalizer_CallsVirtualDispose_FalseArg()
public void DisposeFlushesWriteBuffer()
{
string fileName = GetTestFilePath();
using(FileStream fs = new FileStream(fileName, FileMode.Create))
using (FileStream fs = new FileStream(fileName, FileMode.Create))
{
fs.Write(TestBuffer, 0, TestBuffer.Length);
}
Expand All @@ -213,20 +213,143 @@ public void DisposeFlushesWriteBuffer()
}
}

public class DerivedFileStreamWithFinalizer : FileStream
{
public static int DisposeTrueCalled = 0;
public static int DisposeFalseCalled = 0;

public DerivedFileStreamWithFinalizer(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options)
: base(path, mode, access, share, bufferSize, options)
{
}

public DerivedFileStreamWithFinalizer(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync)
: base(handle, access, bufferSize, isAsync)
{
}

public DerivedFileStreamWithFinalizer(IntPtr handle, FileAccess access, bool ownsHandle)
#pragma warning disable CS0618 // Type or member is obsolete
: base(handle, access, ownsHandle)
#pragma warning restore CS0618 // Type or member is obsolete
{
}

~DerivedFileStreamWithFinalizer() => Dispose(false);

protected override void Dispose(bool disposing)
{
if (disposing)
{
DisposeTrueCalled++;
}
else
{
DisposeFalseCalled++;
}

base.Dispose(disposing);
}
}

public class DerivedFileStreamWithoutFinalizer : FileStream
{
public static int DisposeTrueCalled = 0;
public static int DisposeFalseCalled = 0;

public DerivedFileStreamWithoutFinalizer(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options)
: base(path, mode, access, share, bufferSize, options)
{
}

public DerivedFileStreamWithoutFinalizer(SafeFileHandle handle, FileAccess access, int bufferSize, bool isAsync)
: base(handle, access, bufferSize, isAsync)
{
}

public DerivedFileStreamWithoutFinalizer(IntPtr handle, FileAccess access, bool ownsHandle)
#pragma warning disable CS0618 // Type or member is obsolete
: base(handle, access, ownsHandle)
#pragma warning restore CS0618 // Type or member is obsolete
{
}

protected override void Dispose(bool disposing)
{
if (disposing)
{
DisposeTrueCalled++;
}
else
{
DisposeFalseCalled++;
}

base.Dispose(disposing);
}
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
public void FinalizeFlushesWriteBuffer()
=> VerifyFlushedBufferOnFinalization(
filePath => new FileStream(filePath, FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete, bufferSize: 4096, useAsync: false));

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
public void FinalizeFlushesWriteBufferForDerivedFileStreamWithFinalizerCreatedFromPath()
=> VerifyFlushedBufferOnFinalization(
filePath => new DerivedFileStreamWithFinalizer(filePath, FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete, bufferSize: 4096, FileOptions.None));

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
public void FinalizeFlushesWriteBufferForDerivedFileStreamWithFinalizerCreatedFromSafeFileHandle()
=> VerifyFlushedBufferOnFinalization(
filePath => new DerivedFileStreamWithFinalizer(
File.OpenHandle(filePath, FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete, FileOptions.None),
FileAccess.Write, bufferSize: 4096, isAsync: false));

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
public void FinalizeFlushesWriteBufferForDerivedFileStreamWithFinalizerCreatedFromIntPtr()
=> VerifyFlushedBufferOnFinalization(
filePath => new DerivedFileStreamWithFinalizer(
File.OpenHandle(filePath, FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete, FileOptions.None).DangerousGetHandle(),
FileAccess.Write,
ownsHandle: true));

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
public void FinalizeFlushesWriteBufferForDerivedFileStreamWithoutFinalizerCreatedFromPath()
=> VerifyFlushedBufferOnFinalization(
filePath => new DerivedFileStreamWithoutFinalizer(filePath, FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete, bufferSize: 4096, FileOptions.None));

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
public void FinalizeFlushesWriteBufferForDerivedFileStreamWithoutFinalizerCreatedFromSafeFileHandle()
=> VerifyFlushedBufferOnFinalization(
filePath => new DerivedFileStreamWithoutFinalizer(
File.OpenHandle(filePath, FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete, FileOptions.None),
FileAccess.Write, bufferSize: 4096, isAsync: false));

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
public void FinalizeFlushesWriteBufferForDerivedFileStreamWithoutFinalizerCreatedFromIntPtr()
=> VerifyFlushedBufferOnFinalization(
filePath => new DerivedFileStreamWithoutFinalizer(
File.OpenHandle(filePath, FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete, FileOptions.None).DangerousGetHandle(),
FileAccess.Write,
ownsHandle: true));

private void VerifyFlushedBufferOnFinalization(Func<string, FileStream> factory)
{
string fileName = GetTestFilePath();

// use a separate method to be sure that fs isn't rooted at time of GC.
Action leakFs = () =>
Func<string, Type> leakFs = filePath =>
{
// we must specify useAsync:false, otherwise the finalizer just kicks off an async write.
FileStream fs = new FileStream(fileName, FileMode.Create, FileAccess.Write, FileShare.ReadWrite | FileShare.Delete, bufferSize: 4096, useAsync: false);
FileStream fs = factory(filePath);
fs.Write(TestBuffer, 0, TestBuffer.Length);
fs = null;
return fs.GetType();
};
leakFs();

int beforeWithFinalizer = DerivedFileStreamWithFinalizer.DisposeFalseCalled;
int beforeWithoutFinalizer = DerivedFileStreamWithoutFinalizer.DisposeFalseCalled;
Type type = leakFs(fileName);

GC.Collect();
GC.WaitForPendingFinalizers();
Expand All @@ -238,6 +361,97 @@ public void FinalizeFlushesWriteBuffer()
fsr.Read(buffer, 0, buffer.Length);
Assert.Equal(TestBuffer, buffer);
}

if (type == typeof(DerivedFileStreamWithFinalizer))
{
// derived type finalizer implicitly calls base type finalizer, hence +2
Assert.Equal(beforeWithFinalizer + 2, DerivedFileStreamWithFinalizer.DisposeFalseCalled);
Assert.Equal(0, DerivedFileStreamWithFinalizer.DisposeTrueCalled);
}
else if (type == typeof(DerivedFileStreamWithoutFinalizer))
{
Assert.Equal(beforeWithoutFinalizer + 1, DerivedFileStreamWithoutFinalizer.DisposeFalseCalled);
Assert.Equal(0, DerivedFileStreamWithoutFinalizer.DisposeTrueCalled);
}
}

// this type exists so DerivedFileStreamStrategy can be tested as well
public class DerivedFileStream : FileStream
{
public DerivedFileStream(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options)
: base(path, mode, access, share, bufferSize, options)
{
}
}

public static IEnumerable<object[]> GetFileStreamDisposeSuppressesStrategyFinalizationArgs()
{
foreach (int bufferSize in new[] { 1, 4096 })
{
foreach (FileOptions fileOptions in new[] { FileOptions.Asynchronous, FileOptions.None })
{
yield return new object[] { bufferSize, fileOptions };
}
}
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
[MemberData(nameof(GetFileStreamDisposeSuppressesStrategyFinalizationArgs))]
public Task DisposeSuppressesStrategyFinalization(int bufferSize, FileOptions options)
=> VerifyStrategyFinalization(
() => new FileStream(GetTestFilePath(), FileMode.Create, FileAccess.Write, FileShare.None, bufferSize, options | FileOptions.DeleteOnClose),
false);

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
[MemberData(nameof(GetFileStreamDisposeSuppressesStrategyFinalizationArgs))]
public Task DisposeSuppressesStrategyFinalizationAsync(int bufferSize, FileOptions options)
=> VerifyStrategyFinalization(
() => new FileStream(GetTestFilePath(), FileMode.Create, FileAccess.Write, FileShare.None, bufferSize, options | FileOptions.DeleteOnClose),
true);

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
[MemberData(nameof(GetFileStreamDisposeSuppressesStrategyFinalizationArgs))]
public Task DerivedFileStreamDisposeSuppressesStrategyFinalization(int bufferSize, FileOptions options)
=> VerifyStrategyFinalization(
() => new DerivedFileStream(GetTestFilePath(), FileMode.Create, FileAccess.Write, FileShare.None, bufferSize, options | FileOptions.DeleteOnClose),
false);

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsPreciseGcSupported))]
[MemberData(nameof(GetFileStreamDisposeSuppressesStrategyFinalizationArgs))]
public Task DerivedFileStreamDisposeSuppressesStrategyFinalizationAsync(int bufferSize, FileOptions options)
=> VerifyStrategyFinalization(
() => new DerivedFileStream(GetTestFilePath(), FileMode.Create, FileAccess.Write, FileShare.None, bufferSize, options | FileOptions.DeleteOnClose),
true);

private static async Task VerifyStrategyFinalization(Func<FileStream> factory, bool useAsync)
{
WeakReference weakReference = await EnsureFileStreamIsNotRooted(factory, useAsync);
Assert.True(weakReference.IsAlive);
GC.Collect();
Assert.False(weakReference.IsAlive);

// separate method to avoid JIT lifetime-extension issues
static async Task<WeakReference> EnsureFileStreamIsNotRooted(Func<FileStream> factory, bool useAsync)
{
FileStream fs = factory();
WeakReference weakReference = new WeakReference(
(Stream)typeof(FileStream)
.GetField("_strategy", Reflection.BindingFlags.NonPublic | Reflection.BindingFlags.Instance)
.GetValue(fs),
trackResurrection: true);

Assert.True(weakReference.IsAlive);

if (useAsync)
{
await fs.DisposeAsync();
}
{
fs.Dispose();
}

return weakReference;
}
}
}
}
28 changes: 25 additions & 3 deletions src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,11 @@ public FileStream(IntPtr handle, FileAccess access, bool ownsHandle, int bufferS
ValidateHandle(safeHandle, access, bufferSize, isAsync);

_strategy = FileStreamHelpers.ChooseStrategy(this, safeHandle, access, bufferSize, isAsync);

if (!_strategy.RequiresFinalizer)
{
GC.SuppressFinalize(this);
}
adamsitnik marked this conversation as resolved.
Show resolved Hide resolved
}
catch
{
Expand Down Expand Up @@ -147,6 +152,14 @@ public FileStream(string path, FileMode mode, FileAccess access, FileShare share
{
}

~FileStream()
{
// Preserved for compatibility since FileStream has defined a
// finalizer in past releases and derived classes may depend
// on Dispose(false) call.
Dispose(false);
}

/// <summary>
/// Initializes a new instance of the <see cref="System.IO.FileStream" /> class with the specified path, creation mode, read/write and sharing permission, the access other FileStreams can have to the same file, the buffer size, additional file options and the allocation size.
/// </summary>
Expand Down Expand Up @@ -196,13 +209,23 @@ public FileStream(string path, FileStreamOptions options)

_strategy = FileStreamHelpers.ChooseStrategy(
this, path, options.Mode, options.Access, options.Share, options.BufferSize, options.Options, options.PreallocationSize);

if (!_strategy.RequiresFinalizer)
{
GC.SuppressFinalize(this);
}
}

private FileStream(string path, FileMode mode, FileAccess access, FileShare share, int bufferSize, FileOptions options, long preallocationSize)
{
FileStreamHelpers.ValidateArguments(path, mode, access, share, bufferSize, options, preallocationSize);

_strategy = FileStreamHelpers.ChooseStrategy(this, path, mode, access, share, bufferSize, options, preallocationSize);

if (!_strategy.RequiresFinalizer)
{
GC.SuppressFinalize(this);
}
}

[Obsolete("FileStream.Handle has been deprecated. Use FileStream's SafeFileHandle property instead.")]
Expand Down Expand Up @@ -486,9 +509,8 @@ public override long Position
/// <param name="value">The byte to write to the stream.</param>
public override void WriteByte(byte value) => _strategy.WriteByte(value);

protected override void Dispose(bool disposing) => _strategy.DisposeInternal(disposing);

internal void DisposeInternal(bool disposing) => Dispose(disposing);
// _strategy can be null only when ctor has thrown
protected override void Dispose(bool disposing) => _strategy?.DisposeInternal(disposing);

public override ValueTask DisposeAsync() => _strategy.DisposeAsync();

Expand Down
Loading