Using a file opened in append mode will fail after a gc occurs

[elgonzo]

After a garbage collection occurs, (still active) FileStreams opened in append mode with AlphaFS will cease to function properly.

Specifically, after a gc, trying to read or write from a FileStream opened in append mode will throw an ObjectDisposedException. (Depending on whether there is any buffering involved or not, the exception might not necessarily occur already on the first read/write operation after the gc.)

Example code triggering the bug:

var dummyBuffer = new byte[128];
using (var s = Alphaleonis.Win32.Filesystem.File.Open(@"x:\testfile.dat", System.IO.FileMode.Append))
{
    for (int i=0; i < 1000; ++i)
    {
        s.Write(dummyBuffer, 0, dummyBuffer.Length);
        GC.Collect();  // <--- after the forced garbage collection, one of the subsequent calls to s.Write will throw an ObjectDisposedException
    }
}

Cause of the problem

The offender is the method File.CreateFileCore, in particular these code lines:

if (isAppend)
    new FileStream(safeHandle, FileAccess.Write, NativeMethods.DefaultFileBufferSize, (attributes & ExtendedFileAttributes.Overlapped) != 0).Seek(0, SeekOrigin.End);

Note how a temporary FileStream object is used to execute a seek operation. However, this unreferenced temporary FileStream object will be finalized during the next gc. And herein lies the problem, because this temporary FileStream assumes full control (i.e., takes ownership) of the safeHandle!

This means, whenever this temporary FileStream is finalized by the gc, it will destroy the safeHandle. From that moment on, any other FileStream using that safeHandle (like the FileStream object returned by Alphaleonis.Win32.Filesystem.File.Open) will not be able to use it anymore, causing the aforementioned ObjectDisposedException whenever the FileStream implementation tries to do an I/O access with the safeHandle.

Possible solution

Do not use a temporary FileStream object to execute a Seek operation. Rather, use the Win32 API function SetFilePointer directly, in a similar fashion as FileStream.SeekCore does. (Perhaps it would also be a good idea to check your code for other temporary objects taking ownership of safeFileHandles and alike, as any of those could have a similar potential for such bugs...)

[Yomodo]

Thanks for the detailed report!

I could not get your unit test to fail.

[elgonzo]

There is one minor issue with your fix in File.CreateCore. :)

If SetPointerEx fails, you cannot just throw an exception. You should also close the file handle before throwing the exception. Otherwise the file handle would linger around until the next gc occurs (which could be a long long time...).

Do something like this (pseudo code!):

if (!success)
{
    safeHandle.Dispose(); // or perhaps NativeMethods.CloseHandle(safeHandle) if it is not a SafeHandle but just a native IntPtr handle...
    NativeError.ThrowException(lastError, path);
}

[Yomodo]

So true, fixed!

[elgonzo]

Okay, one more thing about the fix of the fix.

Unless i have overlooked something, your updated fix is a regression compared to your original fix. Sorry about that :(

In the File.CreateCore method, afer calling SetFilePointerEx, you have added the following check:

if (!success)
    NativeMethods.IsValidHandle(safeHandle, lastError, path);

which is useless as a response to a failed SetFilePointerEx call.

The earlier first call of NativeMethods.IsValidHandle made sure that the safeHandle is valid prior to calling SetFilePointerEx. Even if SetFilePointerEx fails, it will not change the validity of the safeHandle (whatever SetFilePointerEx is doing, it cannot alter the internal state of the SafeFileHandle object), nor does it close the underlying native handle.

Thus if (!success) NativeMethods.IsValidHandle(safeHandle, lastError, path); will do exactly nothing. This is bad, because it does not prevent File.Open (append mode) from returning a FileStream object even if SetFilePointerEx failed.

Your original fix was safer/better than this update. The only flaw was it just kept the handle around longer than necessary in an error case. What i suggested in my previous post was to close/dispose the safeHandle in a rather timely fashion.

[Yomodo]

it will not change the validity of the safeHandle

Argh, yes, I overlooked that.