Finalizer implementation improvement. PythonException memory leak fix…

…. Review fixes.
pythonnet · dmitriyse · Aug 31, 2017 · Sep 2, 2017 · Sep 2, 2017 · Sep 2, 2017
commit 80ff7549daf5ae530f80b6eac5f49565b61be2b7
diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs
@@ -64,5 +64,67 @@ public void TestClrObjectFullRelease()
 
             Assert.IsFalse(weakRef.IsAlive, "Clr object should be collected."); 
         }
+
+        [Test]
+        [Ignore("For debug only")]
+        public void TestExceptionMemoryLeak()
+        {
+            dynamic pymodule;
+            PyObject gc;
+            dynamic testmethod;
+
+            var ts = PythonEngine.BeginAllowThreads();
+            IntPtr pylock = PythonEngine.AcquireLock();
+
+#if NETCOREAPP
+            const string s = "../../fixtures";
+#else
+            const string s = "../fixtures";
+#endif
+            string testPath = Path.Combine(TestContext.CurrentContext.TestDirectory, s);
+
+            IntPtr str = Runtime.Runtime.PyString_FromString(testPath);
+            IntPtr path = Runtime.Runtime.PySys_GetObject("path");
+            Runtime.Runtime.PyList_Append(path, str);
+
+            {
+                PyObject sys = PythonEngine.ImportModule("sys");
+                gc = PythonEngine.ImportModule("gc");
+
+                pymodule = PythonEngine.ImportModule("MemoryLeakTest.pyraise");
+                testmethod = pymodule.test_raise_exception;
+            }
+
+            PythonEngine.ReleaseLock(pylock);
+
+            double floatarg1 = 5.1f;
+            dynamic res = null;
+            {
+                for (int i = 1; i <= 10000000; i++)
+                {
+                    using (Py.GIL())
+                    {
+                        try
+                        {
+                            res = testmethod(Py.kw("number", floatarg1), Py.kw("astring", "bbc"));
+                        }
+                        catch (Exception e)
+                        {
+                            if (i % 10000 == 0)
+                            {
+                                TestContext.Progress.WriteLine(e.Message);
+                            }
+                        }
+                    }
+
+                    if (i % 10000 == 0)
+                    {
+                        GC.Collect();
+                    }
+                }
+            }
+
+            PythonEngine.EndAllowThreads(ts);
+        }
     }
 }
diff --git a/src/embed_tests/fixtures/MemoryLeakTest/pyraise.py b/src/embed_tests/fixtures/MemoryLeakTest/pyraise.py
@@ -0,0 +1,8 @@
+def test_raise_exception(number=3, astring='abc'):
+    raise ValueError("testing for memory leak")
+    return astring * int(number)
+
+def test_raise_exception2(number, astring):
+    #raise ValueError("testing for memory leak")
+    #astring * int(number)
+    return "test" 
diff --git a/src/runtime/debughelper.cs b/src/runtime/debughelper.cs
@@ -116,10 +116,10 @@ internal static void debug(string msg)
             Console.WriteLine("  {0}", msg);
         }
 
-        /// <summary>
+        /// <summary>
         /// Helper function to inspect/compare managed to native conversions.
-        /// Especially useful when debugging CustomMarshaler.
-        /// </summary>
+        /// Especially useful when debugging CustomMarshaler.
+        /// </summary>
         /// <param name="bytes"></param>
         [Conditional("DEBUG")]
         public static void PrintHexBytes(byte[] bytes)

diff --git a/src/runtime/pyiter.cs b/src/runtime/pyiter.cs
@@ -40,14 +40,20 @@ public PyIter(PyObject iterable)
             }
         }
 
-        protected override void Dispose(bool disposing)
+        protected internal override void Dispose(bool disposing)
         {
-            if (null != _current)
+            try
             {
-                _current.Dispose();
-                _current = null;
+                if (null != _current)
+                {
+                    _current.Dispose(disposing);
+                    _current = null;
+                }
+            }
+            finally
+            {
+                base.Dispose(disposing);
             }
-            base.Dispose(disposing);
         }
 
         public bool MoveNext()

diff --git a/src/runtime/pyobject.cs b/src/runtime/pyobject.cs
@@ -132,39 +132,32 @@ public T As<T>()
         /// to Python objects will not be released until a managed garbage
         /// collection occurs.
         /// </remarks>
