Add missing GC.SuppressFinalize

[adamsitnik]

fixes #64741

[ghost]

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

Issue Details

---

fixes #64741

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

[stephentoub]

Do we need DisposeInternal at all? Could FileStream's Dispose(bool) instead do:

if (disposing) _strategy.Dispose();

?

[adamsitnik]

@stephentoub to answer your question I've written some additional tests (to cover all scenarios). And I've discovered a new bug: for types that derive from FileStream and have buffering enabled, the buffer is currently not getting flushed when the instance is finalized. Repro:

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

[Fact]
public static void Repro()
{
    string filePath = Path.Combine(Path.GetTempPath(), Path.GetTempFileName());
    Separate(filePath);
    
    GC.Collect();
    GC.WaitForPendingFinalizers();
    
    Assert.Equal(new byte [] { 1} , File.ReadAllBytes(filePath));

    // use a separate method to be sure that fs isn't rooted at time of GC.
    static void Separate(string filePath)
    {
        FileStream fs = new DerivedFileStream(filePath, FileMode.Create, FileAccess.Write, FileShare.None, 4096, FileOptions.None);
        fs.WriteByte(1);
    }
}

It's caused by the fact that ~DerivedFileStreamStrategy is currently passing false to Dispose:

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

Lines 26 to 31 in 4f2cd55

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

which is perfectly fine, because derived type is expecting that.

However, when it's passed back to BufferedFileStreamStrategy.Dispose the Flush is not performed for !disposing:

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

Lines 142 to 150 in 4f2cd55

	protected override void Dispose(bool disposing)
	{
	try
	{
	if (disposing && !_strategy.IsClosed)
	{
	try
	{
	Flush();

~BufferedFileStreamStrategy workarounds it by always passing true to Dispose:

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

Lines 34 to 40 in 4f2cd55

	~BufferedFileStreamStrategy()
	{
	try
	{
	// the finalizer must at least try to flush the write buffer
	// so we enforce it by passing always true
	Dispose(true);

My current take on it is that removing finalizer from FileStream was a bad idea as it actually got things more complicated and regressed the performance instead of improving it. I think that we should restore it, let it run for a few weeks on main and backport it to 6.0. PTAL at my recent changes and let me know what do you think.

[rpnetcoder]

Dear Team, this is just a quick follow-up.

As the author of original issue, I would like to emphasize the significance of the bug. In my particular case, there are hundreds of thousands of FileStream objects created daily which are properly disposed after being used, but not GC collected in Gen 0 and moved to the Finalization queue. I can imagine even much larger services which might use FileStream under the hood like Microsoft Azure, where, if this bug gets fixed, the whole .NET ecosystem will benefit from it.

Thanks

[adamsitnik]

@rpnetcoder thank you for your feedback! I am definitely willing to fix the bug and do my best to backport it to 6.0.

@stephentoub could you please take a look at the PR? I've addressed your feedback

[adamsitnik]

/backport to release/6.0

[github-actions]

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1886417132