C# wrapper can crash #295

New Issue

2016-12-21T14:21:37-08:00

itsff commented

2016-12-21 14:21:37 -08:00

(Migrated from github.com)

It looks like the C# wrapper is not following "best practices" of calling native code. I think you are just getting lucky that the code is not crashing on you (likely since the reference is long-lived).

Please use GC.KeepAlive() to ensure the GC doesn't finalize YodaNode object while you are within a native function call. Some background:
https://goo.gl/WeL3H4
https://msdn.microsoft.com/en-us/library/ms182300.aspx
https://blogs.msdn.microsoft.com/oldnewthing/20100813-00/?p=13153

I would also suggest creating a light-weight "handle" class whose sole purpose would be holding the native IntPtr and finalization of it. It is a poor practice to have a finalizable object that's holding many managed references. Why? Because now they cannot be collected until the owning object has fully died (as they could potentially be needed by its finalizer method). Literature and a much deeper explanation is available in this excellent book by Marcus Heege.

It looks like the C# wrapper is not following "best practices" of calling native code. I think you are just getting lucky that the code is not crashing on you (likely since the reference is long-lived). Please use **GC.KeepAlive()** to ensure the GC doesn't finalize YodaNode object while you are within a native function call. Some background: https://goo.gl/WeL3H4 https://msdn.microsoft.com/en-us/library/ms182300.aspx https://blogs.msdn.microsoft.com/oldnewthing/20100813-00/?p=13153 I would also suggest creating a light-weight "handle" class whose sole purpose would be holding the native IntPtr and finalization of it. It is a poor practice to have a finalizable object that's holding many managed references. Why? Because now they cannot be collected until the owning object has fully died (as they could potentially be needed by its finalizer method). Literature and a much deeper explanation is available in this [excellent book by Marcus Heege](https://books.google.com/books?id=wZoQyVi5f60C&lpg=PA266&ots=Eh2Xb7EeuD&dq=marcus%20heege%20graph%20promotion%20problem&pg=PA266#v=onepage&q=marcus%20heege%20graph%20promotion%20problem&f=false).

splhack commented

2016-12-21 15:10:57 -08:00

(Migrated from github.com)

@itsff Can you share a concrete example of the scenario that we need to call GC.KeepAlive()? I think all managed objects already live longer than the associated unmanaged objects.

@itsff Can you share a concrete example of the scenario that we need to call `GC.KeepAlive()`? I think all managed objects already live longer than the associated unmanaged objects.

woehrl01 commented

2016-12-22 00:29:26 -08:00

(Migrated from github.com)

@splhack I just read through most of the provided links and did some research. It seems the following is true if you run this outside of visual studio

If if you are inside a object method the finalizer can be run:

YogaNode node = new YogaNode();
node.Width = 10;
node.CalculateLayout();
//GC.KeepAlive(node);

That means, it could be that the node gets finalized inside the CalculateLayout() function, if you don't use node afterwards (GC.KeepAlive would force keeping the referince till after the end of the call).
Which means the memory gets freed while the calculation runs.

The suggest fix is to use SafeHandle instead of IntPtras the interop marshaler handles this in a special way to ensure that this doesn't get freed inside an interop call.

I think the following should work, do you agree @itsff ?

    internal class YogaNodeHandle : SafeHandleZeroOrMinusOneIsInvalid
    {
        private YogaNodeHandle()
            : base(true)
        {
        }

        [ReliabilityContract(Consistency.WillNotCorruptState, Cer.MayFail)]
        protected override bool ReleaseHandle()
        {
            Native.YGNodeFree(handle);
            return true;
        }

    }

and change the Native.cs like, e.g.:

        [DllImport(DllName)]
        public static extern YogaNodeHandle YGNodeNew();

        [DllImport(DllName)]
        public static extern void YGNodeFree(IntPtr node);

        [DllImport(DllName)]
        public static extern void YGNodeCopyStyle(YogaNodeHandle dstNode, YogaNodeHandle srcNode);

and removing the Finalizer from YogaNode.

