Make the GC clear weakrefs later.

Clear the weakrefs to unreachable objects after finalizers are called.

Author: nascheme

Lib/_threading_local.py

@@ -49,7 +49,7 @@ def local_deleted(_, key=key):
             # When the localimpl is deleted, remove the thread attribute.
             thread = wrthread()
             if thread is not None:
-                del thread.__dict__[key]
+                thread.__dict__.pop(key, None)
         def thread_deleted(_, idt=idt):
             # When the thread is deleted, remove the local dict.
             # Note that this is suboptimal if the thread object gets

Lib/test/test_finalization.py

@@ -280,8 +280,9 @@ def test_simple_resurrect(self):
             gc.collect()
             self.assert_del_calls(ids)
             self.assert_survivors(ids)
-            # XXX is this desirable?
-            self.assertIs(wr(), None)
+            # This used to be None because weakrefs were cleared before
+            # calling finalizers.  Now they are cleared after.
+            self.assertIsNot(wr(), None)
             # When trying to destroy the object a second time, __del__
             # isn't called anymore (and the object isn't resurrected).
             self.clear_survivors()
@@ -388,8 +389,10 @@ def check_resurrecting_chain(self, classes):
             gc.collect()
             self.assert_del_calls(ids)
             self.assert_survivors(survivor_ids)
-            # XXX desirable?
-            self.assertEqual([wr() for wr in wrs], [None] * N)
+            for wr in wrs:
+                # These values used to be None because weakrefs were cleared
+                # before calling finalizers.  Now they are cleared after.
+                self.assertIsNotNone(wr())
             self.clear_survivors()
             gc.collect()
             self.assert_del_calls(ids)

Lib/test/test_gc.py

@@ -262,9 +262,11 @@ class Cyclic(tuple):
             #    finalizer.
             def __del__(self):
 
-                # 5. Create a weakref to `func` now. If we had created
-                #    it earlier, it would have been cleared by the
-                #    garbage collector before calling the finalizers.
+                # 5. Create a weakref to `func` now. In previous
+                #    versions of Python, this would avoid having it
+                #    cleared by the garbage collector before calling
+                #    the finalizers.  Now, weakrefs get cleared after
+                #    calling finalizers.
                 self[1].ref = weakref.ref(self[0])
 
                 # 6. Drop the global reference to `latefin`. The only
@@ -293,14 +295,18 @@ def func():
         #    which will find `cyc` and `func` as garbage.
         gc.collect()
 
