When GC.KeepAlive(this) is needed when doing P/Invoke on unmanaged resources?

Question

I have a TestNet wrapper for a native component. The native component exposes a blocking TestNative::Foo() that communicates with managed part through calling managed callbacks and a weak GCHandle that is used to retrieve the reference to the .NET wrapper and provides a context. The GCHandle is weak since the .NET wrapper is meant to hide the fact that is handling unmanaged resources to user and deliberately doesn't implement the IDisposable interface: being non weak it would prevent TestNet instances from being collected at all, creating a memory leak. What's happening is that in Release build only the garbage collector will collect reference to .NET wrapper while executing the managed callback, even before both TestNative::Foo() and surprisingly TestNet::Foo() unblocks. I understood the problem my self and I can fix it by issuing a GC.KeepAlive(this) after the P/Invoke call but since the knowledge of this is not very widespread, it seems a lot of people are doing it wrong. I have few questions:

1. Is GC.KeepAlive(this) always needed in a managed method if last instruction is a P/Invoke call on unmanaged resources or it's just needed in this special case, namely the switch to managed execution context while marshaling the managed callback from native code? The question could be: should I put GC.KeepAlive(this) everywhere? This old microsoft blog (original link is 404, here is cached) seems to suggest so! But this would be game changer and basically it would mean that most people never did P/Invoke correctly, because this would require reviewing most P/Invoke calls in wrappers. Is there for example a rule that say that garbage collector (EDIT: or better the finalizer) can't run for objects that belong to the current thread while execution context is unamanaged (native)?
2. Where I can find proper documentation? I could find CodeAnalysis policy CA2115 pointing to generically use GC.KeepAlive(this) any time a unmanaged resource is accessed with P/Invoke. In general GC.KeepAlive(this) seems to be very rarely needed when dealing with finalizers.
3. Why is this happening only in Release build? It looks like an optimization but not being needed at all in Debug build hides an important behavior of the garbage collector.

NOTE: I have no problem with delegates being collected, that is a different issue which I know how to handle properly. The issue here is with objects holding unmanaged resources being collected when P/Invoke calls are not finished yet.

It follows code that clearly manifest the problem. Creates a C# console application and a C++ Dll1 project and build them in Release mode:

Program.cs:

using System;
using System.Runtime.InteropServices;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            var test = new TestNet();
            try
            {
                test.Foo();
            }
            catch (Exception ex)
            {
                Console.WriteLine(ex);
            }
        }
    }

    class TestNet
    {
        [UnmanagedFunctionPointer(CallingConvention.Cdecl)]
        delegate void Callback(IntPtr data);

        static Callback _callback;

        IntPtr _nativeHandle;
        GCHandle _thisHandle;

        static TestNet()
        {
            // NOTE: Keep delegates references so they can be
            // stored persistently in unmanaged resources
            _callback = callback;
        }

        public TestNet()
        {
            _nativeHandle = CreateTestNative();

            // Keep a weak handle to self. Weak is necessary
            // to not prevent garbage collection of TestNet instances
            _thisHandle = GCHandle.Alloc(this, GCHandleType.Weak);

            TestNativeSetCallback(_nativeHandle, _callback, GCHandle.ToIntPtr(_thisHandle));
        }

        ~TestNet()
        {
            Console.WriteLine("this.~TestNet()");
            FreeTestNative(_nativeHandle);
            _thisHandle.Free();
        }

        public void Foo()
        {
            Console.WriteLine("this.Foo() begins");
            TestNativeFoo(_nativeHandle);

            // This is never printed when the object is collected!
            Console.WriteLine("this.Foo() ends");

            // Without the following GC.KeepAlive(this) call
            // in Release build the program will consistently collect
            // the object in callback() and crash on next iteration 
            //GC.KeepAlive(this);
        }

        static void callback(IntPtr data)
        {
            Console.WriteLine("TestNet.callback() begins");
            // Retrieve the weak reference to self. As soon as the istance
            // of TestNet exists. 
            var self = (TestNet)GCHandle.FromIntPtr(data).Target;
            self.callback();

            // Enforce garbage collection. On release build
            self = null;
            GC.Collect();
            GC.WaitForPendingFinalizers();
            Console.WriteLine("TestNet.callback() ends");
        }

        void callback()
        {
            Console.WriteLine("this.callback()");
        }

        [DllImport("Dll1", CallingConvention = CallingConvention.Cdecl)]
        static extern IntPtr CreateTestNative();

        [DllImport("Dll1", CallingConvention = CallingConvention.Cdecl)]
        static extern void FreeTestNative(IntPtr obj);

        [DllImport("Dll1", CallingConvention = CallingConvention.Cdecl)]
        static extern void TestNativeSetCallback(IntPtr obj, Callback callback, IntPtr data);

        [DllImport("Dll1", CallingConvention = CallingConvention.Cdecl)]
        static extern void TestNativeFoo(IntPtr obj);
    }
}