@splhack I just read through most of the provided links and did some research. It seems the following is true if you run this outside of visual studio If if you are inside a object method the finalizer can be run: ```csharp YogaNode node = new YogaNode(); node.Width = 10; node.CalculateLayout(); //GC.KeepAlive(node); ``` That means, it could be that the node gets finalized inside the CalculateLayout() function, if you don't use ```node``` afterwards (```GC.KeepAlive``` would force keeping the referince till after the end of the call). Which means the memory gets freed while the calculation runs. The suggest fix is to use ```SafeHandle``` instead of ```IntPtr```as the interop marshaler handles this in a special way to ensure that this doesn't get freed inside an interop call. I think the following should work, do you agree @itsff ? ```csharp internal class YogaNodeHandle : SafeHandleZeroOrMinusOneIsInvalid { private YogaNodeHandle() : base(true) { } [ReliabilityContract(Consistency.WillNotCorruptState, Cer.MayFail)] protected override bool ReleaseHandle() { Native.YGNodeFree(handle); return true; } } ``` and change the ```Native.cs``` like, e.g.: ```csharp [DllImport(DllName)] public static extern YogaNodeHandle YGNodeNew(); [DllImport(DllName)] public static extern void YGNodeFree(IntPtr node); [DllImport(DllName)] public static extern void YGNodeCopyStyle(YogaNodeHandle dstNode, YogaNodeHandle srcNode); ``` and removing the Finalizer from ```YogaNode```.

👍 1

itsff commented

2016-12-22 06:06:55 -08:00

(Migrated from github.com)

Well done, @woehrl01.
Moving to a dedicated SafeHandle class should address both of my concerns. First and foremost, the automatic marshaller will ensure the SafeHandle isn't disposed while the library is making a P/Invoke call. Second, moving away from having a finalizer in YogaNode will allow managed objects referenced by YogaNode to be GCed earlier, thus improving overall efficiency.

To add some background to our discussion: C# is one of those languages where a lifetime of an object can (and often is) much shorter than the "scope" it is declared in. The CLR will mark objects eligible for collection as soon as they are no longer used. As @woehrl01 pointed out in his comment, 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. To prevent this from happening, there are basically two options: either call GC.KeepAlive(obj) to mark the object as still in use, or (as @woehrl01 suggested) switch to using a SafeHandle so that the framework can reference count your native calls.

Well done, @woehrl01. Moving to a dedicated `SafeHandle` class should address both of my concerns. First and foremost, the automatic marshaller will ensure the `SafeHandle` isn't disposed while the library is making a P/Invoke call. Second, moving away from having a finalizer in `YogaNode` will allow managed objects referenced by `YogaNode` to be GCed earlier, thus improving overall efficiency. To add some background to our discussion: C# is one of those languages where a lifetime of an object can (and often is) much shorter than the "scope" it is declared in. The CLR will mark objects eligible for collection as soon as they are no longer used. As @woehrl01 pointed out in his comment, 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. To prevent this from happening, there are basically two options: either call `GC.KeepAlive(obj)` to mark the object as still in use, or (as @woehrl01 suggested) switch to using a `SafeHandle` so that the framework can reference count your native calls.

emilsjolander commented

2016-12-22 06:13:09 -08:00

(Migrated from github.com)

@woehrl01 or @itsff could you open a PR with these changes. Great find!

woehrl01 commented

2016-12-22 07:01:33 -08:00

(Migrated from github.com)

@itsff cool, I leave it up to you. I don't mind doing the PR. Don't want to heist some "credits" so. 😄

itsff commented

2016-12-22 07:16:26 -08:00

(Migrated from github.com)

Sounds good, gentlemen. I will do a PR tonight. @woehrl01 @emilsjolander

❤️ 1

splhack commented

2016-12-22 08:22:22 -08:00

(Migrated from github.com)

It seems Microsoft.Win32.SafeHandles is available on all Unity platform https://github.com/OSVR/Managed-OSVR/pull/10#r31332441

It seems `Microsoft.Win32.SafeHandles` is available on all Unity platform https://github.com/OSVR/Managed-OSVR/pull/10#r31332441

matthargett commented

2016-12-22 14:12:44 -08:00

(Migrated from github.com)

Please use this NuGet package so the behavior is correctly polyfilled across all NETstandard platforms:
https://www.nuget.org/packages/System.Runtime.Handles/4.3.0

Please use this NuGet package so the behavior is correctly polyfilled across all NETstandard platforms: https://www.nuget.org/packages/System.Runtime.Handles/4.3.0

itsff commented

2016-12-23 10:08:22 -08:00

(Migrated from github.com)

Resolved by PR #297

👍 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#295