Finalizer raises FinalizationException when it sees an object from pr…

…evious run.
pythonnet · filmor · Nov 23, 2021 · Mar 3, 2020 · Nov 12, 2021 · Nov 12, 2021
commit 1897d1b309e4645c72340b00a2648d3e40c70bac
diff --git a/src/embed_tests/pyinitialize.cs b/src/embed_tests/pyinitialize.cs
@@ -40,8 +40,10 @@ public static void LoadSpecificArgs()
             {
                 using (var argv = new PyList(Runtime.Runtime.PySys_GetObject("argv")))
                 {
-                    Assert.AreEqual(args[0], argv[0].ToString());
-                    Assert.AreEqual(args[1], argv[1].ToString());
+                    using var v0 = argv[0];
+                    using var v1 = argv[1];
+                    Assert.AreEqual(args[0], v0.ToString());
+                    Assert.AreEqual(args[1], v1.ToString());
                 }
             }
         }
@@ -54,12 +56,16 @@ public void ImportClassShutdownRefcount()
 
             PyObject ns = Py.Import(typeof(ImportClassShutdownRefcountClass).Namespace);
             PyObject cls = ns.GetAttr(nameof(ImportClassShutdownRefcountClass));
+            BorrowedReference clsRef = cls.Reference;
+#pragma warning disable CS0618 // Type or member is obsolete
+            cls.Leak();
+#pragma warning restore CS0618 // Type or member is obsolete
             ns.Dispose();
 
-            Assert.Less(cls.Refcount, 256);
+            Assert.Less(Runtime.Runtime.Refcount(clsRef), 256);
 
             PythonEngine.Shutdown();
-            Assert.Greater(cls.Refcount, 0);
+            Assert.Greater(Runtime.Runtime.Refcount(clsRef), 0);
         }
 
         /// <summary>

diff --git a/src/runtime/finalizer.cs b/src/runtime/finalizer.cs
@@ -2,6 +2,7 @@
 using System.Collections.Concurrent;
 using System.Collections.Generic;
 using System.ComponentModel;
+using System.Diagnostics;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
@@ -30,10 +31,12 @@ public class ErrorArgs : EventArgs
         [DefaultValue(DefaultThreshold)]
         public int Threshold { get; set; } = DefaultThreshold;
 
+        bool started;
+
         [DefaultValue(true)]
         public bool Enable { get; set; } = true;
 
-        private ConcurrentQueue<IntPtr> _objQueue = new ConcurrentQueue<IntPtr>();
+        private ConcurrentQueue<PendingFinalization> _objQueue = new ();
         private int _throttled;
 
         #region FINALIZER_CHECK
@@ -79,6 +82,8 @@ internal IncorrectRefCountException(IntPtr ptr)
 
         internal void ThrottledCollect()
         {
+            if (!started) throw new InvalidOperationException($"{nameof(PythonEngine)} is not initialized");
+
             _throttled = unchecked(this._throttled + 1);
             if (!Enable || _throttled < Threshold) return;
             _throttled = 0;
@@ -87,12 +92,13 @@ internal void ThrottledCollect()
 
         internal List<IntPtr> GetCollectedObjects()
         {
-            return _objQueue.ToList();
+            return _objQueue.Select(o => o.PyObj).ToList();
         }
 
-        internal void AddFinalizedObject(ref IntPtr obj)
+        internal void AddFinalizedObject(ref IntPtr obj, int run)
         {
-            if (!Enable || obj == IntPtr.Zero)
+            Debug.Assert(obj != IntPtr.Zero);
+            if (!Enable)
             {
                 return;
             }
@@ -101,14 +107,20 @@ internal void AddFinalizedObject(ref IntPtr obj)
             lock (_queueLock)
 #endif
             {
-                this._objQueue.Enqueue(obj);
+                this._objQueue.Enqueue(new PendingFinalization { PyObj = obj, RuntimeRun = run });
             }
             obj = IntPtr.Zero;
         }
 
+        internal static void Initialize()
+        {
+            Instance.started = true;
+        }
+
         internal static void Shutdown()
         {
             Instance.DisposeAll();
+            Instance.started = false;
         }
 
         private void DisposeAll()
@@ -124,36 +136,31 @@ private void DisposeAll()
 #if FINALIZER_CHECK
                 ValidateRefCount();
 #endif
-                IntPtr obj;
                 Runtime.PyErr_Fetch(out var errType, out var errVal, out var traceback);
 
+                int run = Runtime.GetRun();
+
                 try
                 {
                     while (!_objQueue.IsEmpty)
                     {
-                        if (!_objQueue.TryDequeue(out obj))
+                        if (!_objQueue.TryDequeue(out var obj))
+                            continue;
+
+                        if (obj.RuntimeRun != run)
+                        {
+                            HandleFinalizationException(obj.PyObj, new RuntimeShutdownException(obj.PyObj));
                             continue;
+                        }
 
-                        Runtime.XDecref(obj);
+                        Runtime.XDecref(obj.PyObj);
                         try
                         {
                             Runtime.CheckExceptionOccurred();
                         }
                         catch (Exception e)
                         {
-                            var errorArgs = new ErrorArgs
-                            {
-                                Error = e,
-                            };
-
-                            ErrorHandler?.Invoke(this, errorArgs);
-
-                            if (!errorArgs.Handled)
-                            {
-                                throw new FinalizationException(
-                                    "Python object finalization failed",
-                                    disposable: obj, innerException: e);
-                            }
+                            HandleFinalizationException(obj.PyObj, e);
                         }
                     }
                 }
@@ -166,6 +173,23 @@ private void DisposeAll()
             }
         }
 
