diff --git a/src/embed_tests/TestDomainReload.cs b/src/embed_tests/TestDomainReload.cs index a606932f3..3e0a18c70 100644 --- a/src/embed_tests/TestDomainReload.cs +++ b/src/embed_tests/TestDomainReload.cs @@ -191,7 +191,7 @@ from Python.EmbeddingTest.Domain import MyClass def test_obj_call(): obj = MyClass() obj.Method() - obj.StaticMethod() + MyClass.StaticMethod() obj.Property = 1 obj.Field = 10 @@ -288,7 +288,7 @@ void ExecTest() GC.Collect(); GC.WaitForPendingFinalizers(); // <- this will put former `num` into Finalizer queue - Finalizer.Instance.Collect(forceDispose: true); + Finalizer.Instance.Collect(); // ^- this will call PyObject.Dispose, which will call XDecref on `num.Handle`, // but Python interpreter from "run" 1 is long gone, so it will corrupt memory instead. Assert.False(numRef.IsAlive); @@ -333,7 +333,7 @@ void ExecTest() PythonEngine.Initialize(); // <- "run" 2 starts GC.Collect(); GC.WaitForPendingFinalizers(); - Finalizer.Instance.Collect(forceDispose: true); + Finalizer.Instance.Collect(); Assert.False(objRef.IsAlive); } finally diff --git a/src/embed_tests/TestFinalizer.cs b/src/embed_tests/TestFinalizer.cs index 6e2160762..46e2fcdf1 100644 --- a/src/embed_tests/TestFinalizer.cs +++ b/src/embed_tests/TestFinalizer.cs @@ -2,9 +2,9 @@ using Python.Runtime; using System; using System.Collections.Generic; -using System.ComponentModel; using System.Diagnostics; using System.Linq; +using System.Runtime.CompilerServices; using System.Threading; namespace Python.EmbeddingTest @@ -28,26 +28,14 @@ public void TearDown() PythonEngine.Shutdown(); } - private static bool FullGCCollect() + private static void FullGCCollect() { - GC.Collect(GC.MaxGeneration, GCCollectionMode.Forced); - try - { - return GC.WaitForFullGCComplete() == GCNotificationStatus.Succeeded; - } - catch (NotImplementedException) - { - // Some clr runtime didn't implement GC.WaitForFullGCComplete yet. - return false; - } - finally - { - GC.WaitForPendingFinalizers(); - } + GC.Collect(); + GC.WaitForPendingFinalizers(); } [Test] - [Ignore("Ignore temporarily")] + [Obsolete("GC tests are not guaranteed")] public void CollectBasicObject() { Assert.IsTrue(Finalizer.Instance.Enable); @@ -64,11 +52,7 @@ public void CollectBasicObject() Assert.IsFalse(called, "The event handler was called before it was installed"); Finalizer.Instance.CollectOnce += handler; - WeakReference shortWeak; - WeakReference longWeak; - { - MakeAGarbage(out shortWeak, out longWeak); - } + IntPtr pyObj = MakeAGarbage(out var shortWeak, out var longWeak); FullGCCollect(); // The object has been resurrected Warn.If( @@ -86,7 +70,7 @@ public void CollectBasicObject() var garbage = Finalizer.Instance.GetCollectedObjects(); Assert.NotZero(garbage.Count, "There should still be garbage around"); Warn.Unless( - garbage.Any(T => ReferenceEquals(T.Target, longWeak.Target)), + garbage.Contains(pyObj), $"The {nameof(longWeak)} reference doesn't show up in the garbage list", garbage ); @@ -104,20 +88,15 @@ public void CollectBasicObject() } [Test] - [Ignore("Ignore temporarily")] + [Obsolete("GC tests are not guaranteed")] public void CollectOnShutdown() { IntPtr op = MakeAGarbage(out var shortWeak, out var longWeak); - int hash = shortWeak.Target.GetHashCode(); - List garbage; - if (!FullGCCollect()) - { - Assert.IsTrue(WaitForCollected(op, hash, 10000)); - } + FullGCCollect(); Assert.IsFalse(shortWeak.IsAlive); - garbage = Finalizer.Instance.GetCollectedObjects(); + List garbage = Finalizer.Instance.GetCollectedObjects(); Assert.IsNotEmpty(garbage, "The garbage object should be collected"); - Assert.IsTrue(garbage.Any(r => ReferenceEquals(r.Target, longWeak.Target)), + Assert.IsTrue(garbage.Contains(op), "Garbage should contains the collected object"); PythonEngine.Shutdown(); @@ -125,12 +104,29 @@ public void CollectOnShutdown() Assert.IsEmpty(garbage); } + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] // ensure lack of references to obj + [Obsolete("GC tests are not guaranteed")] private static IntPtr MakeAGarbage(out WeakReference shortWeak, out WeakReference longWeak) { - PyLong obj = new PyLong(1024); - shortWeak = new WeakReference(obj); - longWeak = new WeakReference(obj, true); - return obj.Handle; + IntPtr handle = IntPtr.Zero; + WeakReference @short = null, @long = null; + // must create Python object in the thread where we have GIL + IntPtr val = PyLong.FromLong(1024); + // must create temp object in a different thread to ensure it is not present + // when conservatively scanning stack for GC roots. + // see https://xamarin.github.io/bugzilla-archives/17/17593/bug.html + var garbageGen = new Thread(() => + { + var obj = new PyObject(val, skipCollect: true); + @short = new WeakReference(obj); + @long = new WeakReference(obj, true); + handle = obj.Handle; + }); + garbageGen.Start(); + Assert.IsTrue(garbageGen.Join(TimeSpan.FromSeconds(5)), "Garbage creation timed out"); + shortWeak = @short; + longWeak = @long; + return handle; } private static long CompareWithFinalizerOn(PyObject pyCollect, bool enbale) @@ -191,62 +187,6 @@ public void SimpleTestMemory() } } - 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; - var errorMessage = ""; - EventHandler handleFunc = (sender, args) => - { - called = true; - errorMessage = args.Error.Message; - }; - 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; - } - Assert.AreEqual(errorMessage, "MyPyObject"); - } - [Test] public void ValidateRefCount() { @@ -279,6 +219,7 @@ public void ValidateRefCount() } } + [MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.NoOptimization)] // ensure lack of references to s1 and s2 private static IntPtr CreateStringGarbage() { PyString s1 = new PyString("test_string"); @@ -286,29 +227,5 @@ private static IntPtr CreateStringGarbage() PyString s2 = new PyString(s1.Handle); return s1.Handle; } - - private static bool WaitForCollected(IntPtr op, int hash, int milliseconds) - { - var stopwatch = Stopwatch.StartNew(); - do - { - var garbage = Finalizer.Instance.GetCollectedObjects(); - foreach (var item in garbage) - { - // The validation is not 100% precise, - // but it's rare that two conditions satisfied but they're still not the same object. - if (item.Target.GetHashCode() != hash) - { - continue; - } - var obj = (IPyDisposable)item.Target; - if (obj.GetTrackedHandles().Contains(op)) - { - return true; - } - } - } while (stopwatch.ElapsedMilliseconds < milliseconds); - return false; - } } } diff --git a/src/runtime/debughelper.cs b/src/runtime/debughelper.cs index babe726c6..5e854bffd 100644 --- a/src/runtime/debughelper.cs +++ b/src/runtime/debughelper.cs @@ -137,5 +137,12 @@ public static void PrintHexBytes(byte[] bytes) Console.WriteLine(); } } + + [Conditional("DEBUG")] + public static void AssertHasReferences(IntPtr obj) + { + long refcount = Runtime.Refcount(obj); + Debug.Assert(refcount > 0, "Object refcount is 0 or less"); + } } } diff --git a/src/runtime/delegatemanager.cs b/src/runtime/delegatemanager.cs index bd8f1ee4c..a8ab6d2b5 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 : IPyDisposable + public class Dispatcher { public IntPtr target; public Type dtype; @@ -202,7 +202,7 @@ public Dispatcher(IntPtr target, Type dtype) return; } _finalized = true; - Finalizer.Instance.AddFinalizedObject(this); + Finalizer.Instance.AddFinalizedObject(ref target); } public void Dispose() @@ -276,11 +276,6 @@ 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 70b69345b..3861ec6cb 100644 --- a/src/runtime/finalizer.cs +++ b/src/runtime/finalizer.cs @@ -27,7 +27,7 @@ public class ErrorArgs : EventArgs public int Threshold { get; set; } public bool Enable { get; set; } - private ConcurrentQueue _objQueue = new ConcurrentQueue(); + private ConcurrentQueue _objQueue = new ConcurrentQueue(); private int _throttled; #region FINALIZER_CHECK @@ -42,7 +42,7 @@ public class ErrorArgs : EventArgs public class IncorrectFinalizeArgs : EventArgs { public IntPtr Handle { get; internal set; } - public ICollection ImpactedObjects { get; internal set; } + public ICollection ImpactedObjects { get; internal set; } } public class IncorrectRefCountException : Exception @@ -73,8 +73,6 @@ private Finalizer() Threshold = 200; } - [Obsolete("forceDispose parameter is unused. All objects are disposed regardless.")] - public void Collect(bool forceDispose) => this.DisposeAll(); public void Collect() => this.DisposeAll(); internal void ThrottledCollect() @@ -85,14 +83,14 @@ internal void ThrottledCollect() this.Collect(); } - public List GetCollectedObjects() + internal List GetCollectedObjects() { - return _objQueue.Select(T => new WeakReference(T)).ToList(); + return _objQueue.ToList(); } - internal void AddFinalizedObject(IPyDisposable obj) + internal void AddFinalizedObject(ref IntPtr obj) { - if (!Enable) + if (!Enable || obj == IntPtr.Zero) { return; } @@ -103,6 +101,7 @@ internal void AddFinalizedObject(IPyDisposable obj) { this._objQueue.Enqueue(obj); } + obj = IntPtr.Zero; } internal static void Shutdown() @@ -123,29 +122,44 @@ private void DisposeAll() #if FINALIZER_CHECK ValidateRefCount(); #endif - IPyDisposable obj; - while (_objQueue.TryDequeue(out obj)) + IntPtr obj; + Runtime.PyErr_Fetch(out var errType, out var errVal, out var traceback); + + try { - try - { - obj.Dispose(); - } - catch (Exception e) + while (!_objQueue.IsEmpty) { - var handler = ErrorHandler; - if (handler is null) + if (!_objQueue.TryDequeue(out obj)) + continue; + + Runtime.XDecref(obj); + try { - throw new FinalizationException( - "Python object finalization failed", - disposable: obj, innerException: e); + Runtime.CheckExceptionOccurred(); } - - handler.Invoke(this, new ErrorArgs() + catch (Exception e) { - Error = e - }); + var handler = ErrorHandler; + if (handler is null) + { + throw new FinalizationException( + "Python object finalization failed", + disposable: obj, innerException: e); + } + + handler.Invoke(this, new ErrorArgs() + { + Error = e + }); + } } } + finally + { + // Python requires finalizers to preserve exception: + // https://docs.python.org/3/extending/newtypes.html#finalization-and-de-allocation + Runtime.PyErr_Restore(errType, errVal, traceback); + } } } @@ -158,33 +172,26 @@ private void ValidateRefCount() } var counter = new Dictionary(); var holdRefs = new Dictionary(); - var indexer = new Dictionary>(); + var indexer = new Dictionary>(); foreach (var obj in _objQueue) { - IntPtr[] handles = obj.GetTrackedHandles(); - foreach (var handle in handles) + var handle = obj; + if (!counter.ContainsKey(handle)) { - 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); + 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) { @@ -227,12 +234,13 @@ private void ValidateRefCount() public class FinalizationException : Exception { - public IPyDisposable Disposable { get; } + public IntPtr PythonObject { get; } - public FinalizationException(string message, IPyDisposable disposable, Exception innerException) + public FinalizationException(string message, IntPtr disposable, Exception innerException) : base(message, innerException) { - this.Disposable = disposable ?? throw new ArgumentNullException(nameof(disposable)); + if (disposable == IntPtr.Zero) throw new ArgumentNullException(nameof(disposable)); + this.PythonObject = disposable; } } } diff --git a/src/runtime/pybuffer.cs b/src/runtime/pybuffer.cs index eadf4e2a7..cf657a033 100644 --- a/src/runtime/pybuffer.cs +++ b/src/runtime/pybuffer.cs @@ -7,7 +7,7 @@ namespace Python.Runtime { - public sealed class PyBuffer : IPyDisposable + public sealed class PyBuffer : IDisposable { private PyObject _exporter; private Py_buffer _view; @@ -236,7 +236,7 @@ private void Dispose(bool disposing) { return; } - Finalizer.Instance.AddFinalizedObject(this); + Finalizer.Instance.AddFinalizedObject(ref _view.obj); } /// @@ -248,10 +248,5 @@ public void Dispose() Dispose(true); GC.SuppressFinalize(this); } - - public IntPtr[] GetTrackedHandles() - { - return new IntPtr[] { _view.obj }; - } } } diff --git a/src/runtime/pylong.cs b/src/runtime/pylong.cs index 2f85824de..0e21c7c49 100644 --- a/src/runtime/pylong.cs +++ b/src/runtime/pylong.cs @@ -74,7 +74,7 @@ public PyLong(uint value) : base(FromInt((int)value)) } - private static IntPtr FromLong(long value) + internal static IntPtr FromLong(long value) { IntPtr val = Runtime.PyLong_FromLongLong(value); PythonException.ThrowIfIsNull(val); diff --git a/src/runtime/pyobject.cs b/src/runtime/pyobject.cs index 9328312ce..6957db8df 100644 --- a/src/runtime/pyobject.cs +++ b/src/runtime/pyobject.cs @@ -8,11 +8,6 @@ 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 @@ -21,7 +16,7 @@ public interface IPyDisposable : IDisposable /// for details. /// [Serializable] - public partial class PyObject : DynamicObject, IEnumerable, IPyDisposable + public partial class PyObject : DynamicObject, IEnumerable, IDisposable { #if TRACE_ALLOC /// @@ -54,6 +49,19 @@ public PyObject(IntPtr ptr) #endif } + [Obsolete("for testing purposes only")] + internal PyObject(IntPtr ptr, bool skipCollect) + { + if (ptr == IntPtr.Zero) throw new ArgumentNullException(nameof(ptr)); + + obj = ptr; + if (!skipCollect) + Finalizer.Instance.ThrottledCollect(); +#if TRACE_ALLOC + Traceback = new StackTrace(1); +#endif + } + /// /// Creates new pointing to the same object as /// the . Increments refcount, allowing @@ -78,7 +86,7 @@ internal PyObject(BorrowedReference reference) { return; } - Finalizer.Instance.AddFinalizedObject(this); + Finalizer.Instance.AddFinalizedObject(ref obj); } @@ -202,6 +210,10 @@ protected virtual void Dispose(bool disposing) Runtime.XDecref(this.obj); } } + else + { + throw new InvalidOperationException("Runtime is already finalizing"); + } this.obj = IntPtr.Zero; } @@ -211,11 +223,6 @@ 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 20c933ad2..d61573733 100644 --- a/src/runtime/pyscope.cs +++ b/src/runtime/pyscope.cs @@ -22,14 +22,14 @@ public class PyGILAttribute : Attribute } [PyGIL] - public class PyScope : DynamicObject, IPyDisposable + public class PyScope : DynamicObject, IDisposable { public readonly string Name; /// /// the python Module object the scope associated with. /// - internal readonly IntPtr obj; + internal IntPtr obj; /// /// the variable dict of the scope. @@ -522,11 +522,6 @@ public void Dispose() this.OnDispose?.Invoke(this); } - public IntPtr[] GetTrackedHandles() - { - return new IntPtr[] { obj }; - } - ~PyScope() { if (_finalized || _isDisposed) @@ -534,7 +529,7 @@ public IntPtr[] GetTrackedHandles() return; } _finalized = true; - Finalizer.Instance.AddFinalizedObject(this); + Finalizer.Instance.AddFinalizedObject(ref obj); } } diff --git a/src/runtime/pythonexception.cs b/src/runtime/pythonexception.cs index 1e10967d7..97a80bc76 100644 --- a/src/runtime/pythonexception.cs +++ b/src/runtime/pythonexception.cs @@ -8,7 +8,7 @@ namespace Python.Runtime /// Provides a managed interface to exceptions thrown by the Python /// runtime. /// - public class PythonException : System.Exception, IPyDisposable + public class PythonException : System.Exception, IDisposable { private IntPtr _pyType = IntPtr.Zero; private IntPtr _pyValue = IntPtr.Zero; @@ -67,7 +67,9 @@ public PythonException() return; } _finalized = true; - Finalizer.Instance.AddFinalizedObject(this); + Finalizer.Instance.AddFinalizedObject(ref _pyType); + Finalizer.Instance.AddFinalizedObject(ref _pyValue); + Finalizer.Instance.AddFinalizedObject(ref _pyTB); } /// @@ -233,11 +235,6 @@ public void Dispose() } } - public IntPtr[] GetTrackedHandles() - { - return new IntPtr[] { _pyType, _pyValue, _pyTB }; - } - /// /// Matches Method ///