-        protected virtual void Dispose(bool disposing)
+        protected internal virtual void Dispose(bool disposing)
         {
             if (!disposed)
             {
                 disposed = true;
+                IntPtr objToDispose = obj;
+                obj = IntPtr.Zero;
 
                 if (disposing)
                 {
-                    try
+                    if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
                     {
-                        if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
+                        IntPtr gs = PythonEngine.AcquireLock();
+                        try
                         {
-                            IntPtr gs = PythonEngine.AcquireLock();
-                            try
-                            {
-                                Runtime.XDecref(obj);
-                                obj = IntPtr.Zero;
-                            }
-                            finally
-                            {
-                                PythonEngine.ReleaseLock(gs);
-                            }
+                            Runtime.XDecref(objToDispose);
+                        }
+                        finally
+                        {
+                            PythonEngine.ReleaseLock(gs);
                         }
-                    }
-                    catch
-                    {
-                        // Do nothing.
                     }
                 }
                 else
                 {
-                    _referenceDecrementer?.ScheduleDecRef(obj);
-                    obj = IntPtr.Zero;
+                    _referenceDecrementer?.ScheduleDecRef(objToDispose);
                 }
             }
         }

diff --git a/src/runtime/pyreferencedecrementer.cs b/src/runtime/pyreferencedecrementer.cs
@@ -7,6 +7,14 @@
 
 namespace Python.Runtime
 {
+    /// <summary>
+    /// Background python objects reference decrementer. 
+    /// !!! By some unknown reasons python references decrements have been performed in the GC thread - affects GC. !!!
+    /// This component contains background thread with the queue of IntPtr python object references. 
+    /// Finalizers should schedule reference decrement operation in this component to avoid problem with GC.
+    /// Each PythonEngine Init/Shutdown have it's own PyReferenceDecrementer, so PyObject should also have a field with the
+    /// reference to the correct PyReferenceDecrementer.
+    /// </summary>
     internal class PyReferenceDecrementer : IDisposable
     {
         private static readonly DedicatedThreadTaskScheduler DedicatedThreadTaskScheduler = new DedicatedThreadTaskScheduler();

diff --git a/src/runtime/pyscope.cs b/src/runtime/pyscope.cs
@@ -31,7 +31,7 @@ public class PyScope : DynamicObject, IDisposable
         /// <summary>
         /// the python Module object the scope associated with.
         /// </summary>
-        internal readonly IntPtr obj;
+        internal IntPtr obj;
 
         /// <summary>
         /// the variable dict of the scope.
@@ -522,34 +522,37 @@ protected virtual void Dispose(bool disposing)
         {
             if (!_isDisposed)
             {
-                _isDisposed = true;
-
+                IntPtr objToDispose = obj;
                 if (disposing)
                 {
                     try
                     {
-                        if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
-                        {
-                            IntPtr gs = PythonEngine.AcquireLock();
-                            try
-                            {
-                                Runtime.XDecref(obj);
-                            }
-                            finally
-                            {
-                                PythonEngine.ReleaseLock(gs);
-                            }
-                        }
+                        OnDispose?.Invoke(this);
+                    }
+                    finally
+                    {
+                        _isDisposed = true;
+                        obj = IntPtr.Zero;
                     }
-                    catch
+
+                    if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
                     {
-                        // Do nothing.
+                        IntPtr gs = PythonEngine.AcquireLock();
+                        try
+                        {
+                            Runtime.XDecref(objToDispose);
+                        }
+                        finally
+                        {
+                            PythonEngine.ReleaseLock(gs);
+                        }
                     }
-                    OnDispose?.Invoke(this);
                 }
                 else
                 {
-                    _referenceDecrementer?.ScheduleDecRef(obj);
+                    _isDisposed = true;
+                    obj = IntPtr.Zero;
+                    _referenceDecrementer?.ScheduleDecRef(objToDispose);
                 }
             }
         }

diff --git a/src/runtime/pythonengine.cs b/src/runtime/pythonengine.cs
@@ -598,7 +598,7 @@ internal GILState()
 
             protected virtual void Dispose(bool disposing)
             {
-                //ReleeaseLock is thread bound and if it's called in finalizer thread it can wrongly release lock.
+                //ReleaseLock is thread bound and if it's called in finalizer thread it can wrongly release lock.
                 if (disposing)
                 {
                     PythonEngine.ReleaseLock(state);

diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs
@@ -21,9 +21,11 @@ public PythonException()
             _referenceDecrementer = PythonEngine.CurrentRefDecrementer;
             IntPtr gs = PythonEngine.AcquireLock();
             Runtime.PyErr_Fetch(ref _pyType, ref _pyValue, ref _pyTB);
-            Runtime.XIncref(_pyType);
-            Runtime.XIncref(_pyValue);
-            Runtime.XIncref(_pyTB);
+
+            // Those references already owned by the caller
+            ////Runtime.XIncref(_pyType);
+            ////Runtime.XIncref(_pyValue);
+            ////Runtime.XIncref(_pyTB);
             if (_pyType != IntPtr.Zero && _pyValue != IntPtr.Zero)
             {
                 string type;
@@ -44,7 +46,13 @@ public PythonException()
             }
             if (_pyTB != IntPtr.Zero)
             {
-                PyObject tb_module = PythonEngine.ImportModule("traceback");
+
+                if (_tbModule == null)
+                {
+                    _tbModule = PythonEngine.ImportModule("traceback");
+                }
+                PyObject tb_module = _tbModule;
+
                 Runtime.XIncref(_pyTB);
                 using (var pyTB = new PyObject(_pyTB))
                 {
@@ -54,11 +62,14 @@ public PythonException()
             PythonEngine.ReleaseLock(gs);
         }
 
+        private static PyObject _tbModule;
+
         // Ensure that encapsulated Python objects are decref'ed appropriately
         // when the managed exception wrapper is garbage-collected.
 
         ~PythonException()
         {
+
             Dispose(false);
         }
 
@@ -152,49 +163,51 @@ protected void Dispose(bool disposing)
             {
                 disposed = true;
 
+                IntPtr pyTypeToDispose = _pyType;
+                _pyType = IntPtr.Zero;
+
+                IntPtr pyValueToDispose = _pyValue;
+                _pyValue = IntPtr.Zero;
+
+                IntPtr pyTBToDispose = _pyTB;
+                _pyTB = IntPtr.Zero;
+
                 if (disposing)
                 {
                     if (Runtime.Py_IsInitialized() > 0 && !Runtime.IsFinalizing)
                     {
+                        IntPtr gs = PythonEngine.AcquireLock();
                         try
                         {
-                            IntPtr gs = PythonEngine.AcquireLock();
-                            try
-                            {
-                                Runtime.XDecref(_pyType);
-                                Runtime.XDecref(_pyValue);
-                                // XXX Do we ever get TraceBack? //
-                                if (_pyTB != IntPtr.Zero)
-                                {
-                                    Runtime.XDecref(_pyTB);
-                                }
-                            }
-                            finally
+                            Runtime.XDecref(pyTypeToDispose);
+                            Runtime.XDecref(pyValueToDispose);
+                            // XXX Do we ever get TraceBack? //
+                            if (pyTBToDispose != IntPtr.Zero)
                             {
-                                PythonEngine.ReleaseLock(gs);
+                                Runtime.XDecref(pyTBToDispose);
                             }
                         }
-                        catch
+                        finally
                         {
-                            // Do nothing.
+                            PythonEngine.ReleaseLock(gs);
                         }
                     }
                 }
                 else
                 {
-                    if (_pyType != IntPtr.Zero)
+                    if (pyTypeToDispose != IntPtr.Zero)
                     {
-                        _referenceDecrementer?.ScheduleDecRef(_pyType);
+                        _referenceDecrementer?.ScheduleDecRef(pyTypeToDispose);
                     }
 
-                    if (_pyValue != IntPtr.Zero)
+                    if (pyValueToDispose != IntPtr.Zero)
                     {
-                        _referenceDecrementer?.ScheduleDecRef(_pyValue);
+                        _referenceDecrementer?.ScheduleDecRef(pyValueToDispose);
                     }
 
-                    if (_pyTB != IntPtr.Zero)
+                    if (pyTBToDispose != IntPtr.Zero)
                     {
-                        _referenceDecrementer?.ScheduleDecRef(_pyTB);
+                        _referenceDecrementer?.ScheduleDecRef(pyTBToDispose);
                     }
                 }
             }