Dll1.cpp:

#include <iostream>

extern "C" typedef void (*Callback)(void *data);

class TestNative
{
public:
    void SetCallback(Callback callback1, void *data);
    void Foo();
private:
    Callback m_callback;
    void *m_data;
};

void TestNative::SetCallback(Callback callback, void * data)
{
    m_callback = callback;
    m_data = data;
}

void TestNative::Foo()
{
    // Foo() will never end
    while (true)
    {
        m_callback(m_data);
    }
}

extern "C"
{
    __declspec(dllexport) TestNative * CreateTestNative()
    {
        return new TestNative();
    }

    __declspec(dllexport) void FreeTestNative(TestNative *obj)
    {
        delete obj;
    }

    __declspec(dllexport) void TestNativeSetCallback(TestNative *obj, Callback callback1, void * data)
    {
        obj->SetCallback(callback1, data);
    }

    __declspec(dllexport) void TestNativeFoo(TestNative *obj)
    {
        obj->Foo();
    }
}

The output is consistently:

this.Foo() begins
TestNet.callback() begins
this.callback()
this.~TestNet()
TestNet.callback() ends
TestNet.callback() begins
System.NullReferenceException: Object reference not set to an instance of an object.

If one uncomment the GC.KeepAlive(this) call in TestNet.Foo() the program correctly never ends.

Dairymaid

To add a bit of context information, I was observing this problem in a real world application crashing on native code with calls to _purecall followed by _abort, which actually meant that the wrapper holding unmanaged resources was finalized externally. – Dairymaid

https://mcmap.net/q/1176259/-where-do-i-put-gc-keepalive? – Alvarez

@Alvarez unfortunately that answer is not really relevant to my question. The issue with delegates being garbage collected is very well known and in my code I'm in the exact situation described as when delegates must be stored, as I'm properly doing in my example code. My issue is about object holding unmanaged resources being collected when P/Invoke call is not finished yet. – Dairymaid

devblogs.microsoft.com/oldnewthing/20100810-00/?p=13193? – Alvarez

@Alvarez yes! That article is extremely relevant to this question and points to CA2115 that I already found! So yes, it clearly seems that the object can be marked for GC collection anytime after there is really no use of the object reference anymore, even in the middle of P/Invoke call if there's a concurrent GC strategy. Still the question would: can also the finalizer for that object be called while in the current thread there's execution of native code? I think this is not possible: it "happens" in my code since the execution context already switched to managed invoking the callback. – Dairymaid

The problem is not Debug vs Release, its optimized vs non optimized. Check Optimize Code in Debug and you should see the same problem: #37462878 and https://mcmap.net/q/30331/-understanding-garbage-collection-in-net. Anyway, what you do seems very complicated. Why don't you use the IDisposable standard and member callback, for example like this: pastebin.com/raw/z0eL1iQf – Latialatices

@SimonMourier I don't implement IDisposable pattern because I don't hold file handles, sockets or other resources for which it's recommended to use IDisposable: it's just a C++ native API and C# users shouldn't be forced to rely on using. Anyway: I just realized that GC.KeepAlive() API documents exactly my use case with callbacks, basically answering my second question. Your answer perfectly covers question 3). Not yet answered question is whenever GC.KeepAlive(this) is always needed or not – Dairymaid

@SimonMourier anyway, interesting technique with instance members delegates: that would simplify my code, of course, not needing GCHandle anymore. I didn't know it could also work this way. It doesn't seems to prevent possible garbage collection of the managed instance during the blocking native call, though (that's why you still suggested me IDisposable, I guess). – Dairymaid

IDisposable is for unmanaged resources. Your _nativeHandle IntPtr is typically an unmanaged resource since it's allocated from the unmanaged world (like a registry key handle for example). For what it's worth, in 20 years of .NET COM and interop I never had the need for GC.KeepAlive. – Latialatices

@SimonMourier yes, I understand IDisposable is generically for unmanaged resources of any kind. In my case these unmanaged resources are just C++ objects for which deletion can happen non-deterministically, and don't have any resource that works with open/close logic (for example registry handles, as you say, that would be unsafe to close non deterministically in the general case). IDisposable could be added anyway, but it would be hard to maintain in a way that API users can arbitrarily dispose objects without corrupting others. I tried that way, but people were just misusing the pattern. – Dairymaid

@SimonMourier ok, I tested the garbage collector and there are definitely situations where it will collect instances of managed wrappers even before returning from P/Invoke calls and not related to call of managed callbacks, as it was in the main question. Look at this example – Dairymaid

codeproject.com/Articles/29534/… – Unorganized

@DaveM thank you but implementing the IDisposable pattern is really not the solution to early collection of objects as observed in the provided example. – Dairymaid

Did you read the whole article? – Unorganized

@DaveM well, no... but if you meant the SafeHandle part, which I discovered in last hour, you could have pointed it out ;) . But really:thank you for suggestions, It's just that I am coming to conclusions. I found very interesting the HandleRef, which is less used nowadays but it's not deprecated and present in .NET standard 2.0 as well. They are both techniques to solve managed objects lifecycle when doing Interop. – Dairymaid

