Defer weakref clears only for refs to classes.

This avoids breaking tests while fixing the issue with tp_subclasses. In
the long term, it would be better to defer the clear of all weakrefs,
not just the ones referring to classes.  However, that is a more
distruptive change and would seem to have a higher chance of breaking
user code.  So, it would not be something to do in a bugfix release.

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:
-                thread.__dict__.pop(key, None)
+                del thread.__dict__[key]
         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,9 +280,8 @@ def test_simple_resurrect(self):
             gc.collect()
             self.assert_del_calls(ids)
             self.assert_survivors(ids)
-            # This used to be None because weakrefs were cleared before
-            # calling finalizers.  Now they are cleared after.
-            self.assertIsNot(wr(), None)
+            # XXX is this desirable?
+            self.assertIs(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()
@@ -389,10 +388,8 @@ def check_resurrecting_chain(self, classes):
             gc.collect()
             self.assert_del_calls(ids)
             self.assert_survivors(survivor_ids)
-            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())
+            # XXX desirable?
+            self.assertEqual([wr() for wr in wrs], [None] * N)
             self.clear_survivors()
             gc.collect()
             self.assert_del_calls(ids)

Lib/test/test_gc.py

@@ -658,11 +658,9 @@ def callback(ignored):
         gc.collect()
         self.assertEqual(len(ouch), 2)  # else the callbacks didn't run
         for x in ouch:
-            # 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)
+            # If the callback resurrected one of these guys, the instance
+            # would be damaged, with an empty __dict__.
+            self.assertEqual(x, None)
 
     def test_bug21435(self):
         # This is a poor test - its only virtue is that it happened to
@@ -1049,8 +1047,8 @@ class Z:
         # release references and create trash
         del a, wr_cycle
         gc.collect()
-        # In older versions of Python, the weakref was cleared by the
-        # gc.  Now it is not cleared and so the callback is run.
+        # if called, it means there is a bug in the GC.  The weakref should be
+        # cleared before Z dies.
         callback.assert_not_called()
         gc.enable()
 
@@ -1332,7 +1330,6 @@ 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.
@@ -1408,7 +1405,6 @@ 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,12 +807,7 @@ def test_closefd_attr(self):
     def test_garbage_collection(self):
         # FileIO objects are collected, and collecting them flushes
         # all data to disk.
-        #
-        # 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)
+        with warnings_helper.check_warnings(('', ResourceWarning)):
             f = self.FileIO(os_helper.TESTFN, "wb")
             f.write(b"abcxxx")
             f.f = f
@@ -1813,11 +1808,7 @@ 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)
-        # 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)
+        with warnings_helper.check_warnings(('', ResourceWarning)):
             rawio = self.FileIO(os_helper.TESTFN, "w+b")
             f = self.tp(rawio)
             f.f = f
@@ -2166,11 +2157,7 @@ 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)
-        # 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)
+        with warnings_helper.check_warnings(('', ResourceWarning)):
             rawio = self.FileIO(os_helper.TESTFN, "w+b")
             f = self.tp(rawio)
             f.write(b"123xxx")
@@ -4092,8 +4079,7 @@ 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.catch_warnings():
-            warnings.simplefilter("ignore", ResourceWarning)
+        with warnings_helper.check_warnings(('', 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.assertIn(n2, (0, 1))
+        self.assertEqual(n2, 0)
 
     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 && ref != Py_None) {
+        if (ref != NULL) {
             PyObject *current = NULL;
             int rc = PyWeakref_GetRef(ref, &current);
             /* We only need "current" for pointer comparison. */

Modules/gc_weakref.txt

@@ -1,16 +1,6 @@
 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

@@ -901,6 +901,30 @@ handle_weakref_callbacks(PyGC_Head *unreachable, PyGC_Head *old)
         for (PyWeakReference *wr = *wrlist; wr != NULL; wr = next_wr) {
             next_wr = wr->wr_next;
 
+            // Since Python 2.3, weakrefs to cyclic garbage have been cleared
+            // *before* calling finalizers.  However, since tp_subclasses
+            // started being necessary to invalidate caches (e.g. by
+            // PyType_Modified()), that clearing has created a bug.  If the
+            // weakref to the subclass is cleared before a finalizer 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 can
+            // introduce other problems since some finalizer code expects that
+            // the weakrefs will be cleared first.  The "multiprocessing"
+            // package contains finalizer logic like this, for example.  So,
+            // we have the PyType_Check() test above and only defer the clear
+            // of 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 (!PyType_Check(wr->wr_object)) {
+                // _PyWeakref_ClearRef clears the weakref but leaves the
+                // callback pointer intact.  Obscure: it also changes *wrlist.
+                _PyObject_ASSERT((PyObject *)wr, wr->wr_object == op);
+                _PyWeakref_ClearRef(wr);
+                _PyObject_ASSERT((PyObject *)wr, wr->wr_object == Py_None);
+            }
+
             if (wr->wr_callback == NULL) {
                 /* no callback */
                 continue;

Python/gc_free_threading.c

@@ -1513,6 +1513,30 @@ find_weakref_callbacks(struct collection_state *state)
         for (PyWeakReference *wr = *wrlist; wr != NULL; wr = next_wr) {
             next_wr = wr->wr_next;
 
+            // Since Python 2.3, weakrefs to cyclic garbage have been cleared
+            // *before* calling finalizers.  However, since tp_subclasses
+            // started being necessary to invalidate caches (e.g. by
+            // PyType_Modified()), that clearing has created a bug.  If the
+            // weakref to the subclass is cleared before a finalizer 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 can
+            // introduce other problems since some finalizer code expects that
+            // the weakrefs will be cleared first.  The "multiprocessing"
+            // package contains finalizer logic like this, for example.  So,
+            // we have the PyType_Check() test above and only defer the clear
+            // of 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 (!PyType_Check(wr->wr_object)) {
+                // _PyWeakref_ClearRef clears the weakref but leaves the
+                // callback pointer intact.  Obscure: it also changes *wrlist.
+                _PyObject_ASSERT((PyObject *)wr, wr->wr_object == op);
+                _PyWeakref_ClearRef(wr);
+                _PyObject_ASSERT((PyObject *)wr, wr->wr_object == Py_None);
+            }
+
             // We do not invoke callbacks for weakrefs that are themselves
             // unreachable. This is partly for historical reasons: weakrefs
             // predate safe object finalization, and a weakref that is itself