From 8083f3b409ad99562cbe9914eb66c3964d1551eb Mon Sep 17 00:00:00 2001 From: amos402 Date: Sun, 24 Jun 2018 03:20:44 +0800 Subject: [PATCH 01/18] Finalizer for PyObject --- src/embed_tests/TestFinalizer.cs | 143 +++++++++++++++++++++++++++++++ src/runtime/delegatemanager.cs | 29 ++++--- src/runtime/finalizer.cs | 117 +++++++++++++++++++++++++ src/runtime/pyobject.cs | 14 +-- src/runtime/pyscope.cs | 12 +-- src/runtime/pythonexception.cs | 24 +++--- src/runtime/runtime.cs | 11 +++ 7 files changed, 319 insertions(+), 31 deletions(-) create mode 100644 src/embed_tests/TestFinalizer.cs create mode 100644 src/runtime/finalizer.cs diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs new file mode 100644 index 000000000..03e2f1be5 --- /dev/null +++ b/src/embed_tests/TestFinalizer.cs @@ -0,0 +1,143 @@ +using NUnit.Framework; +using Python.Runtime; +using System; +using System.Threading; + +namespace Python.EmbeddingTest +{ + public class TestFinalizer + { + private string _PYTHONMALLOC = string.Empty; + + [SetUp] + public void SetUp() + { + try + { + _PYTHONMALLOC = Environment.GetEnvironmentVariable("PYTHONMALLOC"); + } + catch (ArgumentNullException) + { + _PYTHONMALLOC = string.Empty; + } + Environment.SetEnvironmentVariable("PYTHONMALLOC", "malloc"); + PythonEngine.Initialize(); + } + + [TearDown] + public void TearDown() + { + PythonEngine.Shutdown(); + if (string.IsNullOrEmpty(_PYTHONMALLOC)) + { + Environment.SetEnvironmentVariable("PYTHONMALLOC", _PYTHONMALLOC); + } + } + + private static void FullGCCollect() + { + GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced); + GC.WaitForFullGCComplete(); + GC.WaitForPendingFinalizers(); + } + + [Test] + public void CollectBasicObject() + { + int thId = Thread.CurrentThread.ManagedThreadId; + Finalizer.Instance.Threshold = 1; + bool called = false; + EventHandler handler = (s, e) => + { + Assert.AreEqual(thId, Thread.CurrentThread.ManagedThreadId); + Assert.GreaterOrEqual(e.ObjectCount, 1); + called = true; + }; + Finalizer.Instance.CollectOnce += handler; + FullGCCollect(); + PyLong obj = new PyLong(1024); + + WeakReference shortWeak = new WeakReference(obj); + WeakReference longWeak = new WeakReference(obj, true); + obj = null; + FullGCCollect(); + // The object has been resurrected + Assert.IsFalse(shortWeak.IsAlive); + Assert.IsTrue(longWeak.IsAlive); + + Assert.IsFalse(called); + var garbage = Finalizer.Instance.GetCollectedObjects(); + // FIXME: If make some query for garbage, + // the above case will failed Assert.IsFalse(shortWeak.IsAlive) + //Assert.IsTrue(garbage.All(T => T.IsAlive)); + + Finalizer.Instance.CallPendingFinalizers(); + Assert.IsTrue(called); + + FullGCCollect(); + //Assert.IsFalse(garbage.All(T => T.IsAlive)); + + Assert.IsNull(longWeak.Target); + + Finalizer.Instance.CollectOnce -= handler; + } + + private static long CompareWithFinalizerOn(PyObject pyCollect, bool enbale) + { + // Must larger than 512 bytes make sure Python use + string str = new string('1', 1024); + Finalizer.Instance.Enable = true; + FullGCCollect(); + FullGCCollect(); + pyCollect.Invoke(); + Finalizer.Instance.CallPendingFinalizers(); + Finalizer.Instance.Enable = enbale; + + // Estimate unmanaged memory size + long before = Environment.WorkingSet - GC.GetTotalMemory(true); + for (int i = 0; i < 10000; i++) + { + // Memory will leak when disable Finalizer + new PyString(str); + } + FullGCCollect(); + FullGCCollect(); + pyCollect.Invoke(); + if (enbale) + { + Finalizer.Instance.CallPendingFinalizers(); + } + + FullGCCollect(); + FullGCCollect(); + long after = Environment.WorkingSet - GC.GetTotalMemory(true); + return after - before; + + } + + /// + /// Because of two vms both have their memory manager, + /// this test only prove the finalizer has take effect. + /// + [Test] + [Ignore("Too many uncertainties, only manual on when debugging")] + public void SimpleTestMemory() + { + bool oldState = Finalizer.Instance.Enable; + try + { + using (PyObject gcModule = PythonEngine.ImportModule("gc")) + using (PyObject pyCollect = gcModule.GetAttr("collect")) + { + long span1 = CompareWithFinalizerOn(pyCollect, false); + long span2 = CompareWithFinalizerOn(pyCollect, true); + Assert.Less(span2, span1); + } + } + finally + { + Finalizer.Instance.Enable = oldState; + } + } + } +} diff --git a/src/runtime/delegatemanager.cs b/src/runtime/delegatemanager.cs index 7632816d1..706bd4bc4 100644 --- a/src/runtime/delegatemanager.cs +++ b/src/runtime/delegatemanager.cs @@ -181,10 +181,12 @@ A possible alternate strategy would be to create custom subclasses too "special" for this to work. It would be more work, so for now the 80/20 rule applies :) */ - public class Dispatcher + public class Dispatcher : IDisposable { public IntPtr target; public Type dtype; + private bool _disposed = false; + private bool _finalized = false; public Dispatcher(IntPtr target, Type dtype) { @@ -195,18 +197,25 @@ public Dispatcher(IntPtr target, Type dtype) ~Dispatcher() { - // We needs to disable Finalizers until it's valid implementation. - // Current implementation can produce low probability floating bugs. - return; + if (_finalized || _disposed) + { + return; + } + _finalized = true; + Finalizer.Instance.AddFinalizedObject(this); + } - // Note: the managed GC thread can run and try to free one of - // these *after* the Python runtime has been finalized! - if (Runtime.Py_IsInitialized() > 0) + public void Dispose() + { + if (_disposed) { - IntPtr gs = PythonEngine.AcquireLock(); - Runtime.XDecref(target); - PythonEngine.ReleaseLock(gs); + return; } + _disposed = true; + Runtime.XDecref(target); + target = IntPtr.Zero; + dtype = null; + GC.SuppressFinalize(this); } public object Dispatch(ArrayList args) diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs new file mode 100644 index 000000000..344260f97 --- /dev/null +++ b/src/runtime/finalizer.cs @@ -0,0 +1,117 @@ +using System; +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Linq; +using System.Runtime.InteropServices; +using System.Threading; + +namespace Python.Runtime +{ + public class Finalizer + { + public class CollectArgs : EventArgs + { + public int ObjectCount { get; set; } + } + + public static readonly Finalizer Instance = new Finalizer(); + + public event EventHandler CollectOnce; + + private ConcurrentQueue _objQueue = new ConcurrentQueue(); + + [UnmanagedFunctionPointer(CallingConvention.Cdecl)] + private delegate int PedingCall(IntPtr arg); + private PedingCall _collectAction; + private bool _pending = false; + private readonly object _collectingLock = new object(); + public int Threshold { get; set; } + public bool Enable { get; set; } + + private Finalizer() + { + Enable = true; + Threshold = 200; + _collectAction = OnCollect; + } + + public void CallPendingFinalizers() + { + if (Thread.CurrentThread.ManagedThreadId != Runtime.MainManagedThreadId) + { + throw new Exception("PendingCall should execute in main Python thread"); + } + Runtime.Py_MakePendingCalls(); + } + + public List GetCollectedObjects() + { + return _objQueue.Select(T => new WeakReference(T)).ToList(); + } + + internal void AddFinalizedObject(IDisposable obj) + { + if (!Enable) + { + return; + } + if (Runtime.Py_IsInitialized() == 0) + { + // XXX: Memory will leak if a PyObject finalized after Python shutdown, + // for avoiding that case, user should call GC.Collect manual before shutdown. + return; + } + _objQueue.Enqueue(obj); + GC.ReRegisterForFinalize(obj); + if (_objQueue.Count >= Threshold) + { + Collect(); + } + } + + internal static void Shutdown() + { + Instance.DisposeAll(); + Instance.CallPendingFinalizers(); + Runtime.PyErr_Clear(); + } + + private void Collect() + { + lock (_collectingLock) + { + if (_pending) + { + return; + } + _pending = true; + } + IntPtr func = Marshal.GetFunctionPointerForDelegate(_collectAction); + if (Runtime.Py_AddPendingCall(func, IntPtr.Zero) != 0) + { + // Full queue, append next time + _pending = false; + } + } + + private int OnCollect(IntPtr arg) + { + CollectOnce?.Invoke(this, new CollectArgs() + { + ObjectCount = _objQueue.Count + }); + DisposeAll(); + _pending = false; + return 0; + } + + private void DisposeAll() + { + IDisposable obj; + while (_objQueue.TryDequeue(out obj)) + { + obj.Dispose(); + } + } + } +} diff --git a/src/runtime/pyobject.cs b/src/runtime/pyobject.cs index 0e075824a..e47908a76 100644 --- a/src/runtime/pyobject.cs +++ b/src/runtime/pyobject.cs @@ -17,6 +17,7 @@ public class PyObject : DynamicObject, IEnumerable, IDisposable { protected internal IntPtr obj = IntPtr.Zero; private bool disposed = false; + private bool _finalized = false; /// /// PyObject Constructor @@ -41,14 +42,15 @@ protected PyObject() // Ensure that encapsulated Python object is decref'ed appropriately // when the managed wrapper is garbage-collected. - ~PyObject() { - // We needs to disable Finalizers until it's valid implementation. - // Current implementation can produce low probability floating bugs. - return; - - Dispose(); + if (_finalized || disposed) + { + return; + } + // Prevent a infinity loop by calling GC.WaitForPendingFinalizers + _finalized = true; + Finalizer.Instance.AddFinalizedObject(this); } diff --git a/src/runtime/pyscope.cs b/src/runtime/pyscope.cs index 67f93c6e2..32d9626bd 100644 --- a/src/runtime/pyscope.cs +++ b/src/runtime/pyscope.cs @@ -37,6 +37,7 @@ public class PyScope : DynamicObject, IDisposable internal readonly IntPtr variables; private bool _isDisposed; + private bool _finalized = false; /// /// The Manager this scope associated with. @@ -527,11 +528,12 @@ public void Dispose() ~PyScope() { - // We needs to disable Finalizers until it's valid implementation. - // Current implementation can produce low probability floating bugs. - return; - - Dispose(); + if (_finalized || _isDisposed) + { + return; + } + _finalized = true; + Finalizer.Instance.AddFinalizedObject(this); } } diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs index ded7fbeb5..1b166854a 100644 --- a/src/runtime/pythonexception.cs +++ b/src/runtime/pythonexception.cs @@ -6,7 +6,7 @@ namespace Python.Runtime /// Provides a managed interface to exceptions thrown by the Python /// runtime. /// - public class PythonException : System.Exception + public class PythonException : System.Exception, IDisposable { private IntPtr _pyType = IntPtr.Zero; private IntPtr _pyValue = IntPtr.Zero; @@ -15,6 +15,7 @@ public class PythonException : System.Exception private string _message = ""; private string _pythonTypeName = ""; private bool disposed = false; + private bool _finalized = false; public PythonException() { @@ -45,11 +46,13 @@ public PythonException() } if (_pyTB != IntPtr.Zero) { - PyObject tb_module = PythonEngine.ImportModule("traceback"); - Runtime.XIncref(_pyTB); - using (var pyTB = new PyObject(_pyTB)) + using (PyObject tb_module = PythonEngine.ImportModule("traceback")) { - _tb = tb_module.InvokeMethod("format_tb", pyTB).ToString(); + Runtime.XIncref(_pyTB); + using (var pyTB = new PyObject(_pyTB)) + { + _tb = tb_module.InvokeMethod("format_tb", pyTB).ToString(); + } } } PythonEngine.ReleaseLock(gs); @@ -60,11 +63,12 @@ public PythonException() ~PythonException() { - // We needs to disable Finalizers until it's valid implementation. - // Current implementation can produce low probability floating bugs. - return; - - Dispose(); + if (_finalized || disposed) + { + return; + } + _finalized = true; + Finalizer.Instance.AddFinalizedObject(this); } /// diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs index b08a56622..46e25354b 100644 --- a/src/runtime/runtime.cs +++ b/src/runtime/runtime.cs @@ -2,6 +2,7 @@ using System.Runtime.InteropServices; using System.Security; using System.Text; +using System.Threading; namespace Python.Runtime { @@ -198,6 +199,8 @@ public class Runtime internal static bool IsPython2 = pyversionnumber < 30; internal static bool IsPython3 = pyversionnumber >= 30; + public static int MainManagedThreadId { get; internal set; } + /// /// Encoding to use to convert Unicode to/from Managed to Native /// @@ -211,6 +214,7 @@ internal static void Initialize() if (Py_IsInitialized() == 0) { Py_Initialize(); + MainManagedThreadId = Thread.CurrentThread.ManagedThreadId; } if (PyEval_ThreadsInitialized() == 0) @@ -350,6 +354,7 @@ internal static void Initialize() internal static void Shutdown() { + Finalizer.Shutdown(); AssemblyManager.Shutdown(); Exceptions.Shutdown(); ImportHook.Shutdown(); @@ -1654,5 +1659,11 @@ internal static bool PyObject_TypeCheck(IntPtr ob, IntPtr tp) [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] internal static extern IntPtr PyMethod_Function(IntPtr ob); + + [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] + internal static extern int Py_AddPendingCall(IntPtr func, IntPtr arg); + + [DllImport(_PythonDll, CallingConvention = CallingConvention.Cdecl)] + internal static extern int Py_MakePendingCalls(); } } From af33e749d16f8ba382de8dcee17c516f4fec80ff Mon Sep 17 00:00:00 2001 From: amos402 Date: Sun, 24 Jun 2018 15:40:14 +0800 Subject: [PATCH 02/18] Avoid test interdependency --- src/embed_tests/TestPyScope.cs | 101 ++++++++++++++++++--------------- 1 file changed, 55 insertions(+), 46 deletions(-) diff --git a/src/embed_tests/TestPyScope.cs b/src/embed_tests/TestPyScope.cs index 49c15a3a1..21c0d2b3f 100644 --- a/src/embed_tests/TestPyScope.cs +++ b/src/embed_tests/TestPyScope.cs @@ -293,24 +293,27 @@ public void TestImportScopeByName() [Test] public void TestVariables() { - (ps.Variables() as dynamic)["ee"] = new PyInt(200); - var a0 = ps.Get("ee"); - Assert.AreEqual(200, a0); + using (Py.GIL()) + { + (ps.Variables() as dynamic)["ee"] = new PyInt(200); + var a0 = ps.Get("ee"); + Assert.AreEqual(200, a0); - ps.Exec("locals()['ee'] = 210"); - var a1 = ps.Get("ee"); - Assert.AreEqual(210, a1); + ps.Exec("locals()['ee'] = 210"); + var a1 = ps.Get("ee"); + Assert.AreEqual(210, a1); - ps.Exec("globals()['ee'] = 220"); - var a2 = ps.Get("ee"); - Assert.AreEqual(220, a2); + ps.Exec("globals()['ee'] = 220"); + var a2 = ps.Get("ee"); + Assert.AreEqual(220, a2); - using (var item = ps.Variables()) - { - item["ee"] = new PyInt(230); + using (var item = ps.Variables()) + { + item["ee"] = new PyInt(230); + } + var a3 = ps.Get("ee"); + Assert.AreEqual(230, a3); } - var a3 = ps.Get("ee"); - Assert.AreEqual(230, a3); } /// @@ -324,49 +327,55 @@ public void TestThread() //should be removed. dynamic _ps = ps; var ts = PythonEngine.BeginAllowThreads(); - using (Py.GIL()) - { - _ps.res = 0; - _ps.bb = 100; - _ps.th_cnt = 0; - //add function to the scope - //can be call many times, more efficient than ast - ps.Exec( - "def update():\n" + - " global res, th_cnt\n" + - " res += bb + 1\n" + - " th_cnt += 1\n" - ); - } - int th_cnt = 3; - for (int i =0; i< th_cnt; i++) + try { - System.Threading.Thread th = new System.Threading.Thread(()=> + using (Py.GIL()) + { + _ps.res = 0; + _ps.bb = 100; + _ps.th_cnt = 0; + //add function to the scope + //can be call many times, more efficient than ast + ps.Exec( + "def update():\n" + + " global res, th_cnt\n" + + " res += bb + 1\n" + + " th_cnt += 1\n" + ); + } + int th_cnt = 3; + for (int i = 0; i < th_cnt; i++) + { + System.Threading.Thread th = new System.Threading.Thread(() => + { + using (Py.GIL()) + { + //ps.GetVariable("update")(); //call the scope function dynamicly + _ps.update(); + } + }); + th.Start(); + } + //equivalent to Thread.Join, make the main thread join the GIL competition + int cnt = 0; + while (cnt != th_cnt) { using (Py.GIL()) { - //ps.GetVariable("update")(); //call the scope function dynamicly - _ps.update(); + cnt = ps.Get("th_cnt"); } - }); - th.Start(); - } - //equivalent to Thread.Join, make the main thread join the GIL competition - int cnt = 0; - while(cnt != th_cnt) - { + System.Threading.Thread.Sleep(10); + } using (Py.GIL()) { - cnt = ps.Get("th_cnt"); + var result = ps.Get("res"); + Assert.AreEqual(101 * th_cnt, result); } - System.Threading.Thread.Sleep(10); } - using (Py.GIL()) + finally { - var result = ps.Get("res"); - Assert.AreEqual(101* th_cnt, result); + PythonEngine.EndAllowThreads(ts); } - PythonEngine.EndAllowThreads(ts); } } } From 6d9f89718ccd1535326753919a7faf45af0369f0 Mon Sep 17 00:00:00 2001 From: amos402 Date: Sun, 24 Jun 2018 15:51:39 +0800 Subject: [PATCH 03/18] Add source to .csproj --- src/embed_tests/Python.EmbeddingTest.csproj | 249 +++++++-------- src/embed_tests/TestFinalizer.cs | 6 +- src/runtime/Python.Runtime.csproj | 327 ++++++++++---------- 3 files changed, 294 insertions(+), 288 deletions(-) diff --git a/src/embed_tests/Python.EmbeddingTest.csproj b/src/embed_tests/Python.EmbeddingTest.csproj index 66e8c7165..11ef2daac 100644 --- a/src/embed_tests/Python.EmbeddingTest.csproj +++ b/src/embed_tests/Python.EmbeddingTest.csproj @@ -1,125 +1,126 @@ - - - - Debug - AnyCPU - {4165C59D-2822-499F-A6DB-EACA4C331EB5} - Library - Python.EmbeddingTest - Python.EmbeddingTest - bin\Python.EmbeddingTest.xml - bin\ - v4.0 - - 1591 - ..\..\ - $(SolutionDir)\bin\ - 6 - true - prompt - - - x86 - - - x64 - - - true - DEBUG;TRACE - full - - - - - true - pdbonly - - - true - DEBUG;TRACE - full - - - - - true - pdbonly - - - true - DEBUG;TRACE - full - - - - - true - pdbonly - - - true - DEBUG;TRACE - full - - - - - true - pdbonly - - - - - ..\..\packages\NUnit.3.7.1\lib\net40\nunit.framework.dll - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - {097B4AC0-74E9-4C58-BCF8-C69746EC8271} - Python.Runtime - - - - - - - - $(TargetPath) - $(TargetDir)$(TargetName).pdb - - - - - + + + + Debug + AnyCPU + {4165C59D-2822-499F-A6DB-EACA4C331EB5} + Library + Python.EmbeddingTest + Python.EmbeddingTest + bin\Python.EmbeddingTest.xml + bin\ + v4.0 + + 1591 + ..\..\ + $(SolutionDir)\bin\ + 6 + true + prompt + + + x86 + + + x64 + + + true + DEBUG;TRACE + full + + + + + true + pdbonly + + + true + DEBUG;TRACE + full + + + + + true + pdbonly + + + true + DEBUG;TRACE + full + + + + + true + pdbonly + + + true + DEBUG;TRACE + full + + + + + true + pdbonly + + + + + ..\..\packages\NUnit.3.7.1\lib\net40\nunit.framework.dll + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + {097B4AC0-74E9-4C58-BCF8-C69746EC8271} + Python.Runtime + + + + + + + + $(TargetPath) + $(TargetDir)$(TargetName).pdb + + + + + \ No newline at end of file diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index 03e2f1be5..6041e76cc 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -44,6 +44,8 @@ private static void FullGCCollect() [Test] public void CollectBasicObject() { + Assert.IsTrue(Finalizer.Instance.Enable); + int thId = Thread.CurrentThread.ManagedThreadId; Finalizer.Instance.Threshold = 1; bool called = false; @@ -62,11 +64,13 @@ public void CollectBasicObject() obj = null; FullGCCollect(); // The object has been resurrected - Assert.IsFalse(shortWeak.IsAlive); + // FIXME: Sometimes the shortWeak would get alive + //Assert.IsFalse(shortWeak.IsAlive); Assert.IsTrue(longWeak.IsAlive); Assert.IsFalse(called); var garbage = Finalizer.Instance.GetCollectedObjects(); + Assert.NotZero(garbage.Count); // FIXME: If make some query for garbage, // the above case will failed Assert.IsFalse(shortWeak.IsAlive) //Assert.IsTrue(garbage.All(T => T.IsAlive)); diff --git a/src/runtime/Python.Runtime.csproj b/src/runtime/Python.Runtime.csproj index 1fea78082..28ea6424f 100644 --- a/src/runtime/Python.Runtime.csproj +++ b/src/runtime/Python.Runtime.csproj @@ -1,169 +1,170 @@ - - - - Debug - AnyCPU - {097B4AC0-74E9-4C58-BCF8-C69746EC8271} - Library - Python.Runtime - Python.Runtime - bin\Python.Runtime.xml - bin\ - v4.0 - - 1591 - ..\..\ - $(SolutionDir)\bin\ - Properties - 6 - true - false - ..\pythonnet.snk - - - + + + + Debug + AnyCPU + {097B4AC0-74E9-4C58-BCF8-C69746EC8271} + Library + Python.Runtime + Python.Runtime + bin\Python.Runtime.xml + bin\ + v4.0 + + 1591 + ..\..\ + $(SolutionDir)\bin\ + Properties + 6 + true + false + ..\pythonnet.snk + + + - - PYTHON2;PYTHON27;UCS4 - true - pdbonly - - - PYTHON3;PYTHON36;UCS4 - true - pdbonly - - - true - PYTHON2;PYTHON27;UCS4;TRACE;DEBUG - false - full - - - true - PYTHON3;PYTHON36;UCS4;TRACE;DEBUG - false - full - - - PYTHON2;PYTHON27;UCS2 - true - pdbonly - - - PYTHON3;PYTHON36;UCS2 - true - pdbonly - - - true - PYTHON2;PYTHON27;UCS2;TRACE;DEBUG - false - full - - - true - PYTHON3;PYTHON36;UCS2;TRACE;DEBUG - false - full - - - - - - - - Properties\SharedAssemblyInfo.cs - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - clr.py - - - - - $(TargetPath) - $(TargetDir)$(TargetName).pdb - - - - - + --> + + PYTHON2;PYTHON27;UCS4 + true + pdbonly + + + PYTHON3;PYTHON36;UCS4 + true + pdbonly + + + true + PYTHON2;PYTHON27;UCS4;TRACE;DEBUG + false + full + + + true + PYTHON3;PYTHON36;UCS4;TRACE;DEBUG + false + full + + + PYTHON2;PYTHON27;UCS2 + true + pdbonly + + + PYTHON3;PYTHON36;UCS2 + true + pdbonly + + + true + PYTHON2;PYTHON27;UCS2;TRACE;DEBUG + false + full + + + true + PYTHON3;PYTHON36;UCS2;TRACE;DEBUG + false + full + + + + + + + + + Properties\SharedAssemblyInfo.cs + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + clr.py + + + + + $(TargetPath) + $(TargetDir)$(TargetName).pdb + + + + + \ No newline at end of file From 7140fd003be8c5f8d83807ce21c47869aaad0767 Mon Sep 17 00:00:00 2001 From: amos402 Date: Sun, 24 Jun 2018 16:11:05 +0800 Subject: [PATCH 04/18] Make sure recover the environment --- src/embed_tests/TestFinalizer.cs | 55 ++++++++++++++++++-------------- 1 file changed, 31 insertions(+), 24 deletions(-) diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index 6041e76cc..7cf3b7309 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -22,6 +22,7 @@ public void SetUp() } Environment.SetEnvironmentVariable("PYTHONMALLOC", "malloc"); PythonEngine.Initialize(); + Exceptions.Clear(); } [TearDown] @@ -56,34 +57,40 @@ public void CollectBasicObject() called = true; }; Finalizer.Instance.CollectOnce += handler; - FullGCCollect(); - PyLong obj = new PyLong(1024); - - WeakReference shortWeak = new WeakReference(obj); - WeakReference longWeak = new WeakReference(obj, true); - obj = null; - FullGCCollect(); - // The object has been resurrected - // FIXME: Sometimes the shortWeak would get alive - //Assert.IsFalse(shortWeak.IsAlive); - Assert.IsTrue(longWeak.IsAlive); - - Assert.IsFalse(called); - var garbage = Finalizer.Instance.GetCollectedObjects(); - Assert.NotZero(garbage.Count); - // FIXME: If make some query for garbage, - // the above case will failed Assert.IsFalse(shortWeak.IsAlive) - //Assert.IsTrue(garbage.All(T => T.IsAlive)); + try + { + FullGCCollect(); + PyLong obj = new PyLong(1024); + + WeakReference shortWeak = new WeakReference(obj); + WeakReference longWeak = new WeakReference(obj, true); + obj = null; + FullGCCollect(); + // The object has been resurrected + // FIXME: Sometimes the shortWeak would get alive + //Assert.IsFalse(shortWeak.IsAlive); + Assert.IsTrue(longWeak.IsAlive); + + Assert.IsFalse(called); + var garbage = Finalizer.Instance.GetCollectedObjects(); + Assert.NotZero(garbage.Count); + // FIXME: If make some query for garbage, + // the above case will failed Assert.IsFalse(shortWeak.IsAlive) + //Assert.IsTrue(garbage.All(T => T.IsAlive)); - Finalizer.Instance.CallPendingFinalizers(); - Assert.IsTrue(called); + Finalizer.Instance.CallPendingFinalizers(); + Assert.IsTrue(called); - FullGCCollect(); - //Assert.IsFalse(garbage.All(T => T.IsAlive)); + FullGCCollect(); + //Assert.IsFalse(garbage.All(T => T.IsAlive)); - Assert.IsNull(longWeak.Target); + Assert.IsNull(longWeak.Target); + } + finally + { + Finalizer.Instance.CollectOnce -= handler; + } - Finalizer.Instance.CollectOnce -= handler; } private static long CompareWithFinalizerOn(PyObject pyCollect, bool enbale) From f66697dd941091b87bb28617ee308657c87a1eef Mon Sep 17 00:00:00 2001 From: amos402 Date: Sun, 24 Jun 2018 16:52:34 +0800 Subject: [PATCH 05/18] Add StackTrace of C# exception --- src/runtime/pythonexception.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs index 1b166854a..4b4d8b107 100644 --- a/src/runtime/pythonexception.cs +++ b/src/runtime/pythonexception.cs @@ -136,7 +136,7 @@ public override string Message /// public override string StackTrace { - get { return _tb; } + get { return _tb + base.StackTrace; } } /// From 799d37e51232e814294b292c4414ea6edacba924 Mon Sep 17 00:00:00 2001 From: amos402 Date: Sun, 24 Jun 2018 20:42:14 +0800 Subject: [PATCH 06/18] Clean up the test and interface --- src/embed_tests/TestFinalizer.cs | 55 +++++++++++++++++--------------- src/runtime/finalizer.cs | 29 +++++++++++------ 2 files changed, 49 insertions(+), 35 deletions(-) diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index 7cf3b7309..d9e9729fb 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -1,6 +1,7 @@ using NUnit.Framework; using Python.Runtime; using System; +using System.Linq; using System.Threading; namespace Python.EmbeddingTest @@ -8,6 +9,7 @@ namespace Python.EmbeddingTest public class TestFinalizer { private string _PYTHONMALLOC = string.Empty; + private int _oldThreshold; [SetUp] public void SetUp() @@ -21,6 +23,7 @@ public void SetUp() _PYTHONMALLOC = string.Empty; } Environment.SetEnvironmentVariable("PYTHONMALLOC", "malloc"); + _oldThreshold = Finalizer.Instance.Threshold; PythonEngine.Initialize(); Exceptions.Clear(); } @@ -28,6 +31,7 @@ public void SetUp() [TearDown] public void TearDown() { + Finalizer.Instance.Threshold = _oldThreshold; PythonEngine.Shutdown(); if (string.IsNullOrEmpty(_PYTHONMALLOC)) { @@ -56,41 +60,42 @@ public void CollectBasicObject() Assert.GreaterOrEqual(e.ObjectCount, 1); called = true; }; - Finalizer.Instance.CollectOnce += handler; - try - { - FullGCCollect(); - PyLong obj = new PyLong(1024); - WeakReference shortWeak = new WeakReference(obj); - WeakReference longWeak = new WeakReference(obj, true); - obj = null; - FullGCCollect(); - // The object has been resurrected - // FIXME: Sometimes the shortWeak would get alive - //Assert.IsFalse(shortWeak.IsAlive); - Assert.IsTrue(longWeak.IsAlive); + WeakReference shortWeak; + WeakReference longWeak; + { + MakeAGarbage(out shortWeak, out longWeak); + } + FullGCCollect(); + // The object has been resurrected + Assert.IsFalse(shortWeak.IsAlive); + Assert.IsTrue(longWeak.IsAlive); - Assert.IsFalse(called); + { var garbage = Finalizer.Instance.GetCollectedObjects(); Assert.NotZero(garbage.Count); - // FIXME: If make some query for garbage, - // the above case will failed Assert.IsFalse(shortWeak.IsAlive) - //Assert.IsTrue(garbage.All(T => T.IsAlive)); + Assert.IsTrue(garbage.Any(T => ReferenceEquals(T.Target, longWeak.Target))); + } + Assert.IsFalse(called); + Finalizer.Instance.CollectOnce += handler; + try + { Finalizer.Instance.CallPendingFinalizers(); - Assert.IsTrue(called); - - FullGCCollect(); - //Assert.IsFalse(garbage.All(T => T.IsAlive)); - - Assert.IsNull(longWeak.Target); } finally { Finalizer.Instance.CollectOnce -= handler; } + Assert.IsTrue(called); + } + private static void MakeAGarbage(out WeakReference shortWeak, out WeakReference longWeak) + { + PyLong obj = new PyLong(1024); + shortWeak = new WeakReference(obj); + longWeak = new WeakReference(obj, true); + obj = null; } private static long CompareWithFinalizerOn(PyObject pyCollect, bool enbale) @@ -101,7 +106,7 @@ private static long CompareWithFinalizerOn(PyObject pyCollect, bool enbale) FullGCCollect(); FullGCCollect(); pyCollect.Invoke(); - Finalizer.Instance.CallPendingFinalizers(); + Finalizer.Instance.Collect(); Finalizer.Instance.Enable = enbale; // Estimate unmanaged memory size @@ -116,7 +121,7 @@ private static long CompareWithFinalizerOn(PyObject pyCollect, bool enbale) pyCollect.Invoke(); if (enbale) { - Finalizer.Instance.CallPendingFinalizers(); + Finalizer.Instance.Collect(); } FullGCCollect(); diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs index 344260f97..64faaf5b4 100644 --- a/src/runtime/finalizer.cs +++ b/src/runtime/finalizer.cs @@ -22,7 +22,8 @@ public class CollectArgs : EventArgs [UnmanagedFunctionPointer(CallingConvention.Cdecl)] private delegate int PedingCall(IntPtr arg); - private PedingCall _collectAction; + private readonly PedingCall _collectAction; + private bool _pending = false; private readonly object _collectingLock = new object(); public int Threshold { get; set; } @@ -32,7 +33,7 @@ private Finalizer() { Enable = true; Threshold = 200; - _collectAction = OnCollect; + _collectAction = OnPendingCollect; } public void CallPendingFinalizers() @@ -44,6 +45,14 @@ public void CallPendingFinalizers() Runtime.Py_MakePendingCalls(); } + public void Collect() + { + using (var gilState = new Py.GILState()) + { + DisposeAll(); + } + } + public List GetCollectedObjects() { return _objQueue.Select(T => new WeakReference(T)).ToList(); @@ -65,7 +74,7 @@ internal void AddFinalizedObject(IDisposable obj) GC.ReRegisterForFinalize(obj); if (_objQueue.Count >= Threshold) { - Collect(); + AddPendingCollect(); } } @@ -76,7 +85,7 @@ internal static void Shutdown() Runtime.PyErr_Clear(); } - private void Collect() + private void AddPendingCollect() { lock (_collectingLock) { @@ -94,19 +103,19 @@ private void Collect() } } - private int OnCollect(IntPtr arg) + private int OnPendingCollect(IntPtr arg) { - CollectOnce?.Invoke(this, new CollectArgs() - { - ObjectCount = _objQueue.Count - }); - DisposeAll(); + Collect(); _pending = false; return 0; } private void DisposeAll() { + CollectOnce?.Invoke(this, new CollectArgs() + { + ObjectCount = _objQueue.Count + }); IDisposable obj; while (_objQueue.TryDequeue(out obj)) { From cb55163c8e6df2217689afc92898d1b69884f8ec Mon Sep 17 00:00:00 2001 From: amos402 Date: Mon, 25 Jun 2018 00:09:31 +0800 Subject: [PATCH 07/18] Update CHANGELOG.md --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c30b4c393..5e653b04d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,8 +17,10 @@ This document follows the conventions laid out in [Keep a CHANGELOG][]. - Allowed passing `None` for nullable args (#460) - Added keyword arguments based on C# syntax for calling CPython methods (#461) - Implemented GetDynamicMemberNames() for PyObject to allow dynamic object members to be visible in the debugger (#443) +- Added PyObject finalizer support, Python objects referred by C# can be auto collect now. ### Changed +- PythonException included C# call stack ### Fixed From bfc039231dd94c722a6b6dd5d2998481086c165c Mon Sep 17 00:00:00 2001 From: amos402 Date: Mon, 25 Jun 2018 00:22:23 +0800 Subject: [PATCH 08/18] Mono doesn't have GC.WaitForFullGCComplete --- src/embed_tests/TestFinalizer.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index d9e9729fb..985ce3b35 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -20,7 +20,7 @@ public void SetUp() } catch (ArgumentNullException) { - _PYTHONMALLOC = string.Empty; + _PYTHONMALLOC = null; } Environment.SetEnvironmentVariable("PYTHONMALLOC", "malloc"); _oldThreshold = Finalizer.Instance.Threshold; @@ -33,16 +33,12 @@ public void TearDown() { Finalizer.Instance.Threshold = _oldThreshold; PythonEngine.Shutdown(); - if (string.IsNullOrEmpty(_PYTHONMALLOC)) - { - Environment.SetEnvironmentVariable("PYTHONMALLOC", _PYTHONMALLOC); - } + Environment.SetEnvironmentVariable("PYTHONMALLOC", _PYTHONMALLOC); } private static void FullGCCollect() { GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced); - GC.WaitForFullGCComplete(); GC.WaitForPendingFinalizers(); } From 59b614dc538242cee28045616215d78678a9c9a3 Mon Sep 17 00:00:00 2001 From: amos402 Date: Mon, 25 Jun 2018 16:52:16 +0800 Subject: [PATCH 09/18] Fixed PythonException leak --- src/runtime/finalizer.cs | 2 +- src/runtime/pythonexception.cs | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs index 64faaf5b4..2f4549256 100644 --- a/src/runtime/finalizer.cs +++ b/src/runtime/finalizer.cs @@ -105,7 +105,7 @@ private void AddPendingCollect() private int OnPendingCollect(IntPtr arg) { - Collect(); + DisposeAll(); _pending = false; return 0; } diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs index 4b4d8b107..52438b386 100644 --- a/src/runtime/pythonexception.cs +++ b/src/runtime/pythonexception.cs @@ -21,9 +21,6 @@ public PythonException() { IntPtr gs = PythonEngine.AcquireLock(); Runtime.PyErr_Fetch(ref _pyType, ref _pyValue, ref _pyTB); - Runtime.XIncref(_pyType); - Runtime.XIncref(_pyValue); - Runtime.XIncref(_pyTB); if (_pyType != IntPtr.Zero && _pyValue != IntPtr.Zero) { string type; From f4f503222d1af92d60e09b00dcb0472c4a50a287 Mon Sep 17 00:00:00 2001 From: amos402 Date: Tue, 10 Jul 2018 15:15:50 +0800 Subject: [PATCH 10/18] Fixed nPython.exe crash on Shutdown --- src/runtime/finalizer.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs index 2f4549256..ba1064387 100644 --- a/src/runtime/finalizer.cs +++ b/src/runtime/finalizer.cs @@ -80,6 +80,11 @@ internal void AddFinalizedObject(IDisposable obj) internal static void Shutdown() { + if (Runtime.Py_IsInitialized() == 0) + { + Instance._objQueue = new ConcurrentQueue(); + return; + } Instance.DisposeAll(); Instance.CallPendingFinalizers(); Runtime.PyErr_Clear(); From f071c557d52c5c92dd5fddec96bc327d0a8ee1f1 Mon Sep 17 00:00:00 2001 From: amos402 Date: Sat, 4 Aug 2018 16:19:09 +0800 Subject: [PATCH 11/18] Add error handler Synchronized fixed --- src/embed_tests/TestFinalizer.cs | 54 ++++++++++++++++++++++++++++++++ src/runtime/finalizer.cs | 41 ++++++++++++++++++------ src/runtime/runtime.cs | 4 +-- 3 files changed, 87 insertions(+), 12 deletions(-) diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index 985ce3b35..f8d8b6548 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -151,5 +151,59 @@ public void SimpleTestMemory() Finalizer.Instance.Enable = oldState; } } + + class MyPyObject : PyObject + { + public MyPyObject(IntPtr op) : base(op) + { + } + + protected override void Dispose(bool disposing) + { + base.Dispose(disposing); + GC.SuppressFinalize(this); + throw new Exception("MyPyObject"); + } + internal static void CreateMyPyObject(IntPtr op) + { + Runtime.Runtime.XIncref(op); + new MyPyObject(op); + } + } + + [Test] + public void ErrorHandling() + { + bool called = false; + EventHandler handleFunc = (sender, args) => + { + called = true; + Assert.AreEqual(args.Error.Message, "MyPyObject"); + }; + Finalizer.Instance.Threshold = 1; + Finalizer.Instance.ErrorHandler += handleFunc; + try + { + WeakReference shortWeak; + WeakReference longWeak; + { + MakeAGarbage(out shortWeak, out longWeak); + var obj = (PyLong)longWeak.Target; + IntPtr handle = obj.Handle; + shortWeak = null; + longWeak = null; + MyPyObject.CreateMyPyObject(handle); + obj.Dispose(); + obj = null; + } + FullGCCollect(); + Finalizer.Instance.Collect(); + Assert.IsTrue(called); + } + finally + { + Finalizer.Instance.ErrorHandler -= handleFunc; + } + } } } diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs index ba1064387..0fc36768f 100644 --- a/src/runtime/finalizer.cs +++ b/src/runtime/finalizer.cs @@ -14,15 +14,21 @@ public class CollectArgs : EventArgs public int ObjectCount { get; set; } } + public class ErrorArgs : EventArgs + { + public Exception Error { get; set; } + } + public static readonly Finalizer Instance = new Finalizer(); public event EventHandler CollectOnce; + public event EventHandler ErrorHandler; private ConcurrentQueue _objQueue = new ConcurrentQueue(); [UnmanagedFunctionPointer(CallingConvention.Cdecl)] - private delegate int PedingCall(IntPtr arg); - private readonly PedingCall _collectAction; + private delegate int PendingCall(IntPtr arg); + private readonly PendingCall _collectAction; private bool _pending = false; private readonly object _collectingLock = new object(); @@ -87,11 +93,14 @@ internal static void Shutdown() } Instance.DisposeAll(); Instance.CallPendingFinalizers(); - Runtime.PyErr_Clear(); } private void AddPendingCollect() { + if (_pending) + { + return; + } lock (_collectingLock) { if (_pending) @@ -99,12 +108,12 @@ private void AddPendingCollect() return; } _pending = true; - } - IntPtr func = Marshal.GetFunctionPointerForDelegate(_collectAction); - if (Runtime.Py_AddPendingCall(func, IntPtr.Zero) != 0) - { - // Full queue, append next time - _pending = false; + IntPtr func = Marshal.GetFunctionPointerForDelegate(_collectAction); + if (Runtime.Py_AddPendingCall(func, IntPtr.Zero) != 0) + { + // Full queue, append next time + _pending = false; + } } } @@ -124,7 +133,19 @@ private void DisposeAll() IDisposable obj; while (_objQueue.TryDequeue(out obj)) { - obj.Dispose(); + try + { + obj.Dispose(); + Runtime.CheckExceptionOccurred(); + } + catch (Exception e) + { + // We should not bother the main thread + ErrorHandler?.Invoke(this, new ErrorArgs() + { + Error = e + }); + } } } } diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs index 46e25354b..92f6544b9 100644 --- a/src/runtime/runtime.cs +++ b/src/runtime/runtime.cs @@ -199,7 +199,7 @@ public class Runtime internal static bool IsPython2 = pyversionnumber < 30; internal static bool IsPython3 = pyversionnumber >= 30; - public static int MainManagedThreadId { get; internal set; } + public static int MainManagedThreadId { get; private set; } /// /// Encoding to use to convert Unicode to/from Managed to Native @@ -354,10 +354,10 @@ internal static void Initialize() internal static void Shutdown() { - Finalizer.Shutdown(); AssemblyManager.Shutdown(); Exceptions.Shutdown(); ImportHook.Shutdown(); + Finalizer.Shutdown(); Py_Finalize(); } From 0d966412b6a681e4b489b03f343a7df4ebdfd4d5 Mon Sep 17 00:00:00 2001 From: amos402 Date: Thu, 6 Sep 2018 15:06:36 +0800 Subject: [PATCH 12/18] Make collect callback without JIT --- src/runtime/finalizer.cs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs index 0fc36768f..e6efab7c5 100644 --- a/src/runtime/finalizer.cs +++ b/src/runtime/finalizer.cs @@ -117,10 +117,10 @@ private void AddPendingCollect() } } - private int OnPendingCollect(IntPtr arg) + private static int OnPendingCollect(IntPtr arg) { - DisposeAll(); - _pending = false; + Instance.DisposeAll(); + Instance._pending = false; return 0; } From a4bb82d61c45a5fc0e7a5de3aa0615c422fe35e5 Mon Sep 17 00:00:00 2001 From: amos402 Date: Wed, 24 Oct 2018 03:53:09 +0800 Subject: [PATCH 13/18] Add pending marker --- src/runtime/finalizer.cs | 66 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 60 insertions(+), 6 deletions(-) diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs index e6efab7c5..9164d68a1 100644 --- a/src/runtime/finalizer.cs +++ b/src/runtime/finalizer.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Concurrent; using System.Collections.Generic; +using System.Diagnostics; using System.Linq; using System.Runtime.InteropServices; using System.Threading; @@ -24,16 +25,23 @@ public class ErrorArgs : EventArgs public event EventHandler CollectOnce; public event EventHandler ErrorHandler; - private ConcurrentQueue _objQueue = new ConcurrentQueue(); + 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 _objQueue = new ConcurrentQueue(); private bool _pending = false; private readonly object _collectingLock = new object(); - public int Threshold { get; set; } - public bool Enable { get; set; } + private IntPtr _pendingArgs; private Finalizer() { @@ -92,6 +100,23 @@ internal static void Shutdown() 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(); } @@ -108,8 +133,12 @@ private void AddPendingCollect() return; } _pending = true; + var args = new PendingArgs() { cancelled = false }; + IntPtr p = Marshal.AllocHGlobal(Marshal.SizeOf(typeof(PendingArgs))); + Marshal.StructureToPtr(args, p, false); + _pendingArgs = p; IntPtr func = Marshal.GetFunctionPointerForDelegate(_collectAction); - if (Runtime.Py_AddPendingCall(func, IntPtr.Zero) != 0) + if (Runtime.Py_AddPendingCall(func, p) != 0) { // Full queue, append next time _pending = false; @@ -119,8 +148,24 @@ private void AddPendingCollect() private static int OnPendingCollect(IntPtr arg) { - Instance.DisposeAll(); - Instance._pending = false; + Debug.Assert(arg == Instance._pendingArgs); + try + { + unsafe + { + PendingArgs* pendingArgs = (PendingArgs*)arg; + if (pendingArgs->cancelled) + { + return 0; + } + } + Instance.DisposeAll(); + } + finally + { + Instance.ResetPending(); + Marshal.FreeHGlobal(arg); + } return 0; } @@ -148,5 +193,14 @@ private void DisposeAll() } } } + + private void ResetPending() + { + lock (_collectingLock) + { + _pending = false; + _pendingArgs = IntPtr.Zero; + } + } } } From 5254c65b246e7bd79a39067cd4e1363ff5909872 Mon Sep 17 00:00:00 2001 From: amos402 Date: Fri, 26 Oct 2018 03:09:37 +0800 Subject: [PATCH 14/18] Remove PYTHONMALLOC setting --- src/embed_tests/TestFinalizer.cs | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index f8d8b6548..3f09668a4 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -14,15 +14,6 @@ public class TestFinalizer [SetUp] public void SetUp() { - try - { - _PYTHONMALLOC = Environment.GetEnvironmentVariable("PYTHONMALLOC"); - } - catch (ArgumentNullException) - { - _PYTHONMALLOC = null; - } - Environment.SetEnvironmentVariable("PYTHONMALLOC", "malloc"); _oldThreshold = Finalizer.Instance.Threshold; PythonEngine.Initialize(); Exceptions.Clear(); @@ -33,7 +24,6 @@ public void TearDown() { Finalizer.Instance.Threshold = _oldThreshold; PythonEngine.Shutdown(); - Environment.SetEnvironmentVariable("PYTHONMALLOC", _PYTHONMALLOC); } private static void FullGCCollect() From eee368309d761afcb19fda42481bc50b54e737ce Mon Sep 17 00:00:00 2001 From: amos402 Date: Fri, 23 Nov 2018 01:52:05 +0800 Subject: [PATCH 15/18] Fix ref count error --- src/embed_tests/TestPyAnsiString.cs | 1 + src/embed_tests/TestPyFloat.cs | 1 + src/embed_tests/TestPyString.cs | 1 + 3 files changed, 3 insertions(+) diff --git a/src/embed_tests/TestPyAnsiString.cs b/src/embed_tests/TestPyAnsiString.cs index 9ba7d6cc6..b4a965ff7 100644 --- a/src/embed_tests/TestPyAnsiString.cs +++ b/src/embed_tests/TestPyAnsiString.cs @@ -63,6 +63,7 @@ public void TestCtorPtr() const string expected = "foo"; var t = new PyAnsiString(expected); + Runtime.Runtime.XIncref(t.Handle); var actual = new PyAnsiString(t.Handle); Assert.AreEqual(expected, actual.ToString()); diff --git a/src/embed_tests/TestPyFloat.cs b/src/embed_tests/TestPyFloat.cs index f2c85a77f..94e7026c7 100644 --- a/src/embed_tests/TestPyFloat.cs +++ b/src/embed_tests/TestPyFloat.cs @@ -25,6 +25,7 @@ public void Dispose() public void IntPtrCtor() { var i = new PyFloat(1); + Runtime.Runtime.XIncref(i.Handle); var ii = new PyFloat(i.Handle); Assert.AreEqual(i.Handle, ii.Handle); } diff --git a/src/embed_tests/TestPyString.cs b/src/embed_tests/TestPyString.cs index 9d1cdb0e9..0de436e35 100644 --- a/src/embed_tests/TestPyString.cs +++ b/src/embed_tests/TestPyString.cs @@ -64,6 +64,7 @@ public void TestCtorPtr() const string expected = "foo"; var t = new PyString(expected); + Runtime.Runtime.XIncref(t.Handle); var actual = new PyString(t.Handle); Assert.AreEqual(expected, actual.ToString()); From cee8e17a51c1d37078b20b3c71066d3fe8ad3fbe Mon Sep 17 00:00:00 2001 From: amos402 Date: Fri, 23 Nov 2018 03:37:18 +0800 Subject: [PATCH 16/18] Add ref count check for helping discover the bugs of decref too much --- src/embed_tests/TestFinalizer.cs | 42 ++++++- src/runtime/Python.Runtime.15.csproj | 8 +- src/runtime/delegatemanager.cs | 7 +- src/runtime/finalizer.cs | 157 ++++++++++++++++++++++++--- src/runtime/pyobject.cs | 29 ++++- src/runtime/pyscope.cs | 7 +- src/runtime/pythonexception.cs | 7 +- 7 files changed, 232 insertions(+), 25 deletions(-) diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index 3f09668a4..1b7faf084 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -8,7 +8,6 @@ namespace Python.EmbeddingTest { public class TestFinalizer { - private string _PYTHONMALLOC = string.Empty; private int _oldThreshold; [SetUp] @@ -195,5 +194,46 @@ public void ErrorHandling() Finalizer.Instance.ErrorHandler -= handleFunc; } } + + [Test] + public void ValidateRefCount() + { + if (!Finalizer.Instance.RefCountValidationEnabled) + { + Assert.Pass("Only run with FINALIZER_CHECK"); + } + IntPtr ptr = IntPtr.Zero; + bool called = false; + Finalizer.IncorrectRefCntHandler handler = (s, e) => + { + called = true; + Assert.AreEqual(ptr, e.Handle); + Assert.AreEqual(2, e.ImpactedObjects.Count); + // Fix for this test, don't do this on general environment + Runtime.Runtime.XIncref(e.Handle); + return false; + }; + Finalizer.Instance.IncorrectRefCntResovler += handler; + try + { + ptr = CreateStringGarbage(); + FullGCCollect(); + Assert.Throws(() => Finalizer.Instance.Collect()); + Assert.IsTrue(called); + } + finally + { + Finalizer.Instance.IncorrectRefCntResovler -= handler; + } + } + + private static IntPtr CreateStringGarbage() + { + PyString s1 = new PyString("test_string"); + // s2 steal a reference from s1 + PyString s2 = new PyString(s1.Handle); + return s1.Handle; + } + } } diff --git a/src/runtime/Python.Runtime.15.csproj b/src/runtime/Python.Runtime.15.csproj index 794645994..29177b78c 100644 --- a/src/runtime/Python.Runtime.15.csproj +++ b/src/runtime/Python.Runtime.15.csproj @@ -68,10 +68,10 @@ $(DefineConstants);PYTHON3;$(Python3Version);$(PythonMonoDefineConstants) - $(DefineConstants);PYTHON2;$(Python2Version);$(PythonMonoDefineConstants);TRACE;DEBUG + $(DefineConstants);PYTHON2;$(Python2Version);$(PythonMonoDefineConstants);FINALIZER_CHECK;TRACE;DEBUG - $(DefineConstants);PYTHON3;$(Python3Version);$(PythonMonoDefineConstants);TRACE;DEBUG + $(DefineConstants);PYTHON3;$(Python3Version);$(PythonMonoDefineConstants);FINALIZER_CHECK;TRACE;DEBUG $(DefineConstants);PYTHON2;$(Python2Version);$(PythonWinDefineConstants) @@ -80,10 +80,10 @@ $(DefineConstants);PYTHON3;$(Python3Version);$(PythonWinDefineConstants) - $(DefineConstants);PYTHON2;$(Python2Version);$(PythonWinDefineConstants);TRACE;DEBUG + $(DefineConstants);PYTHON2;$(Python2Version);$(PythonWinDefineConstants);FINALIZER_CHECK;TRACE;DEBUG - $(DefineConstants);PYTHON3;$(Python3Version);$(PythonWinDefineConstants);TRACE;DEBUG + $(DefineConstants);PYTHON3;$(Python3Version);$(PythonWinDefineConstants);FINALIZER_CHECK;TRACE;DEBUG diff --git a/src/runtime/delegatemanager.cs b/src/runtime/delegatemanager.cs index 706bd4bc4..bd8f1ee4c 100644 --- a/src/runtime/delegatemanager.cs +++ b/src/runtime/delegatemanager.cs @@ -181,7 +181,7 @@ A possible alternate strategy would be to create custom subclasses too "special" for this to work. It would be more work, so for now the 80/20 rule applies :) */ - public class Dispatcher : IDisposable + public class Dispatcher : IPyDisposable { public IntPtr target; public Type dtype; @@ -276,6 +276,11 @@ public object TrueDispatch(ArrayList args) Runtime.XDecref(op); return result; } + + public IntPtr[] GetTrackedHandles() + { + return new IntPtr[] { target }; + } } diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs index 9164d68a1..80519845a 100644 --- a/src/runtime/finalizer.cs +++ b/src/runtime/finalizer.cs @@ -38,11 +38,48 @@ struct PendingArgs private delegate int PendingCall(IntPtr arg); private readonly PendingCall _collectAction; - private ConcurrentQueue _objQueue = new ConcurrentQueue(); + private ConcurrentQueue _objQueue = new ConcurrentQueue(); private bool _pending = false; private readonly object _collectingLock = new object(); private IntPtr _pendingArgs; + #region FINALIZER_CHECK + +#if FINALIZER_CHECK + private readonly object _queueLock = new object(); + public bool RefCountValidationEnabled { get; set; } = true; +#else + public readonly bool RefCountValidationEnabled = false; +#endif + // Keep these declarations for compat even no FINALIZER_CHECK + public class IncorrectFinalizeArgs : EventArgs + { + public IntPtr Handle { get; internal set; } + public ICollection ImpactedObjects { get; internal set; } + } + + public class IncorrectRefCountException : Exception + { + public IntPtr PyPtr { get; internal set; } + private string _message; + public override string Message => _message; + + public IncorrectRefCountException(IntPtr ptr) + { + PyPtr = ptr; + IntPtr pyname = Runtime.PyObject_Unicode(PyPtr); + string name = Runtime.GetManagedString(pyname); + Runtime.XDecref(pyname); + _message = $"{name} may has a incorrect ref count"; + } + } + + public delegate bool IncorrectRefCntHandler(object sender, IncorrectFinalizeArgs e); + public event IncorrectRefCntHandler IncorrectRefCntResovler; + public bool ThrowIfUnhandleIncorrectRefCount { get; set; } = true; + + #endregion + private Finalizer() { Enable = true; @@ -72,7 +109,7 @@ public List GetCollectedObjects() return _objQueue.Select(T => new WeakReference(T)).ToList(); } - internal void AddFinalizedObject(IDisposable obj) + internal void AddFinalizedObject(IPyDisposable obj) { if (!Enable) { @@ -84,7 +121,12 @@ internal void AddFinalizedObject(IDisposable obj) // for avoiding that case, user should call GC.Collect manual before shutdown. return; } - _objQueue.Enqueue(obj); +#if FINALIZER_CHECK + lock (_queueLock) +#endif + { + _objQueue.Enqueue(obj); + } GC.ReRegisterForFinalize(obj); if (_objQueue.Count >= Threshold) { @@ -96,7 +138,7 @@ internal static void Shutdown() { if (Runtime.Py_IsInitialized() == 0) { - Instance._objQueue = new ConcurrentQueue(); + Instance._objQueue = new ConcurrentQueue(); return; } Instance.DisposeAll(); @@ -175,21 +217,29 @@ private void DisposeAll() { ObjectCount = _objQueue.Count }); - IDisposable obj; - while (_objQueue.TryDequeue(out obj)) +#if FINALIZER_CHECK + lock (_queueLock) +#endif { - try - { - obj.Dispose(); - Runtime.CheckExceptionOccurred(); - } - catch (Exception e) +#if FINALIZER_CHECK + ValidateRefCount(); +#endif + IPyDisposable obj; + while (_objQueue.TryDequeue(out obj)) { - // We should not bother the main thread - ErrorHandler?.Invoke(this, new ErrorArgs() + try + { + obj.Dispose(); + Runtime.CheckExceptionOccurred(); + } + catch (Exception e) { - Error = e - }); + // We should not bother the main thread + ErrorHandler?.Invoke(this, new ErrorArgs() + { + Error = e + }); + } } } } @@ -202,5 +252,80 @@ private void ResetPending() _pendingArgs = IntPtr.Zero; } } + +#if FINALIZER_CHECK + private void ValidateRefCount() + { + if (!RefCountValidationEnabled) + { + return; + } + var counter = new Dictionary(); + var holdRefs = new Dictionary(); + var indexer = new Dictionary>(); + foreach (var obj in _objQueue) + { + IntPtr[] handles = obj.GetTrackedHandles(); + foreach (var handle in handles) + { + if (handle == IntPtr.Zero) + { + continue; + } + if (!counter.ContainsKey(handle)) + { + counter[handle] = 0; + } + counter[handle]++; + if (!holdRefs.ContainsKey(handle)) + { + holdRefs[handle] = Runtime.Refcount(handle); + } + List objs; + if (!indexer.TryGetValue(handle, out objs)) + { + objs = new List(); + indexer.Add(handle, objs); + } + objs.Add(obj); + } + } + foreach (var pair in counter) + { + IntPtr handle = pair.Key; + long cnt = pair.Value; + // Tracked handle's ref count is larger than the object's holds + // it may take an unspecified behaviour if it decref in Dispose + if (cnt > holdRefs[handle]) + { + var args = new IncorrectFinalizeArgs() + { + Handle = handle, + ImpactedObjects = indexer[handle] + }; + bool handled = false; + if (IncorrectRefCntResovler != null) + { + var funcList = IncorrectRefCntResovler.GetInvocationList(); + foreach (IncorrectRefCntHandler func in funcList) + { + if (func(this, args)) + { + handled = true; + break; + } + } + } + if (!handled && ThrowIfUnhandleIncorrectRefCount) + { + throw new IncorrectRefCountException(handle); + } + } + // Make sure no other references for PyObjects after this method + indexer[handle].Clear(); + } + indexer.Clear(); + } +#endif } } diff --git a/src/runtime/pyobject.cs b/src/runtime/pyobject.cs index 4420e4bc4..99b6985db 100644 --- a/src/runtime/pyobject.cs +++ b/src/runtime/pyobject.cs @@ -1,11 +1,17 @@ using System; using System.Collections; using System.Collections.Generic; +using System.Diagnostics; using System.Dynamic; using System.Linq.Expressions; namespace Python.Runtime { + public interface IPyDisposable : IDisposable + { + IntPtr[] GetTrackedHandles(); + } + /// /// Represents a generic Python object. The methods of this class are /// generally equivalent to the Python "abstract object API". See @@ -13,8 +19,15 @@ namespace Python.Runtime /// PY3: https://docs.python.org/3/c-api/object.html /// for details. /// - public class PyObject : DynamicObject, IEnumerable, IDisposable + public class PyObject : DynamicObject, IEnumerable, IPyDisposable { +#if TRACE_ALLOC + /// + /// Trace stack for PyObject's construction + /// + public StackTrace Traceback { get; private set; } +#endif + protected internal IntPtr obj = IntPtr.Zero; private bool disposed = false; private bool _finalized = false; @@ -31,6 +44,9 @@ public class PyObject : DynamicObject, IEnumerable, IDisposable public PyObject(IntPtr ptr) { obj = ptr; +#if TRACE_ALLOC + Traceback = new StackTrace(1); +#endif } // Protected default constructor to allow subclasses to manage @@ -38,12 +54,19 @@ public PyObject(IntPtr ptr) protected PyObject() { +#if TRACE_ALLOC + Traceback = new StackTrace(1); +#endif } // Ensure that encapsulated Python object is decref'ed appropriately // when the managed wrapper is garbage-collected. ~PyObject() { + if (obj == IntPtr.Zero) + { + return; + } if (_finalized || disposed) { return; @@ -158,6 +181,10 @@ public void Dispose() GC.SuppressFinalize(this); } + public IntPtr[] GetTrackedHandles() + { + return new IntPtr[] { obj }; + } /// /// GetPythonType Method diff --git a/src/runtime/pyscope.cs b/src/runtime/pyscope.cs index 05691a9d5..4008ce29a 100644 --- a/src/runtime/pyscope.cs +++ b/src/runtime/pyscope.cs @@ -22,7 +22,7 @@ public class PyGILAttribute : Attribute } [PyGIL] - public class PyScope : DynamicObject, IDisposable + public class PyScope : DynamicObject, IPyDisposable { public readonly string Name; @@ -526,6 +526,11 @@ public void Dispose() this.OnDispose?.Invoke(this); } + public IntPtr[] GetTrackedHandles() + { + return new IntPtr[] { obj }; + } + ~PyScope() { if (_finalized || _isDisposed) diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs index 52438b386..295a63b3d 100644 --- a/src/runtime/pythonexception.cs +++ b/src/runtime/pythonexception.cs @@ -6,7 +6,7 @@ namespace Python.Runtime /// Provides a managed interface to exceptions thrown by the Python /// runtime. /// - public class PythonException : System.Exception, IDisposable + public class PythonException : System.Exception, IPyDisposable { private IntPtr _pyType = IntPtr.Zero; private IntPtr _pyValue = IntPtr.Zero; @@ -174,6 +174,11 @@ public void Dispose() } } + public IntPtr[] GetTrackedHandles() + { + return new IntPtr[] { _pyType, _pyValue, _pyTB }; + } + /// /// Matches Method /// From 247e2d9bd1102a5bde0988a01e881d28b9a98dee Mon Sep 17 00:00:00 2001 From: amos402 Date: Sun, 25 Nov 2018 23:25:26 +0800 Subject: [PATCH 17/18] Fix ref count error --- src/embed_tests/TestPyInt.cs | 2 ++ src/embed_tests/TestPyLong.cs | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/embed_tests/TestPyInt.cs b/src/embed_tests/TestPyInt.cs index 4117336d8..005ab466d 100644 --- a/src/embed_tests/TestPyInt.cs +++ b/src/embed_tests/TestPyInt.cs @@ -86,6 +86,7 @@ public void TestCtorSByte() public void TestCtorPtr() { var i = new PyInt(5); + Runtime.Runtime.XIncref(i.Handle); var a = new PyInt(i.Handle); Assert.AreEqual(5, a.ToInt32()); } @@ -94,6 +95,7 @@ public void TestCtorPtr() public void TestCtorPyObject() { var i = new PyInt(5); + Runtime.Runtime.XIncref(i.Handle); var a = new PyInt(i); Assert.AreEqual(5, a.ToInt32()); } diff --git a/src/embed_tests/TestPyLong.cs b/src/embed_tests/TestPyLong.cs index fe3e13ef5..3c155f315 100644 --- a/src/embed_tests/TestPyLong.cs +++ b/src/embed_tests/TestPyLong.cs @@ -102,6 +102,7 @@ public void TestCtorDouble() public void TestCtorPtr() { var i = new PyLong(5); + Runtime.Runtime.XIncref(i.Handle); var a = new PyLong(i.Handle); Assert.AreEqual(5, a.ToInt32()); } @@ -110,6 +111,7 @@ public void TestCtorPtr() public void TestCtorPyObject() { var i = new PyLong(5); + Runtime.Runtime.XIncref(i.Handle); var a = new PyLong(i); Assert.AreEqual(5, a.ToInt32()); } From 90c67ca1c27f1a19d6a3734f5a07662585139214 Mon Sep 17 00:00:00 2001 From: amos402 Date: Mon, 26 Nov 2018 17:45:11 +0800 Subject: [PATCH 18/18] typo error --- src/embed_tests/TestFinalizer.cs | 4 ++-- src/runtime/finalizer.cs | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index 1b7faf084..bb90c92cf 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -213,7 +213,7 @@ public void ValidateRefCount() Runtime.Runtime.XIncref(e.Handle); return false; }; - Finalizer.Instance.IncorrectRefCntResovler += handler; + Finalizer.Instance.IncorrectRefCntResolver += handler; try { ptr = CreateStringGarbage(); @@ -223,7 +223,7 @@ public void ValidateRefCount() } finally { - Finalizer.Instance.IncorrectRefCntResovler -= handler; + Finalizer.Instance.IncorrectRefCntResolver -= handler; } } diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs index 80519845a..a94f7be31 100644 --- a/src/runtime/finalizer.cs +++ b/src/runtime/finalizer.cs @@ -75,7 +75,7 @@ public IncorrectRefCountException(IntPtr ptr) } public delegate bool IncorrectRefCntHandler(object sender, IncorrectFinalizeArgs e); - public event IncorrectRefCntHandler IncorrectRefCntResovler; + public event IncorrectRefCntHandler IncorrectRefCntResolver; public bool ThrowIfUnhandleIncorrectRefCount { get; set; } = true; #endregion @@ -304,9 +304,9 @@ private void ValidateRefCount() ImpactedObjects = indexer[handle] }; bool handled = false; - if (IncorrectRefCntResovler != null) + if (IncorrectRefCntResolver != null) { - var funcList = IncorrectRefCntResovler.GetInvocationList(); + var funcList = IncorrectRefCntResolver.GetInvocationList(); foreach (IncorrectRefCntHandler func in funcList) { if (func(this, args))