Merge pull request #29 from QuantConnect/fix-memory-leak-finalizer-v2

Fix memory leak finalizer

Author: jaredbroad

.bumpversion.cfg

@@ -1,5 +1,5 @@
 [bumpversion]
-current_version = 1.0.5.19
+current_version = 1.0.5.20
 parse = (?P<major>\d+)\.(?P<minor>\d+)\.(?P<patch>\d+)(\.(?P<release>[a-z]+)(?P<dev>\d+))?
 serialize = 
 	{major}.{minor}.{patch}.{release}{dev}

conda.recipe/meta.yaml

@@ -1,6 +1,6 @@
 package:
   name: pythonnet
-  version: "1.0.5.19"
+  version: "1.0.5.20"
 
 build:
   skip: True  # [not win]

setup.py

@@ -485,7 +485,7 @@ def run(self):
 
 setup(
     name="pythonnet",
-    version="1.0.5.19",
+    version="1.0.5.20",
     description=".Net and Mono integration for Python",
     url='https://pythonnet.github.io/',
     license='MIT',

src/SharedAssemblyInfo.cs

@@ -25,4 +25,4 @@
 // Version Information. Keeping it simple. May need to revisit for Nuget
 // See: https://codingforsmarties.wordpress.com/2016/01/21/how-to-version-assemblies-destined-for-nuget/
 // AssemblyVersion can only be numeric
-[assembly: AssemblyVersion("1.0.5.19")]
+[assembly: AssemblyVersion("1.0.5.20")]

src/clrmodule/ClrModule.cs

@@ -53,7 +53,7 @@ public static void initclr()
         {
 #if USE_PYTHON_RUNTIME_VERSION
             // Has no effect until SNK works. Keep updated anyways.
-            Version = new Version("1.0.5.19"),
+            Version = new Version("1.0.5.20"),
 #endif
             CultureInfo = CultureInfo.InvariantCulture
         };

src/embed_tests/TestFinalizer.cs

@@ -36,16 +36,18 @@ public void CollectBasicObject()
         {
             Assert.IsTrue(Finalizer.Instance.Enable);
 
-            int thId = Thread.CurrentThread.ManagedThreadId;
             Finalizer.Instance.Threshold = 1;
             bool called = false;
+            var objectCount = 0;
             EventHandler<Finalizer.CollectArgs> handler = (s, e) =>
             {
-                Assert.AreEqual(thId, Thread.CurrentThread.ManagedThreadId);
-                Assert.GreaterOrEqual(e.ObjectCount, 1);
+                objectCount = e.ObjectCount;
                 called = true;
             };
 
+            Assert.IsFalse(called);
+            Finalizer.Instance.CollectOnce += handler;
+
             WeakReference shortWeak;
             WeakReference longWeak;
             {
@@ -61,18 +63,16 @@ public void CollectBasicObject()
                 Assert.NotZero(garbage.Count);
                 Assert.IsTrue(garbage.Any(T => ReferenceEquals(T.Target, longWeak.Target)));
             }
-
-            Assert.IsFalse(called);
-            Finalizer.Instance.CollectOnce += handler;
             try
             {
-                Finalizer.Instance.CallPendingFinalizers();
+                Finalizer.Instance.Collect(forceDispose: false);
             }
             finally
             {
                 Finalizer.Instance.CollectOnce -= handler;
             }
             Assert.IsTrue(called);
+            Assert.GreaterOrEqual(objectCount, 1);
         }
 
         private static void MakeAGarbage(out WeakReference shortWeak, out WeakReference longWeak)
@@ -85,7 +85,7 @@ private static void MakeAGarbage(out WeakReference shortWeak, out WeakReference
 
         private static long CompareWithFinalizerOn(PyObject pyCollect, bool enbale)
         {
-            // Must larger than 512 bytes make sure Python use 
+            // Must larger than 512 bytes make sure Python use
             string str = new string('1', 1024);
             Finalizer.Instance.Enable = true;
             FullGCCollect();
@@ -164,10 +164,11 @@ internal static void CreateMyPyObject(IntPtr op)
         public void ErrorHandling()
         {
             bool called = false;
+            var errorMessage = "";
             EventHandler<Finalizer.ErrorArgs> handleFunc = (sender, args) =>
             {
                 called = true;
-                Assert.AreEqual(args.Error.Message, "MyPyObject");
+                errorMessage = args.Error.Message;
             };
             Finalizer.Instance.Threshold = 1;
             Finalizer.Instance.ErrorHandler += handleFunc;
@@ -193,6 +194,7 @@ public void ErrorHandling()
             {
                 Finalizer.Instance.ErrorHandler -= handleFunc;
             }
+            Assert.AreEqual(errorMessage, "MyPyObject");
         }
 
         [Test]

src/runtime/finalizer.cs

@@ -1,10 +1,9 @@
 using System;
 using System.Collections.Concurrent;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.Linq;
-using System.Runtime.InteropServices;
 using System.Threading;
+using System.Threading.Tasks;
 
 namespace Python.Runtime
 {
@@ -28,20 +27,10 @@ public class ErrorArgs : EventArgs
         public int Threshold { get; set; }
         public bool Enable { get; set; }
 
-        [StructLayout(LayoutKind.Sequential, CharSet = CharSet.Ansi)]
-        struct PendingArgs
-        {
-            public bool cancelled;
-        }
-
-        [UnmanagedFunctionPointer(CallingConvention.Cdecl)]
-        private delegate int PendingCall(IntPtr arg);
-        private readonly PendingCall _collectAction;
-
         private ConcurrentQueue<IPyDisposable> _objQueue = new ConcurrentQueue<IPyDisposable>();
         private bool _pending = false;
         private readonly object _collectingLock = new object();
-        private IntPtr _pendingArgs = IntPtr.Zero;
+        private Task _finalizerTask;
 
         #region FINALIZER_CHECK
 
@@ -84,23 +73,23 @@ private Finalizer()
         {
             Enable = true;
             Threshold = 200;
-            _collectAction = OnPendingCollect;
         }
 
-        public void CallPendingFinalizers()
+        public void Collect(bool forceDispose = true)
         {
-            if (Thread.CurrentThread.ManagedThreadId != Runtime.MainManagedThreadId)
+            if (Instance._finalizerTask != null
+                && !Instance._finalizerTask.IsCompleted)
             {
-                throw new Exception("PendingCall should execute in main Python thread");
+                using (Py.GIL())
+                {
+                    var ts = PythonEngine.BeginAllowThreads();
+                    Instance._finalizerTask.Wait();
+                    PythonEngine.EndAllowThreads(ts);
+                }
             }
-            Runtime.Py_MakePendingCalls();
-        }
-
-        public void Collect()
-        {
-            using (var gilState = new Py.GILState())
+            else if (forceDispose)
             {
-                DisposeAll();
+                Instance.DisposeAll();
             }
         }
 
@@ -141,25 +130,7 @@ internal static void Shutdown()
                 Instance._objQueue = new ConcurrentQueue<IPyDisposable>();
                 return;
             }
-            Instance.DisposeAll();
-            if (Thread.CurrentThread.ManagedThreadId != Runtime.MainManagedThreadId)
-            {
-                if (Instance._pendingArgs == IntPtr.Zero)
-                {
-                    Instance.ResetPending();
-                    return;
-                }
-                // Not in main thread just cancel the pending operation to avoid error in different domain
-                // It will make a memory leak
-                unsafe
-                {
-                    PendingArgs* args = (PendingArgs*)Instance._pendingArgs;
-                    args->cancelled = true;
-                }
-                Instance.ResetPending();
-                return;
-            }
-            Instance.CallPendingFinalizers();
+            Instance.Collect(forceDispose: true);
         }
 
         private void AddPendingCollect()
@@ -171,16 +142,14 @@ private void AddPendingCollect()
                     if (!_pending)
                     {
                         _pending = true;
-                        var args = new PendingArgs { cancelled = false };
-                        _pendingArgs = Marshal.AllocHGlobal(Marshal.SizeOf(typeof(PendingArgs)));
-                        Marshal.StructureToPtr(args, _pendingArgs, false);
-                        IntPtr func = Marshal.GetFunctionPointerForDelegate(_collectAction);
-                        if (Runtime.Py_AddPendingCall(func, _pendingArgs) != 0)
+                        // should already be complete but just in case
+                        _finalizerTask?.Wait();
+
+                        _finalizerTask = Task.Factory.StartNew(() =>
                         {
-                            // Full queue, append next time
-                            FreePendingArgs();
+                            Instance.DisposeAll();
                             _pending = false;
-                        }
+                        });
                     }
                 }
                 finally
