Potential crash due to finalization race condition

[itsff]

The Item class has a ~ finalizer which cleans up the native resource. The finalizer will get invoked on a different thread after the Item object instance has been garbage collected. A potential problem can arise when running in release mode, where the runtime employs more aggressive optimizations. Unlike in C/C++ where objects get disposed when they go out of scope, in .NET objects are eligible for garbage collection when they are no longer utilized - no matter the scope (example: a big array is used only at the beginning of a method, so why wait until method exits before GCing the array?).

A contrived example:

void someMethod ()
{
    var xyz = new Item();
    xyz.Height = 100; 
    // ^^^^^ This can crash while we're in Item::flex_item_set_height(item, value);
}

In the example above, the xyz instance becomes eligible for GC when it's no longer needed, which is right after its this pointer got referenced for reading the value of item within Item::Height::set. So it's possible the finalizer thread will invoke ~Item while the flex_item_set_height is still executing. Obviously not a good thing.

Two choices here:

1. Continue using the ~ finalizer and rely on GC.KeepAlive(this) to keep your Item instance alive until you're done calling native functions. https://goo.gl/WeL3H4 https://msdn.microsoft.com/en-us/library/ms182300.aspx https://blogs.msdn.microsoft.com/oldnewthing/20100813-00/?p=13153

2. Ditch the ~ finalizer and switch to SafeHandles. Instead of using IntPtr derive from SafeHandle (or from SafeHandleZeroOrMinusOneIsInvalid) and move the destruction logic to SafeHandle::ReleaseHandle. The runtime treats SafeHandles in a special way, so they won't be finalized while the native code is being executed.

[itsff]

FYI: https://github.com/facebook/yoga/issues/295

[phrohdoh]

Pardon my ignorance here but how is the this ptr for xyz "no longer needed" when calling an instance method (the property setter)?

Edit:

Your comment in the yoga issue seems to answer my question:

the last time the node object is being referenced is within the body of YogaNode::CalculateLayout when its private IntPtr member variable is read. So node can be destroyed by the GC while the execution is still within YogaNode::CalculateLayout right before the invocation of the native static function. YogaNode's finalizer will kick in on its own thread, and in turn delete the native memory being used by the native function.

[itsff]

Yep, essentially that. The this pointer is only needed to resolve the member variable. It's one of the challenges doing mixed (native/managed) development - the runtime is only aware of half of the equation and we need to help it along. The SafeHandle approach should work fine in this case (as it did with yoga) with minimum code changes.

[lrz]

Thanks for filing this issue. As of master we are now following the IDisposable pattern, so I'm closing this ticket as I believe the problem is no longer present.

[itsff]

Removing the ~ finalizer does fix the potential of crash due to a race condition, but now there is a possibility of a memory leak. What happens if user creates a new Item but forgets to call Dispose (most .NET developers do this)? The native memory will never be reclaimed...