+        void HandleFinalizationException(IntPtr obj, Exception cause)
+        {
+            var errorArgs = new ErrorArgs
+            {
+                Error = cause,
+            };
+
+            ErrorHandler?.Invoke(this, errorArgs);
+
+            if (!errorArgs.Handled)
+            {
+                throw new FinalizationException(
+                    "Python object finalization failed",
+                    disposable: obj, innerException: cause);
+            }
+        }
+
 #if FINALIZER_CHECK
         private void ValidateRefCount()
         {
@@ -235,6 +259,12 @@ private void ValidateRefCount()
 #endif
     }
 
+    struct PendingFinalization
+    {
+        public IntPtr PyObj;
+        public int RuntimeRun;
+    }
+
     public class FinalizationException : Exception
     {
         public IntPtr Handle { get; }
@@ -259,5 +289,21 @@ public FinalizationException(string message, IntPtr disposable, Exception innerE
             if (disposable == IntPtr.Zero) throw new ArgumentNullException(nameof(disposable));
             this.Handle = disposable;
         }
+
+        protected FinalizationException(string message, IntPtr disposable)
+            : base(message)
+        {
+            if (disposable == IntPtr.Zero) throw new ArgumentNullException(nameof(disposable));
+            this.Handle = disposable;
+        }
+    }
+
+    public class RuntimeShutdownException : FinalizationException
+    {
+        public RuntimeShutdownException(IntPtr disposable)
+            : base("Python runtime was shut down after this object was created." +
+                   " It is an error to attempt to dispose or to continue using it even after restarting the runtime.", disposable)
+        {
+        }
     }
 }
diff --git a/src/runtime/pybuffer.cs b/src/runtime/pybuffer.cs
@@ -238,9 +238,10 @@ private void Dispose(bool disposing)
         {
             Debug.Assert(!disposedValue);
 
-            _exporter.CheckRun();
-
-            Finalizer.Instance.AddFinalizedObject(ref _view.obj);
+            if (_view.obj != IntPtr.Zero)
+            {
+                Finalizer.Instance.AddFinalizedObject(ref _view.obj, _exporter.run);
+            }
         }
 
         /// <summary>

diff --git a/src/runtime/pyobject.cs b/src/runtime/pyobject.cs
@@ -1,10 +1,10 @@
 using System;
-using System.Collections;
 using System.Collections.Generic;
 using System.Diagnostics;
 using System.Dynamic;
 using System.Linq;
 using System.Linq.Expressions;
+using System.Threading;
 
 namespace Python.Runtime
 {
@@ -27,7 +27,7 @@ public partial class PyObject : DynamicObject, IDisposable
 #endif  
 
         protected internal IntPtr obj = IntPtr.Zero;
-        readonly int run = Runtime.GetRun();
+        internal readonly int run = Runtime.GetRun();
 
         public static PyObject None => new PyObject(new BorrowedReference(Runtime.PyNone));
         internal BorrowedReference Reference => new BorrowedReference(this.obj);
@@ -96,13 +96,19 @@ internal PyObject(in StolenReference reference)
         // when the managed wrapper is garbage-collected.
         ~PyObject()
         {
-            Debug.Assert(obj != IntPtr.Zero);
+            Debug.Assert(obj != IntPtr.Zero || this.GetType() != typeof(PyObject));
+
+            if (obj != IntPtr.Zero)
+            {
 
 #if TRACE_ALLOC
-            CheckRun();
+                CheckRun();
 #endif
 
-            Finalizer.Instance.AddFinalizedObject(ref obj);
+                Interlocked.Increment(ref Runtime._collected);
+
+                Finalizer.Instance.AddFinalizedObject(ref obj, run);
+            }
 
             Dispose(false);
         }
@@ -231,12 +237,18 @@ public void Dispose()
             Dispose(true);
         }
 
+        [Obsolete("Test use only")]
+        internal void Leak()
+        {
+            Debug.Assert(obj != IntPtr.Zero);
+            GC.SuppressFinalize(this);
+            obj = IntPtr.Zero;
+        }
+
         internal void CheckRun()
         {
             if (run != Runtime.GetRun())
-                throw new InvalidOperationException(
-                    "PythonEngine was shut down after this object was created." +
-                    " It is an error to attempt to dispose or to continue using it.");
+                throw new RuntimeShutdownException(obj);
         }
 
         internal BorrowedReference GetPythonTypeReference()

