add missing GC.SuppressFinalize(this) to FileStream.DisposeAsync

[adamsitnik]

I was unable to repro #65835 locally for a few hours, but I believe that the it's caused by lack of GC.SuppressFinalize(this) in FileStream.DisposeAsync and my recent changes from #64997 have just exposed the problem by adding the finalizer to FileStream itself.

Explanation: the default Stream.DisposeAsync calls Dispose:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs

Lines 176 to 180 in 95f7f7a

	public virtual ValueTask DisposeAsync()
	{
	try
	{
	Dispose();

which calls Close which calls GC.SuppressFinalize(this):

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs

Lines 158 to 167 in 95f7f7a

	public void Dispose() => Close();
	public virtual void Close()
	{
	// When initially designed, Stream required that all cleanup logic went into Close(),
	// but this was thought up before IDisposable was added and never revisited. All subclasses
	// should put their cleanup now in Dispose(bool).
	Dispose(true);
	GC.SuppressFinalize(this);
	}

FileStream was overriding DisposeAsync, but not calling GC.SuppressFinalize(this).

runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

Line 500 in 95f7f7a

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

In #65835 we can see that the buffer was actually written to disk twice. I guess (I was not able to repro it) that it's caused by a flush implemented for finalizer. I am not 100% sure because the finally block sets write position to 0:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 115 to 121 in 95f7f7a

	finally
	{
	// Don't set the buffer to null, to avoid a NullReferenceException
	// when users have a race condition in their code (i.e. they call
	// FileStream.Close when calling another method on FileStream like Read).
	_writePos = 0; // WriteByte hot path relies on this

So after DisposeAsync the flushing should in theory see that there is nothing to flush. Moreover, it should also observe that the handle was already closed.

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 125 to 130 in 95f7f7a

	protected sealed override void Dispose(bool disposing)
	{
	if (_strategy.IsClosed)
	{
	return;
	}

I've tried really hard to write a failing unit test, but I've failed. The reason for that is that all custom types deriving from FileStream are calling BaseDisposeAsync:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

Line 579 in 95f7f7a

internal ValueTask BaseDisposeAsync() => base.DisposeAsync();

which does call the base impl and is bug free.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

I was unable to repro #65835 locally for a few hours, but I believe that the it's caused by lack of GC.SuppressFinalize(this) in FileStream.DisposeAsync and my recent changes from #64997 have just exposed the problem by adding the finalizer to FileStream itself.

Explanation: the default Stream.DisposeAsync calls Dispose:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs

Lines 176 to 180 in 95f7f7a

	public virtual ValueTask DisposeAsync()
	{
	try
	{
	Dispose();

which calls Close which calls GC.SuppressFinalize(this):

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Stream.cs

Lines 158 to 167 in 95f7f7a

	public void Dispose() => Close();
	public virtual void Close()
	{
	// When initially designed, Stream required that all cleanup logic went into Close(),
	// but this was thought up before IDisposable was added and never revisited. All subclasses
	// should put their cleanup now in Dispose(bool).
	Dispose(true);
	GC.SuppressFinalize(this);
	}

FileStream was overriding DisposeAsync, but not calling GC.SuppressFinalize(this).

runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

Line 500 in 95f7f7a

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

In #65835 we can see that the buffer was actually written to disk twice. I guess (I was not able to repro it) that it's caused by a flush implemented for finalizer. I am not 100% sure because the finally block sets write position to 0:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 115 to 121 in 95f7f7a

	finally
	{
	// Don't set the buffer to null, to avoid a NullReferenceException
	// when users have a race condition in their code (i.e. they call
	// FileStream.Close when calling another method on FileStream like Read).
	_writePos = 0; // WriteByte hot path relies on this

So after DisposeAsync the flushing should in theory see that there is nothing to flush. Moreover, it should also observe that the handle was already closed.

runtime/src/libraries/System.Private.CoreLib/src/System/IO/Strategies/BufferedFileStreamStrategy.cs

Lines 125 to 130 in 95f7f7a

	protected sealed override void Dispose(bool disposing)
	{
	if (_strategy.IsClosed)
	{
	return;
	}

I've tried really hard to write a failing unit test, but I've failed. The reason for that is that all custom types deriving from FileStream are calling BaseDisposeAsync:

runtime/src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs

Line 579 in 95f7f7a

internal ValueTask BaseDisposeAsync() => base.DisposeAsync();

which does call the base impl and is bug free.

Author:	adamsitnik
Assignees:	-
Labels:	area-System.IO
Milestone:	7.0.0