After using FileStream.SafeFileHandle, is it safe for me to remove a call to GC.KeepAlive()?

1.8k views Asked by At

Consider the following code which I recently changed to use FileStream.SafeFileHandle:

public static void FastWrite<T>(FileStream fs, T[] array, int offset, int count) where T: struct
{
    int sizeOfT = Marshal.SizeOf(typeof(T));
    GCHandle gcHandle = GCHandle.Alloc(array, GCHandleType.Pinned);

    try
    {
        uint bytesWritten;
        uint bytesToWrite = (uint)(count * sizeOfT);
        var overlapped = new NativeOverlapped();

        if
        (
            !WriteFile
            (
                fs.SafeFileHandle,
                new IntPtr(gcHandle.AddrOfPinnedObject().ToInt64() + (offset*sizeOfT)),
                bytesToWrite,
                out bytesWritten,
                ref overlapped
            )
        )
        {
            throw new IOException("Unable to write file.", new Win32Exception(Marshal.GetLastWin32Error()));
        }

        Debug.Assert(bytesWritten == bytesToWrite);

        GC.KeepAlive(fs); // <--- Is this really not necessary?
    }

    finally
    {
        gcHandle.Free();
    }
}

[DllImport("kernel32.dll", SetLastError=true)]
[return: MarshalAs(UnmanagedType.Bool)]

private static extern bool WriteFile
(
    SafeFileHandle       hFile,
    IntPtr               lpBuffer,
    uint                 nNumberOfBytesToWrite,
    out uint             lpNumberOfBytesWritten,
    ref NativeOverlapped lpOverlapped
);

I had previously added the GC.KeepAlive(fs) to ensure that the FileStream won't be garbage-collected until after the Windows API WriteFile() call has returned.

However, after changing to use SafeFileHandle Code Analysis now tells me that it isn't necessary with warning CA2004: Remove calls to GC.KeepAlive:

If you are converting to SafeHandle usage, remove all calls to GC.KeepAlive (object).

I've consulted the documentation on FileStream.SafeFileHandle but it is unclear to me if it really is safe for me to remove the call to GC.KeepAlive().

Is it definitely safe to remove it? And am I using it correctly?

Also, can anyone point me at some decent documentation on the use of SafeHandle?

1

There are 1 answers

1
Hans Passant On BEST ANSWER

The point of using SafeHandle is that the handle won't be closed while the WriteFile() function is executing. Which is really what you want to achieve here. Do note however that the FileStream object may still be finalized. There are no obvious consequences from that happening in the posted code. So the FxCop warning is appropriate.

Do note the strings attached to using code like this. It is very unlikely to be any faster than FileStream.Write(). But you do add risk by not properly dealing with edge conditions. Including using Overlapped but not appropriately handling overlapped I/O, don't do that. If overlapped I/O is actually intended then check this answer for the way that's optimized in the CLR beyond what GCHandle can do. Have a good look at the Reference Source source code for FileStream, particularly focus on the _isAsync field and the error handling for ERROR_NO_DATA and ERROR_INVALID_HANDLE.

And you'll also see it using SafeFileHandle and not using GC.KeepAlive(), like FxCop demands.