diff --git a/src/runtime/runtime.cs b/src/runtime/runtime.cs
@@ -155,6 +155,7 @@ internal static void Initialize(bool initSigs = false, ShutdownMode mode = Shutd
             MainManagedThreadId = Thread.CurrentThread.ManagedThreadId;
 
             IsFinalizing = false;
+            Finalizer.Initialize();
             InternString.Initialize();
 
             InitPyMembers();
@@ -378,6 +379,8 @@ internal static void Shutdown(ShutdownMode mode)
             DisposeLazyModule(inspect);
             PyObjectConversions.Reset();
 
+            bool everythingSeemsCollected = TryCollectingGarbage();
+            Debug.Assert(everythingSeemsCollected);
             Finalizer.Shutdown();
             InternString.Shutdown();
 
@@ -423,6 +426,26 @@ internal static void Shutdown(ShutdownMode mode)
             }
         }
 
+        const int MaxCollectRetriesOnShutdown = 20;
+        internal static int _collected;
+        static bool TryCollectingGarbage()
+        {
+            for (int attempt = 0; attempt < MaxCollectRetriesOnShutdown; attempt++)
+            {
+                Interlocked.Exchange(ref _collected, 0);
+                nint pyCollected = 0;
+                for (int i = 0; i < 2; i++)
+                {
+                    GC.Collect();
+                    GC.WaitForPendingFinalizers();
+                    pyCollected += PyGC_Collect();
+                }
+                if (Volatile.Read(ref _collected) == 0 && pyCollected == 0)
+                    return true;
+            }
+            return false;
+        }
+
         internal static void Shutdown()
         {
             var mode = ShutdownMode;
@@ -818,6 +841,10 @@ internal static unsafe long Refcount(IntPtr op)
             return *p;
         }
 
+        [Pure]
+        internal static long Refcount(BorrowedReference op)
+            => Refcount(op.DangerousGetAddress());
+
         /// <summary>
         /// Call specified function, and handle PythonDLL-related failures.
         /// </summary>
@@ -2212,7 +2239,7 @@ internal static int PyException_SetTraceback(BorrowedReference ex, BorrowedRefer
 
 
 
-        internal static IntPtr PyGC_Collect() => Delegates.PyGC_Collect();
+        internal static nint PyGC_Collect() => Delegates.PyGC_Collect();
 
         internal static IntPtr _Py_AS_GC(BorrowedReference ob)
         {
@@ -2592,7 +2619,7 @@ static Delegates()
                 PyErr_Print = (delegate* unmanaged[Cdecl]<void>)GetFunctionByName(nameof(PyErr_Print), GetUnmanagedDll(_PythonDll));
                 PyCell_Get = (delegate* unmanaged[Cdecl]<BorrowedReference, NewReference>)GetFunctionByName(nameof(PyCell_Get), GetUnmanagedDll(_PythonDll));
                 PyCell_Set = (delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, int>)GetFunctionByName(nameof(PyCell_Set), GetUnmanagedDll(_PythonDll));
-                PyGC_Collect = (delegate* unmanaged[Cdecl]<IntPtr>)GetFunctionByName(nameof(PyGC_Collect), GetUnmanagedDll(_PythonDll));
+                PyGC_Collect = (delegate* unmanaged[Cdecl]<nint>)GetFunctionByName(nameof(PyGC_Collect), GetUnmanagedDll(_PythonDll));
                 PyCapsule_New = (delegate* unmanaged[Cdecl]<IntPtr, IntPtr, IntPtr, NewReference>)GetFunctionByName(nameof(PyCapsule_New), GetUnmanagedDll(_PythonDll));
                 PyCapsule_GetPointer = (delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, IntPtr>)GetFunctionByName(nameof(PyCapsule_GetPointer), GetUnmanagedDll(_PythonDll));
                 PyCapsule_SetPointer = (delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, int>)GetFunctionByName(nameof(PyCapsule_SetPointer), GetUnmanagedDll(_PythonDll));
@@ -2871,7 +2898,7 @@ static Delegates()
             internal static delegate* unmanaged[Cdecl]<void> PyErr_Print { get; }
             internal static delegate* unmanaged[Cdecl]<BorrowedReference, NewReference> PyCell_Get { get; }
             internal static delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, int> PyCell_Set { get; }
-            internal static delegate* unmanaged[Cdecl]<IntPtr> PyGC_Collect { get; }
+            internal static delegate* unmanaged[Cdecl]<nint> PyGC_Collect { get; }
             internal static delegate* unmanaged[Cdecl]<IntPtr, IntPtr, IntPtr, NewReference> PyCapsule_New { get; }
             internal static delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, IntPtr> PyCapsule_GetPointer { get; }
             internal static delegate* unmanaged[Cdecl]<BorrowedReference, IntPtr, int> PyCapsule_SetPointer { get; }