add missing GC.SuppressFinalize(this) to FileStream.DisposeAsync by adamsitnik · Pull Request #65899 · dotnet/runtime

adamsitnik · 2022-02-25T17:17:25Z

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 · 2022-02-25T17:17:35Z

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

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

…eam.BaseDisposeAsync, as FileStream.DisposeAsync calls Dispose(false) now

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

d341407

adamsitnik added the area-System.IO label Feb 25, 2022

adamsitnik added this to the 7.0.0 milestone Feb 25, 2022

adamsitnik requested review from jozkee and stephentoub February 25, 2022 17:17

ghost assigned adamsitnik Feb 25, 2022

This was referenced Feb 25, 2022

Disable System.IO.Tests.FileStream_DisposeAsync.DisposeAsyncFlushes test #65892

Merged

[release/6.0] Add missing GC.SuppressFinalize #65766

Closed

stephentoub reviewed Feb 25, 2022

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/IO/FileStream.cs Outdated Show resolved Hide resolved

adamsitnik added 4 commits February 28, 2022 10:59

apply code review suggestions

fd84c9b

DerivedFileStreamStrategy.DisposeAsync does not need to call _fileStr…

9a430ab

…eam.BaseDisposeAsync, as FileStream.DisposeAsync calls Dispose(false) now

Merge remote-tracking branch 'upstream/main' into issue65835

c4d0b13

re-enable the failing test

0884b24

adamsitnik requested a review from stephentoub February 28, 2022 11:55

runfoapp bot mentioned this pull request Feb 28, 2022

System.Runtime.Serialization.Xml.Tests failed #59926

Closed

stephentoub approved these changes Feb 28, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into issue65835

ba3a068

adamsitnik merged commit 3cbed4b into dotnet:main Mar 1, 2022

adamsitnik deleted the issue65835 branch March 1, 2022 13:15

adamsitnik mentioned this pull request Mar 4, 2022

[test][mono][linux][x64] System.IO.Tests.FileStream_DisposeAsync.DisposeAsyncFlushes library test failed #65835

Closed

ghost locked as resolved and limited conversation to collaborators Mar 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

add missing GC.SuppressFinalize(this) to FileStream.DisposeAsync#65899
adamsitnik merged 6 commits intodotnet:mainfrom
adamsitnik:issue65835

adamsitnik commented Feb 25, 2022

Uh oh!

ghost commented Feb 25, 2022

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	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);
	}

	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

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

Conversation

adamsitnik commented Feb 25, 2022

Uh oh!

ghost commented Feb 25, 2022

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants