Runtime: System.IO.Stream.Close() behavior issues

[jhudsoncedaron]

So normally you would think Dispose() does the same job including writing out the file to disk.

It turns out there is one extra case. Space for the file is not guaranteed to have been allocated until calling close() or CloseHandle() and may not in fact exist. This means that writing a file swallows knowledge of certain errors that should be thrown.

Close() must throw to catch write errors
Dispose() must _not_ throw to avoid problems with using() {} blocks. In effect Dispose() is { try { Cose(); } catch (IOException){}} (omitting certain details of the dispose pattern for clarity)

The code to replace a file safely was:

c# using (f = new System.IO.FileStream("name~", ...) { /\* write contents */ f.Close(); } System.IO.Move("name~", "name");

But now that doesn't work because Close() can't be there to error out. Adding it to just FileStream() doesn't help all that much. I have many occasions to do new MyStream1(new MyStream2(new GzipOutputStream(new FileStream(...))));

[svick]

I don't see what you're describing in reference source:

• Stream.Dispose() is not virtual and calls Close()
• Stream.Close() is virtual and calls Dispose(true)
• Stream.Dispose(bool) is virtual and empty
• FileStream does not override Close(), only Dispose(bool)

I believe this means that FileStream.Dispose() and FileStream.Close() will behave exactly the same.

[jhudsoncedaron]

Unfortunately this means only I've turned up an old design flat in .NET long before .NET Core. Which of the two states do you suppose we're in, the one where Dispose() throws on IO Error or the one where Close() doesn't?

[jhudsoncedaron]

I think I found the code in coreclr/safehandle.cpp; I'm not sure but I think it asserts on close() raising EIO, which in turn causes ReleaseHandle() to return false.

CLR_BOOL fReleaseHandle = FALSE;

DECLARE_ARGHOLDER_ARRAY(args, 1);
args[ARGNUM_0] = OBJECTREF_TO_ARGHOLDER(sh);

PREPARE_SIMPLE_VIRTUAL_CALLSITE_USING_SLOT(s_ReleaseHandleMethodSlot, sh);

CRITICAL_CALLSITE;
CALL_MANAGED_METHOD(fReleaseHandle, CLR_BOOL, args);

if (!fReleaseHandle) {

#ifdef MDA_SUPPORTED
        MDA_TRIGGER_ASSISTANT(ReleaseHandleFailed, ReportViolation(sh->GetTypeHandle(), sh->m_handle));
#endif
    }

[GSPP]

CloseHandle never fails. It never writes so the disk can't become full at that point.

Maybe you could call Flush(false) instead of Close?

[jhudsoncedaron]

@GSPP: You have a windows-specific mind. Let us try for man 2 close() we get: "Not checking the return value of close() is a common but nevertheless serious programming error. It is quite possible that errors on a previous write(2) operation are first reported at the final close(). Not checking the return value when closing the file may lead to silent loss of data."

Due to the rising popularity of dedicated NAS boxes, this behavior is now visible to applications running on Windows.

[GSPP]

Who knew? Interesting design. Makes it hard to write RAII-style APIs I guess. Thanks for pointing that out.

[jhudsoncedaron]

In fact you have seen RAII with this structure before just not the same semantics.

using(IDBTransaction t = connection.BeginTransaction())
{
//...
t.Commit();
}

[ghost]

dotnet/corefx#10038 added Close() method to Stream type.

[jhudsoncedaron]

@jasonwilliams200OK : Code commit does not address actual issue. A safe handle's failure to close is _swallowed_ by coreclr/src/vm/safehandle.cpp function void SafeHandle::RunReleaseMethod(SafeHandle* psh). (MDA_TRIGGER_ASSISTANT doesn't do anything when no debugger attached).

[danmosemsft]

@jhudsoncedaron If I understand correctly, there is nothing that can be done in CoreFX here. We need the behavior of Close() and Dispose() to match Desktop. @stephentoub ?

[jhudsoncedaron]

@danmosemsft : You have a DATA LOSS bug. You should be fixing it in Desktop as well. This went so long uncovered because it does not normally occur on Windows (in fact cannot occur on any Windows native filesystem) so you won't break people by fixing it in Desktop.

[stephentoub]

@jkotas, do you have an opinion here on SafeHandle swalling failures when ReleaseHandle returns false?

[jkotas]

The return value from SafeHandle.ReleaseHandle does not seem to be very useful because of it is used just for MDA reporting on desktop and it does not capture any details about the error. I do not know why it was designed this way.

The error handling in cases like this one is done via exception handling. If there is a data loss, it should be reported via exception thrown from the place where it happened. It means throw the exception from the SafeHandle.ReleaseHandle override. There should not be anything fundamental preventing SafeHandle.ReleaseHandle overrides from throwing exceptions (except best practices in documentation that were written with desktop CLR hosted inside SQL Server process in mind).

My suggestion would be to fix this by throwing exception from SafeFileHandle.ReleaseHandle override.

The interesting question is whether the exception should be thrown unconditionally, or only during graceful disposal (Dispose called with disposing=true). The current FileStream stream implementation is flushing the stream unconditionally in Dispose method, even when called during finalization (see https://github.com/dotnet/corefx/blob/master/src/System.IO.FileSystem/src/System/IO/Win32FileStream.cs#L433). It would suggest to throw the exception from SafeHandle.ReleaseHandle override unconditionally as well.

We need the behavior of Close() and Dispose() to match Desktop

Match desktop != bug for bug compatible with desktop.

If this fix were to made in desktop as well, it would need to be done under a compat switch there.

[zabulus]

It would suggest to throw the exception from SafeHandle.ReleaseHandle override unconditionally as well.

If it throws an exception during finalization isn't this lead to a program termination?
From MSDN docs: https://msdn.microsoft.com/en-us/library/system.object.finalize(v=vs.110).aspx#Notes

There is one action that your implementation of Finalize should never take: it should never throw an exception.

[jkotas]

Right - like any other unhandled exception.

[jhudsoncedaron]

@zabulus : Which is why I wrote that it should be swallowed on Dispose() but thrown on Close(). However I consider a writing file handle leaked all the way to Finalize() to be a deranged state.

[jhudsoncedaron]

This is getting pretty sad. This still isn't fixed in 2.0 and I've ran into other scenarios where Close() needs to exist but doesn't. I've had to resort to throwing in Dispose() as of yesterday.

[jhudsoncedaron]

So this is very interesting. The source for https://github.com/dotnet/coreclr/blob/master/src/mscorlib/src/System/IO/Stream.cs shows a public virtual void Close() but when I try to override it the IDE says NO.

[svick]

@jhudsoncedaron Overriding Stream.Close() works fine for me on .Net Core 2.0 (and on .Net Standard 2.0 too). If it doesn't work for you, can you share a minimal repro (including your csproj) and what error exactly you are getting?

[jhudsoncedaron]

Scratch that. It's nuget doing something dumb again. Adding an explicit reference to System.IO allowed Close() to resolve.

[karelz]

Is this issue still tracking something that needs to happen? Is it still rightfully Design Discussion?
cc @JeremyKuhne

[jhudsoncedaron]

I haven't been able to verify that a close() on Linux that fails due to lack of disk space propagates as a thrown exception. I think it doesn't yet as FileStream doesn't override Close().

[JeremyKuhne]

I'm open to updating FileStream to throw when it can't close the handle successfully when it isn't being finalized. @jhudsoncedaron, is this something you're interested in making a PR for? Otherwise we can mark this as up-for-grabs.

[JeremyKuhne]

Note that if we make this change I would want to make sure we don't throw if we double-dispose and the first dispose was successful in flushing/closing the handle.

[jhudsoncedaron]

@JeremyKuhne Sorry can't do it. It requires changing coreclr.

As for double-dispose; I'm not worried if it doesn't throw in Dispose() at all so long as the handful of stream-wrappers are fixed to chain Close() correctly (I think there's only DeflateOutputStream, TLSStream, and StreamWriter--we note that readers will not throw on Close()). On the other hand the native API is clearly defined to not have that problem. When close() returns -EIO the handle is closed anyway so trying again must be prevented by SafeHandle.

[JeremyKuhne]

It requires changing coreclr.

Do you mean the native part of the runtime? (e.g. C++) I was thinking that we could just manually call SafeFileHandle.SetHandleAsInvalid() and CloseHandle() the handle ourselves. If it fails we could get the last error for the thread- it looks like the result is placed there in the PAL, but I'm not positive I'm following the call chain correctly. @jkotas Am I misunderstanding how this works or is this idea otherwise suboptimal?

[jkotas]

Yes, that sounds about right. But it still requires cloning and building coreclr repo to test it.

[jkotas]

Actually, it may be a bit more involved if you want to handle the case where some other thread has a pending operation on the handle.

[joshudson]

I have been significantly powering up over the past years. I can build and modify and run tests on the native code in corefx. I think I will be able to attack this problem in the next few months.

I will need to be able to depend on the Linux test harness being able to build and run a native binary used only for testing. Fundamentally, this is a ptrace() problem to synthesize a test case.

[joshudson]

Put up pull request for fix. I chickened out and made the Unix behavior match the current Windows behavior of both Close() and Dispose() generate IO exceptions on buffer flush. The behavior can now be toggled for both by inheriting from FileStream and overriding Close() and Dispose().

[joshudson]

There's more fix coming. Right now this interacts badly with Process.Start

[joshudson]

Ok so now I've got the whole fix up. Would someone please do something about these spurious build failures. There's no possible chance that 3 of the 4 pull requests (including test case fixes) failed Windows builds but I'm showing some Windows builds.

I can't integrate the test though. :(

[arunjvs]

@karelz @danmosemsft: I see this as a fundamentals and file corruption bug. Can we get traction on this please?

[danmosemsft]

Hi @arjunvs. Is this something you believe you've hit in production, or just a general proactive concern?

[arunjvs]

We've hit corruption (silent truncation) on customer data while copying from a custom stream to a filestream. But here's a lame "cp" example that repros this:

using System;
using System.IO;

namespace ConsoleApp
{
    class Program
    {
        // dotnet run ~/largefile ~/largefile2 4194304
        static int Main(string[] args)
        {
            try
            {
                using (Stream source = new FileStream(args[0], FileMode.Open, FileAccess.Read))
                {
                    using (Stream dest = new FileStream(args[1], FileMode.Create, FileAccess.Write))
                    {
                        source.CopyTo(dest, Convert.ToInt32(args[3]));
                    }
                }
            }
            catch (Exception ex)
            {
                // If FS got full on Windows before copy could complete, we arrive here with ex as IOException and know that the copy hasn't succeeded.
                Console.WriteLine(ex);
                return ex.HResult;
            }

            // If FS got full on Linux before copy could complete, we arrive here with no exception as though copy succceded.
            // But file is actually truncated to the nearest FS block boundary. That is, it is corrupt.
            // Happens when close() fails instead of write() when we are just 1 or a few blocks away from FS full.

            return 0;
        }
    }
}

I cannot agree more with @jhudsoncedaron and the POSIX man 2 close page:

@GSPP: ... "Not checking the return value of close() is a common but nevertheless serious programming error. It is quite possible that errors on a previous write(2) operation are first reported at the final close(). Not checking the return value when closing the file may lead to silent loss of data."

[danmosemsft]

@arjunvs thanks for the info and the example. @carlossanlop could you please take a look at this?

We would also want to consider whether the fix would be safe to service for.

[carlossanlop]

@joshudson your coreclr PR https://github.com/dotnet/coreclr/pull/24792 had to be closed in December because we migrated both coreclr and corefx to the new dotnet/runtime repo. Would you be able to reopen your PR in the new repo?

[jhudsoncedaron]

@carlossanlop : There are two things that need to be dealt with.

1) Need API review changing

     public class System.IO.Stream {
          public void Close();
      }

to

    public class System.IO.Stream {
        virtual public void Close();
    }

2) The other half of the change in corefx was rejected outright.

[carlossanlop]

@jhudsoncedaron @joshudson
The Stream.Close method is already virtual:

runtime/src/libraries/System.Runtime/ref/System.Runtime.cs#L5305

I think you meant making the Dispose method virtual (based on the changes originally introduced in the PR dotnet/coreclr#24792).

But our documentation states this this is not the recommended pattern to use:

_The IDisposable interface requires the implementation of a single parameterless method, Dispose._
_However, the dispose pattern requires two Dispose methods to be implemented:_

• _A public non-virtual IDisposable.Dispose implementation that has no parameters._
• _A protected virtual Dispose method whose signature is:_

    protected virtual void Dispose(bool disposing)

Making an anti-pattern change in such an important class like Stream would have to have a very good reason to be approved. Do you mind explaining why you would need to make that change?

Re-quoting @JeremyKuhne 's comment above:

_I'm open to updating FileStream to throw when it can't close the handle successfully when it isn't being finalized._

It sounds to me that this change can be made inside the Dispose (bool disposing) method, without having to modify public method signatures.

1. _The other half of the change in corefx was rejected outright._

That was because it is not clear what that change was trying to accomplish.

[jhudsoncedaron]

My apologies. I got Close() and Dispose() mixed up.

That was because it is not clear what that change was trying to accomplish.

close() and fork() fight. In particular, O_CLOEXEC is the equivalent of throwing out the result of close(), which is the bug here.

[carlossanlop]

@jhudsoncedaron
We would still need to justify the reasoning to make Dispose virtual so we can bring this up in the API Proposal meeting. Here are the instructions for the API Review process.

Here is a good example of as strong API proposal. Can you please use this as a template to describe your proposed API change?

Alternatively, do you think this could be fixed without changing the existing public API signatures and instead making all the changes inside Dispose (bool disposing) and Close?

[jhudsoncedaron]

@carlossanlop : Both behaviors are actually pretty bad. I've got code in a local project that had do do class MyStream : Stream, IDisposable because the behavior of finishing writing to the stream and the behavior of bailing out of the block with a thrown exception ought to be different. In fact this came up on a stream that's backed by a database. You don't want to commit the database transaction if the code that's doing the writing throws. While re-implementing IDisposable works, it's bug prone, and even StreamWriter gets it wrong.

The same is true here. If you're already throwing, you probably don't want to replace that error message with the IO error message. It's likely to be less useful.

I did what was necessary in my employer's codebase. I don't want to repeat the hazard in NET Core itself.

I could in fact be wrong here. Maybe always throwing on IO error in Dispose() is better. There's two basic behaviors to choose from. Either using (var fs = new FileStream(...)) fs.Write(...); doesn't throw when it should or using (var fs = new FileStream(...)) { fs.Write(...); throw new Exception("I'm a runaway job"); } gets replaced with out of disk sometimes.

[joshudson]

We have a choice to make. We can't pass on an API review, because the meaning of the API has to change, even if the surface area appears to not change.

We need to make one of the following changes:

---

#### Proposed API

    namespace System.IO {
        public class Stream : IDisposable {
                public void Dispose() => Dispose(true);
                protected virtual Dispose(bool disposing) { if (disposing) Close(); }
                public virtual void Close() => Dispose(true);
                public virtual ValueTask DisposeAsync();
                public virtual ValueTask CloseAsync() => DisposeAsync();
         }
    }

Public Function Description

    public void Dispose();

This function provides IDisposable.Dispose. This function tells the Stream implementation to clean up resources when unwinding from a finally block. This function always calls Dispose(true);

    protected virtual void Dispose(bool disposing);

This function enables derived classes of Stream to flush buffers and clean up resources when being disposed. It should avoid throwing any exceptions.

See CA1065 for why Dispose() should not throw.

    public virtual ValueTask DisposeAsync();

Like Dispose() but asynchronous. Exceptions should not be thrown from here either.

     public virtual void Close();

This function enables derived classes of Stream to flush buffers and clean up resources when being closed by user (programmer) action. Buffers should be flushed, backing database transactions should be committed, etc. Only then should the native resources be torn down. Any errors on saving data should propagate.

    public virtual ValueTask CloseAsync();

Like Close(), but asynchronously. Unfortunately I had to make this call DisposeAsync() for backwards compatibility.

The following code will still work:

       using (var writer = new StreamWriter(new System.IO.Compression.GZipWriter(new FIleStream(...))))
       {
           writer.Write(...);
           writer.Close();
        }

and

        using (var stream new FileStream(...))
        {
             while (...)
             {
                     while (...)
                     {
                           if (...)
                               throw new ApplicationException(string.Format("Runaway job detected, parameter XYZ = {0}, ABC = {1}", ...));
                     }
                     stream.Write(...);
             }
             stream.Close();
        }

The following code already swallowed errors on Linux, Mac OS, or Windows when backed by a set-top NAS device. It will now have the same behavior on Windows with a local hard disk.

       using (var writer = new StreamWriter(new System.IO.Compression.GZipWriter(new FIleStream(...))))
       {
           writer.Write(...);
        }

Some third party Stream implementations should be adjusted because the rules changed. If they don't, they just throw out of Dispose(). I anticipate most of them won't do it and it won't be much issue. The ZipStream providing libraries are the ones that really need to care as they really do write more stuff in Dispose() right now and that really would cause surprises on unwind. On the other hand, Kestrel Server would benefit. There's no point flushing the output buffer once the RAZR page throws an exception.

---

### Proposed API

    namespace System.IO {
        public class Stream : IDisposable {
                public void Dispose() => Close();
                public void Close() => Dispose(true);
                public virtual void Close();
                public virtual ValueTask DisposeAsync();
         }
    }

Public Function Description

    public void Dispose();

This function provides IDisposable.Dispose. This function tells the Stream implementation to clean up resources when unwinding from a finally block. This function always calls Close();

    protected virtual void Close();

This function enables derived classes of Stream to flush buffers and clean up resources no matter how we got here. There is no way to determine whether or not this is a job-completed close or an abortive close. Exceptions should be thrown on IO error. CA1065 https://docs.microsoft.com/en-us/visualstudio/code-quality/ca1065-do-not-raise-exceptions-in-unexpected-locations?view=vs-2015&redirectedfrom=MSDN is thoroughly burned and nobody can depend on using blocks not replacing their exceptions on unwind.

    public virtual ValueTask DisposeAsync();

Like Dispose() but asynchronous. Exceptions should not be thrown from here either.

    public virtual ValueTask CloseAsync();

Like Close(), but asynchronously. Unfortunately I had to make this call DisposeAsync() by default for backwards compatibility.

The following code will work on Linux and Mac OSX as it did on Windows, which is a breaking change.

       using (var writer = new StreamWriter(new System.IO.Compression.GZipWriter(new FIleStream(...))))
       {
           writer.Write(...);
        }

The following code will never behave itself:

        using (var stream new FileStream(...))
        {
             while (...)
             {
                     while (...)
                     {
                           if (...)
                               throw new ApplicationException(string.Format("Runaway job detected, parameter XYZ = {0}, ABC = {1}", ...));
                     }
                     stream.Write(...);
             }
             stream.Close();
        }

The ApplicationException can get replaced with an IO exception, despite most likely being the cause of the IO exception and having the actual information required to debug it.

---

Whew. That was a long post. What happened here is the normal Dispose() pattern cannot apply to FileStream. Neither proposal implements it correctly. That's impossible. You have to decide which breakage you would rather live with. It would be better if the changes could be reviewed without reference to the existing Dispose() pattern and make the better choice without considering the pattern. Personally I do not like burning CA1065 and so would choose the first option for that reason alone. I don't care how much work it is to make the changes to framework, and I'm the one volunteering to do it.

[joshudson]

My original plan was to toggle behavior on whether or not to throw in Dispose() based on a derived class of FileStream; however FileStream.ReadAsync(Memory<byte>) and FileStream.WriteAsync(ReadOnlyMemory<byte>) hate it when you derive from FileStream.

If some aspect of this idea can be recovered, then we can say the proposal breaks nobody, which is something the second proposal cannot hope to achieve.

[karelz]

Triage: There seems to be mix up of several independent issues here. We need to break them out - @carlossanlop is going to help. Thanks!

[carlossanlop]

Let's focus this issue on the discussion of this comment that proposes making some Stream methods virtual and changing behaviors of Dispose and Close.

I opened a separate issue, specific for FileStream.Dispose, to address the problem reported by @arunjvs on Unix: https://github.com/dotnet/runtime/issues/1816

[joshudson]

Since nobody seems to be able to decide if CA1065 should stand or fall, I came up with an option 3.

enum FileOptions {
   //...
   DontThrowInDispose = 65536,
}

This allows calling code to be upgraded in stages, or perhaps not at all.

I've been waiting for somebody to actually be able to decide which path to take before I write one giant patch for handling of close(2) failing.