Defer clear for weakrefs without callbacks.

This fixes GH-132413 and GH-135552.

Author: nascheme

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

@@ -658,9 +658,8 @@ 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)
+            # The weakref should be cleared before executing the callback.
+            self.assertIsNone(x)
 
     def test_bug21435(self):
         # This is a poor test - its only virtue is that it happened to
@@ -1047,8 +1046,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()
 
@@ -1330,6 +1329,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.
@@ -1405,6 +1405,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

@@ -808,7 +808,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
@@ -1809,7 +1814,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
@@ -2158,7 +2167,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")
@@ -4080,7 +4093,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:
 

Python/gc.c

@@ -920,14 +920,8 @@ handle_weakref_callbacks(PyGC_Head *unreachable, PyGC_Head *old)
             // is called, the cache may not be correctly invalidated.  That
             // can lead to segfaults since the caches can refer to deallocated
             // objects.  Delaying the clear of weakrefs until *after*
-            // finalizers have been called fixes that bug.  However, that
-            // deferral could introduce other problems if some finalizer
-            // code expects that the weakrefs will be cleared first.  So, we
-            // have the PyType_Check() test below to only defer the clear of
-            // weakrefs to types.  That solves the issue for tp_subclasses.
-            // In a future version of Python, we should likely defer the
-            // weakref clear for all objects, not just types.
-            if (wr->wr_callback != NULL || !PyType_Check(wr->wr_object)) {
+            // finalizers have been called fixes that bug.
+            if (wr->wr_callback != NULL) {
                 // _PyWeakref_ClearRef clears the weakref but leaves the
                 // callback pointer intact.  Obscure: it also changes *wrlist.
                 _PyObject_ASSERT((PyObject *)wr, wr->wr_object == op);

Python/gc_free_threading.c

@@ -1532,14 +1532,8 @@ find_weakref_callbacks(struct collection_state *state)
             // is called, the cache may not be correctly invalidated.  That
             // can lead to segfaults since the caches can refer to deallocated
             // objects.  Delaying the clear of weakrefs until *after*
-            // finalizers have been called fixes that bug.  However, that
-            // deferral could introduce other problems if some finalizer
-            // code expects that the weakrefs will be cleared first.  So, we
-            // have the PyType_Check() test below to only defer the clear of
-            // weakrefs to types.  That solves the issue for tp_subclasses.
-            // In a future version of Python, we should likely defer the
-            // weakref clear for all objects, not just types.
-            if (wr->wr_callback != NULL || !PyType_Check(wr->wr_object)) {
+            // finalizers have been called fixes that bug.
+            if (wr->wr_callback != NULL) {
                 // _PyWeakref_ClearRef clears the weakref but leaves the
                 // callback pointer intact.  Obscure: it also changes *wrlist.
                 _PyObject_ASSERT((PyObject *)wr, wr->wr_object == op);