P/Invoke, Pinning, and KeepAlive Best Practices

Question

At work we have a native C code responsible for reading and writing to a proprietary flat file database. I have a wrapper written in C# that encapsulates the P/Invoke calls into an OO model. The managed wrappers for the P/Invoke calls have grown in complexity considerably since the project was started. Anecdotally the current wrapper is doing fine, however, I'm thinking that I actually need to do more to ensure correct operation.

A couple of notes brought up by the answers:

1. Probably don't need the KeepAlive
2. Probably don't need the GCHandle pinning
3. If you do use the GCHandle, try...finally that business (CER questions not addressed though)

Here is an example of the revised code:

[DllImport(@"somedll", EntryPoint="ADD", CharSet=CharSet.Ansi,
           ThrowOnUnmappableChar=true, BestFitMapping=false,
           SetLastError=false)]
[ReliabilityContract(Consistency.MayCorruptProcess, Cer.None)]
internal static extern void ADD(
    [In] ref Int32 id,
    [In] [MarshalAs(UnmanagedType.LPStr)] string key,
    [In] byte[] data, // formerly IntPtr
    [In] [MarshalAs(UnmanagedType.LPArray, SizeConst=10)] Int32[] details,
    [In] [MarshalAs(UnmanagedType.LPArray, SizeConst=2)] Int32[] status);

public void Add(FileId file, string key, TypedBuffer buffer)
{
    // ...Arguments get checked

    int[] status = new int[2] { 0, 0 };
    int[] details = new int[10];

    // ...Make the details array

    lock (OPERATION_LOCK)
    {
        ADD(file.Id, key, buffer.GetBytes(), details, status);
        // the byte[], details, and status should be auto
        // pinned/keepalive'd

        if ((status[0] != 0) || (status[1] != 0))
            throw new OurDatabaseException(file, key, status);

        // we no longer KeepAlive the data because it should be auto
        // pinned we DO however KeepAlive our 'file' object since 
        // we're passing it the Id property which will not preserve
        // a reference to 'file' the exception getting thrown 
        // kinda preserves it, but being explicit won't hurt us
        GC.KeepAlive(file);
    }
}

My (revised) questions are:

1. Will data, details, and status be auto-pinned/KeepAlive'd?
2. Have I missed anything else required for this to operate correctly?

EDIT: I recently found a diagram which is what sparked my curiosity. It basically states that once you call a P/Invoke method the GC can preempt your native code. So while the native call may be made synchronously, the GC could choose to run and move/remove my memory. I guess now I'm wondering if automatic pinning is sufficient (or if it even runs).

Answer 1

One immediate problem seems to be that you will never call dataHandle.Free() if you throw your exception, causing a leak.

Answer 2

Unless your unmanaged code is directly manipulating the memory, I don't think you need to pin the object. Pinning essentially informs the GC that it should not move that object around in memory during the compact phase of a collection cycle. This is only important for unmanaged memory access where the unmanaged code is expecting the data to always be in the same location it was when it was passed in. The "mode" the GC operates in (concurrent or preemptive) should have no impact on pinned objects as the behavioral rules of pinning apply in either mode. The marshalling infrastructure in .NET attempts to be smart about how it marshals the data between managed/unmanaged code. In this specific case, the two arrays you are creating will be pinned automatically during the marshalling process.

The call to GC.KeepAlive probably isn't needed as well unless your unmanaged ADD method is asynchronous. GC.KeepAlive is only intended to prevent the GC from reclaiming an object that it thinks is dead during a long running operation. Since file is passed in as a parameter, it is presumably used elsewhere in the code after the call to the managed Add function, so there is no need for the GC.KeepAlive call.

You edited your code sample and removed the calls to GCHandle.Alloc() and Free(), so does that imply the code no longer uses those? If you are still using it, the code inside your lock(OPERATION_LOCK) block should also be wrapped in a try/finally block. In your finally block, you probably want to do something like this:

if (dataHandle.IsAllocated)
{
   dataHandle.Free();
}

Also, you may want to verify that the call GCHandle.Alloc() shouldn't be inside your lock. By having it ouside the lock you will have multiple threads doing allocating memory.

As far as automatic pinning, if the data is automatically pinned during the marshalling process, it is pinned and it won't be moved during a GC collection cycle if one were to occur while your unmanaged code is running. I'm not sure I fully understand your code comment about the reasoning for continuing to call GC.KeepAlive. Does the unamanged code actually set a value for the file.Id field?

Answer 3

1. I'm not sure what the point of your KeepAlive is, since you've already freed teh GCHandle - it seems that the data is no longer needed at that point?
2. Similar to #1, why do you feel you need to call KeepAlive at all? Is tehre something outside of the code you've posted we're not seeing?
3. Probably not. If this is a synchronous P/Invoke then the marshaler will actually pin the incoming variables until it returns. In fact you probably don't need to pin data either (unless this is async, but your construct suggests it's not).
4. No, nothing missed. I think you've actually added more than you need.

EDIT in response to original question edits and comments:

The diagram simply shows that the GC mode changes, The mode has no effect on pinned objects. Types are either pinned or copied during marshaling, depending on the type. In this case you're using a byte array, which the docs say is a blittable type. You'll see that it also specifically states that "As an optimization, arrays of blittable types and classes that contain only blittable members are pinned instead of copied during marshaling." So that means that data is pinned for the duration of the call, and if the GC runs, it is not able to move or free the array. Same is true for status.

The string passed is slightly different, the string data is copied and the pointer is passed on the stack. This behavior also makes it immune to collection and compaction. The GC can't touch the copy (it knows nothing about it) and the pointer is on the stack, with the GC doesn't affect.

I still don't see the point of calling KeepAlive. The file, presumably, isn't available for collection because it got passed in to the method and has some other root (where it was declared) that would keep it alive.

Answer 4

read Best Practices For Managed And Native Code Interoperability and use PInvoke Interop Assistant