Ensure weakrefs with callbacks are cleared early.

We need to clear those before executing the callback.  Since this
ensures they can't run a second time, we don't need
_PyGC_SET_FINALIZED().  Revise comments.

Author: nascheme

Python/gc.c

@@ -901,23 +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
+            // Weakrefs with callbacks always need to be cleared before
+            // executing the callback.  Sometimes the callback will call
+            // the ref object, to check if it's actually a dead reference
+            // (KeyedRef does this, for example).  We want to indicate that it
+            // is dead, even though it is possible a finalizer might resurrect
+            // it.  Clearing also prevents the callback from being executing
+            // more than once.
+            //
+            // Since Python 2.3, all 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)) {
+            // 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)) {
                 // _PyWeakref_ClearRef clears the weakref but leaves the
                 // callback pointer intact.  Obscure: it also changes *wrlist.
                 _PyObject_ASSERT((PyObject *)wr, wr->wr_object == op);
@@ -962,11 +969,6 @@ handle_weakref_callbacks(PyGC_Head *unreachable, PyGC_Head *old)
                 continue;
             }
 
-            if (_PyGC_FINALIZED((PyObject *)wr)) {
-                /* Callback was already run (weakref must have been resurrected). */
-                continue;
-            }
-
             /* Create a new reference so that wr can't go away
              * before we can process it again.
              */
@@ -995,10 +997,6 @@ handle_weakref_callbacks(PyGC_Head *unreachable, PyGC_Head *old)
         callback = wr->wr_callback;
         _PyObject_ASSERT(op, callback != NULL);
 
-        /* Ensure we don't execute the callback again if the weakref is
-         * resurrected. */
-        _PyGC_SET_FINALIZED(op);
-
         /* copy-paste of weakrefobject.c's handle_callback() */
         temp = PyObject_CallOneArg(callback, (PyObject *)wr);
         if (temp == NULL) {

Python/gc_free_threading.c

@@ -1513,23 +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
+            // Weakrefs with callbacks always need to be cleared before
+            // executing the callback.  Sometimes the callback will call
+            // the ref object, to check if it's actually a dead reference
+            // (KeyedRef does this, for example).  We want to indicate that it
+            // is dead, even though it is possible a finalizer might resurrect
+            // it.  Clearing also prevents the callback from being executing
+            // more than once.
+            //
+            // Since Python 2.3, all 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)) {
+            // 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)) {
                 // _PyWeakref_ClearRef clears the weakref but leaves the
                 // callback pointer intact.  Obscure: it also changes *wrlist.
                 _PyObject_ASSERT((PyObject *)wr, wr->wr_object == op);
@@ -1546,11 +1553,6 @@ find_weakref_callbacks(struct collection_state *state)
                 continue;
             }
 
-            if (_PyGC_FINALIZED((PyObject *)wr)) {
-                // Callback was already run (weakref must have been resurrected).
-                continue;
-            }
-
             // Create a new reference so that wr can't go away before we can
             // process it again.
             merge_refcount((PyObject *)wr, 1);
@@ -1610,10 +1612,6 @@ call_weakref_callbacks(struct collection_state *state)
         PyObject *callback = wr->wr_callback;
         _PyObject_ASSERT(op, callback != NULL);
 
-        /* Ensure we don't execute the callback again if the weakref is
-         * resurrected. */
-        _PyGC_SET_FINALIZED(op);
-
         /* copy-paste of weakrefobject.c's handle_callback() */
         PyObject *temp = PyObject_CallOneArg(callback, (PyObject *)wr);
         if (temp == NULL) {