@@ -190,29 +159,6 @@ private void AddPendingCollect()
             }
         }
 
-        private static int OnPendingCollect(IntPtr arg)
-        {
-            Debug.Assert(arg == Instance._pendingArgs);
-            try
-            {
-                unsafe
-                {
-                    PendingArgs* pendingArgs = (PendingArgs*)arg;
-                    if (pendingArgs->cancelled)
-                    {
-                        return 0;
-                    }
-                }
-                Instance.DisposeAll();
-            }
-            finally
-            {
-                Instance.FreePendingArgs();
-                Instance.ResetPending();
-            }
-            return 0;
-        }
-
         private void DisposeAll()
         {
             CollectOnce?.Invoke(this, new CollectArgs()
@@ -223,46 +169,32 @@ private void DisposeAll()
             lock (_queueLock)
 #endif
             {
+                using (Py.GIL())
+                {
 #if FINALIZER_CHECK
-                ValidateRefCount();
+                    ValidateRefCount();
 #endif
-                IPyDisposable obj;
-                while (_objQueue.TryDequeue(out obj))
-                {
-                    try
-                    {
-                        obj.Dispose();
-                        Runtime.CheckExceptionOccurred();
-                    }
-                    catch (Exception e)
+                    IPyDisposable obj;
+                    while (_objQueue.TryDequeue(out obj))
                     {
-                        // We should not bother the main thread
-                        ErrorHandler?.Invoke(this, new ErrorArgs()
+                        try
                         {
-                            Error = e
-                        });
+                            obj.Dispose();
+                            Runtime.CheckExceptionOccurred();
+                        }
+                        catch (Exception e)
+                        {
+                            // We should not bother the main thread
+                            ErrorHandler?.Invoke(this, new ErrorArgs()
+                            {
+                                Error = e
+                            });
+                        }
                     }
                 }
             }
         }
 
-        private void FreePendingArgs()
-        {
-            if (_pendingArgs != IntPtr.Zero)
-            {
-                Marshal.FreeHGlobal(_pendingArgs);
-                _pendingArgs = IntPtr.Zero;
-            }
-        }
-
-        private void ResetPending()
-        {
-            lock (_collectingLock)
-            {
-                _pending = false;
-            }
-        }
-
 #if FINALIZER_CHECK
         private void ValidateRefCount()
         {

src/runtime/resources/clr.py

@@ -2,7 +2,7 @@
 Code in this module gets loaded into the main clr module.
 """
 
-__version__ = "1.0.5.19"
+__version__ = "1.0.5.20"
 
 
 class clrproperty(object):