-        # 9. Previously, this would crash because `func_qualname`
-        #    had been NULL-ed out by func_clear().
+        # 9. Previously, this would crash because the weakref
+        #    created in the finalizer revealed the function after
+        #    `tp_clear` was called and `func_qualname`
+        #    had been NULL-ed out by func_clear().  Now, we clear
+        #    weakrefs to unreachable objects before calling `tp_clear`
+        #    but after calling finalizers.
         print(f"{func=}")
         """
-        # We're mostly just checking that this doesn't crash.
         rc, stdout, stderr = assert_python_ok("-c", code)
         self.assertEqual(rc, 0)
-        self.assertRegex(stdout, rb"""\A\s*func=<function  at \S+>\s*\z""")
+        # The `func` global is None because the weakref was cleared.
+        self.assertRegex(stdout, rb"""\A\s*func=None""")
         self.assertFalse(stderr)
 
     @refcount_test
@@ -652,9 +658,11 @@ def callback(ignored):
         gc.collect()
         self.assertEqual(len(ouch), 2)  # else the callbacks didn't run
         for x in ouch:
-            # If the callback resurrected one of these guys, the instance
-            # would be damaged, with an empty __dict__.
-            self.assertEqual(x, None)
+            # In previous versions of Python, the weakrefs are cleared
+            # before calling finalizers.  Now they are cleared after.
+            # So when the object is resurrected, the weakref is not
+            # cleared since it is no longer unreachable.
+            self.assertIsInstance(x, C1055820)
 
     def test_bug21435(self):
         # This is a poor test - its only virtue is that it happened to
@@ -1041,8 +1049,8 @@ class Z:
         # release references and create trash
         del a, wr_cycle
         gc.collect()
-        # if called, it means there is a bug in the GC.  The weakref should be
-        # cleared before Z dies.
+        # In older versions of Python, the weakref was cleared by the
+        # gc.  Now it is not cleared and so the callback is run.
         callback.assert_not_called()
         gc.enable()
 
@@ -1324,6 +1332,7 @@ def setUp(self):
     def tearDown(self):
         gc.disable()
 
+    @unittest.skipIf(Py_GIL_DISABLED, "requires GC generations or increments")
     def test_bug1055820c(self):
         # Corresponds to temp2c.py in the bug report.  This is pretty
         # elaborate.
@@ -1399,6 +1408,7 @@ def callback(ignored):
             self.assertEqual(x, None)
 
     @gc_threshold(1000, 0, 0)
+    @unittest.skipIf(Py_GIL_DISABLED, "requires GC generations or increments")
     def test_bug1055820d(self):
         # Corresponds to temp2d.py in the bug report.  This is very much like
         # test_bug1055820c, but uses a __del__ method instead of a weakref

Lib/test/test_io.py

@@ -807,7 +807,12 @@ def test_closefd_attr(self):
     def test_garbage_collection(self):
         # FileIO objects are collected, and collecting them flushes
         # all data to disk.
-        with warnings_helper.check_warnings(('', ResourceWarning)):
+        #
+        # Note that using warnings_helper.check_warnings() will keep the
+        # file alive due to the `source` argument to warn().  So, use
+        # catch_warnings() instead.
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore", ResourceWarning)
             f = self.FileIO(os_helper.TESTFN, "wb")
             f.write(b"abcxxx")
             f.f = f
@@ -1808,7 +1813,11 @@ def test_garbage_collection(self):
         # C BufferedReader objects are collected.
         # The Python version has __del__, so it ends into gc.garbage instead
         self.addCleanup(os_helper.unlink, os_helper.TESTFN)
-        with warnings_helper.check_warnings(('', ResourceWarning)):
+        # Note that using warnings_helper.check_warnings() will keep the
+        # file alive due to the `source` argument to warn().  So, use
+        # catch_warnings() instead.
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore", ResourceWarning)
             rawio = self.FileIO(os_helper.TESTFN, "w+b")
             f = self.tp(rawio)
             f.f = f
@@ -2157,7 +2166,11 @@ def test_garbage_collection(self):
         # all data to disk.
         # The Python version has __del__, so it ends into gc.garbage instead
         self.addCleanup(os_helper.unlink, os_helper.TESTFN)
-        with warnings_helper.check_warnings(('', ResourceWarning)):
+        # Note that using warnings_helper.check_warnings() will keep the
+        # file alive due to the `source` argument to warn().  So, use
+        # catch_warnings() instead.
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore", ResourceWarning)
             rawio = self.FileIO(os_helper.TESTFN, "w+b")
             f = self.tp(rawio)
             f.write(b"123xxx")
@@ -4079,7 +4092,8 @@ def test_garbage_collection(self):
         # C TextIOWrapper objects are collected, and collecting them flushes
         # all data to disk.
         # The Python version has __del__, so it ends in gc.garbage instead.
-        with warnings_helper.check_warnings(('', ResourceWarning)):
+        with warnings.catch_warnings():
+            warnings.simplefilter("ignore", ResourceWarning)
             rawio = self.FileIO(os_helper.TESTFN, "wb")
             b = self.BufferedWriter(rawio)
             t = self.TextIOWrapper(b, encoding="ascii")

Lib/test/test_weakref.py

@@ -1314,7 +1314,7 @@ def check_len_cycles(self, dict_type, cons):
         n2 = len(dct)
         # one item may be kept alive inside the iterator
         self.assertIn(n1, (0, 1))
-        self.assertEqual(n2, 0)
+        self.assertIn(n2, (0, 1))
 
     def test_weak_keyed_len_cycles(self):
         self.check_len_cycles(weakref.WeakKeyDictionary, lambda k: (k, 1))

Modules/_datetimemodule.c

@@ -218,7 +218,7 @@ clear_current_module(PyInterpreterState *interp, PyObject *expected)
         if (PyDict_GetItemRef(dict, INTERP_KEY, &ref) < 0) {
             goto error;
         }
-        if (ref != NULL) {
+        if (ref != NULL && ref != Py_None) {
             PyObject *current = NULL;
             int rc = PyWeakref_GetRef(ref, &current);
             /* We only need "current" for pointer comparison. */

Modules/gc_weakref.txt

@@ -1,6 +1,16 @@
 Intro
 =====
 
+**************************************************************************
+Note: this document was written long ago, before PEP 442 (safe object
+finalization) was implemented.  While that has changed some things, this
+document is still mostly accurate. Just note that the rules being discussed
+here apply to the unreachable set of objects *after* non-legacy finalizers
+have been called.  Also, the clearing of weakrefs has been changed to happen
+later in the collection (after running finalizers but before tp_clear is
+called).
+**************************************************************************
+
 The basic rule for dealing with weakref callbacks (and __del__ methods too,
 for that matter) during cyclic gc: