sharpdx / SharpDX

SharpDX GitHub Repository
http://sharpdx.org
MIT License
1.68k stars 642 forks source link

Finalizer releases last ref too early when EnableReleaseOnFinalizer is true #1106

Open MillerJames opened 5 years ago

MillerJames commented 5 years ago

I’ve found that with Configuration.EnableReleaseOnFinalizer set to true, I occasionally get an exception if the last use of a ComObject is passing it into a generated interop method. For example, when using SharpDX versions 3.1.1 or 4.2.0, the following call to d2drendertarget.FillGeometry occasionally throws an exception. I believe this is because the finalizer runs and decrements the ref count to zero before the native method acquires its own ref:

// Some managed method in my C# code
public void FillGeo(MyGeometryType geometry, D2D.Brush d2dbrush)
{
    D2D.Geometry d2dgeometry = geometry.CreateD2DGeometry();
    this.d2drendertarget.FillGeometry(d2dgeometry, d2dbrush);
}
// generated FillGeometry interop method in SharpDX (comments mine)
public unsafe void DrawGeometry(SharpDX.Direct2D1.Geometry geometry, SharpDX.Direct2D1.Brush brush,
    System.Single strokeWidth, SharpDX.Direct2D1.StrokeStyle strokeStyle)
{
    System.IntPtr geometry_ = System.IntPtr.Zero;
    System.IntPtr brush_ = System.IntPtr.Zero;
    System.IntPtr strokeStyle_ = System.IntPtr.Zero;
    geometry_ = SharpDX.CppObject.ToCallbackPtr<SharpDX.Direct2D1.Geometry>(geometry); 

    // At this point, “geometry” is eligible for finalization, its last use was in the above line. 
    // If the finalizer runs now, it will decrement the native Geometry object's ref count to zero
    brush_ = SharpDX.CppObject.ToCallbackPtr<SharpDX.Direct2D1.Brush>(brush);
    strokeStyle_ = SharpDX.CppObject.ToCallbackPtr<SharpDX.Direct2D1.StrokeStyle>(strokeStyle);

    // The native method called here will try to dereference "geometry_" but the native object it 
    // points at might have been destroyed (if the managed object's finalizer already ran)
    SharpDX.Direct2D1.LocalInterop.CalliStdCallvoid(this._nativePointer, (void *)geometry_, 
        (void *)brush_, strokeWidth, (void *)strokeStyle_, (*(void ***)this._nativePointer)[22]);
}

This case can be trivially resolved with a using block in my code, but it seems like the point of Configuration.EnableReleaseOnFinalizer is that explicitly disposing is not required. I'm working around this issue with using blocks and GC.KeepAlive for the cases I've found in my codebase, but resolving this more generally would be great.

Would it be reasonable to have SharpGen generate GC.KeepAlive calls to delay finalization for objects whose native pointer is passed into a native method call? i.e. the generated method would look something like:

public unsafe void DrawGeometry(SharpDX.Direct2D1.Geometry geometry, SharpDX.Direct2D1.Brush brush, System.Single strokeWidth, SharpDX.Direct2D1.StrokeStyle strokeStyle)
{
    System.IntPtr geometry_ = System.IntPtr.Zero;
    System.IntPtr brush_ = System.IntPtr.Zero;
    System.IntPtr strokeStyle_ = System.IntPtr.Zero;
    geometry_ = SharpDX.CppObject.ToCallbackPtr<SharpDX.Direct2D1.Geometry>(geometry);
    brush_ = SharpDX.CppObject.ToCallbackPtr<SharpDX.Direct2D1.Brush>(brush);
    strokeStyle_ = SharpDX.CppObject.ToCallbackPtr<SharpDX.Direct2D1.StrokeStyle>(strokeStyle);
    SharpDX.Direct2D1.LocalInterop.CalliStdCallvoid(this._nativePointer, (void *)geometry_, 
        (void *)brush_, strokeWidth, (void *)strokeStyle_, (*(void ***)this._nativePointer)[22]);

    GC.KeepAlive(this);
    GC.KeepAlive(geometry);
    GC.KeepAlive(brush);
    GC.KeepAlive(strokeStyle);
}
jkoritzinsky commented 5 years ago

Hey can you open an issue for this on the SharpGenTools repo? That's where the code generator lives. I'm busy with work right now but I'd definitely be interested in adding this into the generator.

MillerJames commented 5 years ago

Sure, I just opened CppObject finalized before native method is called in SharpGenTools, basically just copy-pasted this issue. Thanks!