fixed CollectBasicObject by ensuring MakeAGarbage is not keeping temp object alive

lostmsu · lostmsu · commit 45c370a725da · 2020-12-10T16:30:13.000-08:00
diff --git 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);
@@ -104,18 +92,13 @@ 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<WeakReference> garbage;
-            if (!FullGCCollect())
-            {
-                Assert.IsTrue(WaitForCollected(op, hash, 10000));
-            }
+            FullGCCollect();
             Assert.IsFalse(shortWeak.IsAlive);
-            garbage = Finalizer.Instance.GetCollectedObjects();
+            List<WeakReference> garbage = Finalizer.Instance.GetCollectedObjects();
             Assert.IsNotEmpty(garbage, "The garbage object should be collected");
             Assert.IsTrue(garbage.Any(r => ReferenceEquals(r.Target, longWeak.Target)),
                 "Garbage should contains the collected object");
@@ -125,12 +108,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)
@@ -211,6 +211,7 @@ internal static void CreateMyPyObject(IntPtr op)
         }
 
         [Test]
+        [Obsolete("GC tests are not guaranteed")]
         public void ErrorHandling()
         {
             bool called = false;
@@ -228,7 +229,7 @@ public void ErrorHandling()
                 WeakReference longWeak;
                 {
                     MakeAGarbage(out shortWeak, out longWeak);
-                    var obj = (PyLong)longWeak.Target;
+                    var obj = (PyObject)longWeak.Target;
                     IntPtr handle = obj.Handle;
                     shortWeak = null;
                     longWeak = null;
@@ -279,36 +280,13 @@ 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");
             // s2 steal a reference from s1
             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/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
@@ -54,6 +54,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
+        }
+
         /// <summary>
         /// Creates new <see cref="PyObject"/> pointing to the same object as
         /// the <paramref name="reference"/>. Increments refcount, allowing <see cref="PyObject"/>

Original file line number	Diff line number	Diff line change
`@@ -74,7 +74,7 @@ public PyLong(uint value) : base(FromInt((int)value))`
`74`	`74`	`}`
`75`	`75`
`76`	`76`
`77`		`- private static IntPtr FromLong(long value)`
	`77`	`+ internal static IntPtr FromLong(long value)`
`78`	`78`	`{`
`79`	`79`	`IntPtr val = Runtime.PyLong_FromLongLong(value);`
`80`	`80`	`PythonException.ThrowIfIsNull(val);`