Answer 1

Summarizing very useful comments and research done:

1) Is GC.KeepAlive(this) always needed in a managed instance method if last instruction is a P/Invoke call using unmanaged resources hold by the instance?

Yes, if you don't want the user of the API to have last responsibility of holding a non-collectible reference for the instance of the managed object in pathological cases, look the example below. But it's not the only way: HandleRef or SafeHandle techiniques can also be used to prolong the lifetime of a managed object when doing P/Invoke Interop.

The example will subsequently call native methods through managed instances holding native resources:

using System;
using System.Diagnostics;
using System.Runtime.InteropServices;
using System.Threading;

namespace ConsoleApp1
{
    class Program
    {
        static void Main(string[] args)
        {
            new Thread(delegate()
            {
                // Run a separate thread enforcing GC collections every second
                while(true)
                {
                    GC.Collect();
                    Thread.Sleep(1000);
                }
            }).Start();

            while (true)
            {
                var test = new TestNet();
                test.Foo();
                TestNet.Dump();
            }
        }
    }

    class TestNet
    {
        static ManualResetEvent _closed;
        static long _closeTime;
        static long _fooEndTime;

        IntPtr _nativeHandle;

        public TestNet()
        {
            _closed = new ManualResetEvent(false);
            _closeTime = -1;
            _fooEndTime = -1;
            _nativeHandle = CreateTestNative();
        }

        public static void Dump()
        {
            // Ensure the now the object will now be garbage collected
            GC.Collect();
            GC.WaitForPendingFinalizers();

            // Wait for current object to be garbage collected
            _closed.WaitOne();
            Trace.Assert(_closeTime != -1);
            Trace.Assert(_fooEndTime != -1);
            if (_closeTime <= _fooEndTime)
                Console.WriteLine("WARN: Finalize() commenced before Foo() return");
            else
                Console.WriteLine("Finalize() commenced after Foo() return");
        }

        ~TestNet()
        {
            _closeTime = Stopwatch.GetTimestamp();
            FreeTestNative(_nativeHandle);
            _closed.Set();
        }

        public void Foo()
        {
            // The native implementation just sleeps for 250ms
            TestNativeFoo(_nativeHandle);

            // Uncomment to have all Finalize() to commence after Foo()
            //GC.KeepAlive(this);
            _fooEndTime = Stopwatch.GetTimestamp();
        }

        [DllImport("Dll1", CallingConvention = CallingConvention.Cdecl)]
        static extern IntPtr CreateTestNative();

        [DllImport("Dll1", CallingConvention = CallingConvention.Cdecl)]
        static extern void FreeTestNative(IntPtr obj);

        [DllImport("Dll1", CallingConvention = CallingConvention.Cdecl)]
        static extern void TestNativeFoo(IntPtr obj);
    }
}

For the native call to be always safe we expect finalizer to be called only after Foo() return. Instead we can easily enforce violations by manually invoking garbage collection in a background thread. Output follows:

Finalize() commenced after Foo() return
WARN: Finalize() commenced before Foo() return
Finalize() commenced after Foo() return
Finalize() commenced after Foo() return
Finalize() commenced after Foo() return
WARN: Finalize() commenced before Foo() return
Finalize() commenced after Foo() return

2) Where I can find documentation?

Documentation of GC.KeepAlive() provides an example very similar to the managed callback in the original question. HandleRef has also very interesting considerations about lifecycle of managed objects and Interop:

If you use platform invoke to call a managed object, and the object is not referenced elsewhere after the platform invoke call, it is possible for the garbage collector to finalize the managed object. This action releases the resource and invalidates the handle, causing the platform invoke call to fail. Wrapping a handle with HandleRef guarantees that the managed object is not garbage collected until the platform invoke call completes.

Also link[1] found by @GSerg explains when an object is eligible for collection, pointing that this reference is not in the root set, allowing it to be collected also when instance method has not returned.

3) Why is this happening only in Release build?

It's an optimization and can happen also in Debug build, with optimization enabled, as pointed by @SimonMourier. It's not enabled by default also in Debug because it could prevent debugging of variables in the current method scope, as explained in these other answers.

[1] https://devblogs.microsoft.com/oldnewthing/20100810-00/?p=13193?

Dairymaid