Merge branch 'pythongh-111926-thread-safe-weakref' into nogil-integra…

…tion

Include/internal/pycore_interp.h

@@ -59,6 +59,12 @@ struct _stoptheworld_state {
     PyThreadState *requester; // Thread that requested the pause (may be NULL).
 };
 
+#ifdef Py_GIL_DISABLED
+// This should be prime but otherwise the choice is arbitrary. A larger value
+// increases concurrency at the expense of memory.
+#define NUM_WEAKREF_LIST_LOCKS 127
+#endif
+
 /* cross-interpreter data registry */
 
 /* Tracks some rare events per-interpreter, used by the optimizer to turn on/off
@@ -203,6 +209,7 @@ struct _is {
 #if defined(Py_GIL_DISABLED)
     struct _mimalloc_interp_state mimalloc;
     struct _brc_state brc;  // biased reference counting state
+    PyMutex weakref_locks[NUM_WEAKREF_LIST_LOCKS];
 #endif
 
     // Per-interpreter state for the obmalloc allocator.  For the main

Include/internal/pycore_lock.h

@@ -113,7 +113,7 @@ typedef enum _PyLockFlags {
 
 // Lock a mutex with an optional timeout and additional options. See
 // _PyLockFlags for details.
-extern PyLockStatus
+PyAPI_FUNC(PyLockStatus)
 _PyMutex_LockTimed(PyMutex *m, PyTime_t timeout_ns, _PyLockFlags flags);
 
 // Lock a mutex with aditional options. See _PyLockFlags for details.

Include/internal/pycore_object.h

@@ -88,7 +88,6 @@ PyAPI_FUNC(void) _Py_NO_RETURN _Py_FatalRefcountErrorFunc(
 PyAPI_DATA(Py_ssize_t) _Py_RefTotal;
 
 extern void _Py_AddRefTotal(PyInterpreterState *, Py_ssize_t);
-extern void _Py_IncRefTotal(PyInterpreterState *);
 extern void _Py_DecRefTotal(PyInterpreterState *);
 
 #  define _Py_DEC_REFTOTAL(interp) \
@@ -507,6 +506,25 @@ _Py_XNewRefWithLock(PyObject *obj)
     return _Py_NewRefWithLock(obj);
 }
 
+static inline void
+_PyObject_SetMaybeWeakref(PyObject *op)
+{
+    if (_Py_IsImmortal(op)) {
+        return;
+    }
+    for (;;) {
+        Py_ssize_t shared = _Py_atomic_load_ssize_relaxed(&op->ob_ref_shared);
+        if ((shared & _Py_REF_SHARED_FLAG_MASK) != 0) {
+            // Nothing to do if it's in WEAKREFS, QUEUED, or MERGED states.
+            return;
+        }
+        if (_Py_atomic_compare_exchange_ssize(
+                &op->ob_ref_shared, &shared, shared | _Py_REF_MAYBE_WEAKREF)) {
+            return;
+        }
+    }
+}
+
 #endif
 
 #ifdef Py_REF_DEBUG

Include/internal/pycore_pyatomic_ft_wrappers.h

@@ -33,6 +33,8 @@ extern "C" {
     _Py_atomic_load_ptr_acquire(&value)
 #define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) \
     _Py_atomic_load_uintptr_acquire(&value)
+#define FT_ATOMIC_STORE_PTR(value, new_value) \
+    _Py_atomic_store_ptr(&value, new_value)
 #define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) \
     _Py_atomic_store_ptr_relaxed(&value, new_value)
 #define FT_ATOMIC_STORE_PTR_RELEASE(value, new_value) \
@@ -51,6 +53,7 @@ extern "C" {
 #define FT_ATOMIC_LOAD_UINT8_RELAXED(value) value
 #define FT_ATOMIC_LOAD_PTR_ACQUIRE(value) value
 #define FT_ATOMIC_LOAD_UINTPTR_ACQUIRE(value) value
+#define FT_ATOMIC_STORE_PTR(value, new_value) value = new_value
 #define FT_ATOMIC_STORE_PTR_RELAXED(value, new_value) value = new_value
 #define FT_ATOMIC_STORE_PTR_RELEASE(value, new_value) value = new_value
 #define FT_ATOMIC_STORE_UINTPTR_RELEASE(value, new_value) value = new_value

Include/internal/pycore_weakref.h

@@ -9,8 +9,26 @@ extern "C" {
 #endif
 
 #include "pycore_critical_section.h" // Py_BEGIN_CRITICAL_SECTION()
+#include "pycore_lock.h"
 #include "pycore_object.h"           // _Py_REF_IS_MERGED()
 
+#ifdef Py_GIL_DISABLED
+
+#define WEAKREF_LIST_LOCK(obj) \
+    _PyInterpreterState_GET()  \
+        ->weakref_locks[((uintptr_t)obj) % NUM_WEAKREF_LIST_LOCKS]
+
+#define LOCK_WEAKREFS(obj) \
+    PyMutex_LockFlags(&WEAKREF_LIST_LOCK(obj), _Py_LOCK_DONT_DETACH)
+#define UNLOCK_WEAKREFS(obj) PyMutex_Unlock(&WEAKREF_LIST_LOCK(obj))
+
+#else
+
+#define LOCK_WEAKREFS(obj)
+#define UNLOCK_WEAKREFS(obj)
+
+#endif
+
 static inline int _is_dead(PyObject *obj)
 {
     // Explanation for the Py_REFCNT() check: when a weakref's target is part
@@ -30,53 +48,82 @@ static inline int _is_dead(PyObject *obj)
 static inline PyObject* _PyWeakref_GET_REF(PyObject *ref_obj)
 {
     assert(PyWeakref_Check(ref_obj));
-    PyObject *ret = NULL;
-    Py_BEGIN_CRITICAL_SECTION(ref_obj);
     PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj);
-    PyObject *obj = ref->wr_object;
 
+#if !defined(Py_GIL_DISABLED)
+    PyObject *obj = ref->wr_object;
     if (obj == Py_None) {
         // clear_weakref() was called
-        goto end;
+        return NULL;
     }
 
     if (_is_dead(obj)) {
-        goto end;
+        return NULL;
     }
-#if !defined(Py_GIL_DISABLED)
     assert(Py_REFCNT(obj) > 0);
+    return Py_NewRef(obj);
+#else
+    PyObject *obj = _Py_atomic_load_ptr(&ref->wr_object);
+    if (obj == Py_None) {
+        // clear_weakref() was called
+        return NULL;
+    }
+    LOCK_WEAKREFS(obj);
+    if (_Py_atomic_load_ptr(&ref->wr_object) == Py_None) {
+        // clear_weakref() was called
+        UNLOCK_WEAKREFS(obj);
+        return NULL;
+    }
+    if (_Py_TryIncref(&obj, obj)) {
+        UNLOCK_WEAKREFS(obj);
+        return obj;
+    }
+    UNLOCK_WEAKREFS(obj);
+    return NULL;
 #endif
-    ret = Py_NewRef(obj);
-end:
-    Py_END_CRITICAL_SECTION();
-    return ret;
 }
 
 static inline int _PyWeakref_IS_DEAD(PyObject *ref_obj)
 {
     assert(PyWeakref_Check(ref_obj));
     int ret = 0;
-    Py_BEGIN_CRITICAL_SECTION(ref_obj);
     PyWeakReference *ref = _Py_CAST(PyWeakReference*, ref_obj);
+#ifdef Py_GIL_DISABLED
+    PyObject *obj = _Py_atomic_load_ptr(&ref->wr_object);
+#else
     PyObject *obj = ref->wr_object;
+#endif
     if (obj == Py_None) {
         // clear_weakref() was called
         ret = 1;
     }
     else {
+        LOCK_WEAKREFS(obj);
         // See _PyWeakref_GET_REF() for the rationale of this test
+#ifdef Py_GIL_DISABLED
+        PyObject *obj_reloaded = _Py_atomic_load_ptr(&ref->wr_object);
+        ret = (obj_reloaded == Py_None) || _is_dead(obj);
+#else
         ret = _is_dead(obj);
+#endif
+        UNLOCK_WEAKREFS(obj);
     }
-    Py_END_CRITICAL_SECTION();
     return ret;
 }
 
+// NB: In free-threaded builds the weakref list lock for the referenced object
+// must be held around calls to this function.
 extern Py_ssize_t _PyWeakref_GetWeakrefCount(PyWeakReference *head);
 
+extern Py_ssize_t _PyWeakref_GetWeakrefCountThreadsafe(PyObject *obj);
+
+// Clear all the weak references to obj but leave their callbacks uncalled and
+// intact.
+extern void _PyWeakref_ClearWeakRefsExceptCallbacks(PyObject *obj);
+
 extern void _PyWeakref_ClearRef(PyWeakReference *self);
 
 #ifdef __cplusplus
 }
 #endif
 #endif /* !Py_INTERNAL_WEAKREF_H */
-

Include/object.h

@@ -771,6 +771,7 @@ PyAPI_FUNC(void) _Py_NegativeRefcount(const char *filename, int lineno,
                                       PyObject *op);
 PyAPI_FUNC(void) _Py_INCREF_IncRefTotal(void);
 PyAPI_FUNC(void) _Py_DECREF_DecRefTotal(void);
+PyAPI_FUNC(void) _Py_IncRefTotal(PyInterpreterState *);
 #endif  // Py_REF_DEBUG && !Py_LIMITED_API
 
 PyAPI_FUNC(void) _Py_Dealloc(PyObject *);

Lib/test/test_weakref.py

@@ -1865,6 +1865,25 @@ def test_threaded_weak_valued_consistency(self):
                 self.assertEqual(len(d), 1)
                 o = None  # lose ref
 
+    @support.cpython_only
+    def test_weak_valued_consistency(self):
+        # A single-threaded, deterministic repro for issue #28427: old keys
+        # should not remove new values from WeakValueDictionary. This relies on
+        # an implementation detail of CPython's WeakValueDictionary (its
+        # underlying dictionary of KeyedRefs) to reproduce the issue.
+        d = weakref.WeakValueDictionary()
+        with support.disable_gc():
+            d[10] = RefCycle()
+            # Keep the KeyedRef alive after it's replaced so that GC will invoke
+            # the callback.
+            wr = d.data[10]
+            # Replace the value with something that isn't cyclic garbage
+            o = RefCycle()
+            d[10] = o
+            # Trigger GC, which will invoke the callback for `wr`
+            gc.collect()
+            self.assertEqual(len(d), 1)
+
     def check_threaded_weak_dict_copy(self, type_, deepcopy):
         # `type_` should be either WeakKeyDictionary or WeakValueDictionary.
         # `deepcopy` should be either True or False.

Modules/_weakref.c

@@ -14,7 +14,6 @@ module _weakref
 #include "clinic/_weakref.c.h"
 
 /*[clinic input]
-@critical_section object
 _weakref.getweakrefcount -> Py_ssize_t
 
   object: object
@@ -25,14 +24,12 @@ Return the number of weak references to 'object'.
 
 static Py_ssize_t
 _weakref_getweakrefcount_impl(PyObject *module, PyObject *object)
-/*[clinic end generated code: output=301806d59558ff3e input=6535a580f1d0ebdc]*/
+/*[clinic end generated code: output=301806d59558ff3e input=7d4d04fcaccf64d5]*/
 {
     if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(object))) {
         return 0;
     }
-    PyWeakReference **list = GET_WEAKREFS_LISTPTR(object);
-    Py_ssize_t count = _PyWeakref_GetWeakrefCount(*list);
-    return count;
+    return _PyWeakref_GetWeakrefCountThreadsafe(object);
 }
 
 
@@ -77,7 +74,6 @@ _weakref__remove_dead_weakref_impl(PyObject *module, PyObject *dct,
 
 
 /*[clinic input]
-@critical_section object
 _weakref.getweakrefs
     object: object
     /
@@ -86,26 +82,57 @@ Return a list of all weak reference objects pointing to 'object'.
 [clinic start generated code]*/
 
 static PyObject *
-_weakref_getweakrefs_impl(PyObject *module, PyObject *object)
-/*[clinic end generated code: output=5ec268989fb8f035 input=3dea95b8f5b31bbb]*/
+_weakref_getweakrefs(PyObject *module, PyObject *object)
+/*[clinic end generated code: output=25c7731d8e011824 input=00c6d0e5d3206693]*/
 {
     if (!_PyType_SUPPORTS_WEAKREFS(Py_TYPE(object))) {
         return PyList_New(0);
     }
 
     PyWeakReference **list = GET_WEAKREFS_LISTPTR(object);
-    Py_ssize_t count = _PyWeakref_GetWeakrefCount(*list);
+    Py_ssize_t count = _PyWeakref_GetWeakrefCountThreadsafe(object);
 
     PyObject *result = PyList_New(count);
     if (result == NULL) {
         return NULL;
     }
 
+#ifdef Py_GIL_DISABLED
+    Py_ssize_t num_added = 0;
+    LOCK_WEAKREFS(object);
+    PyWeakReference *current = *list;
+    // Weakrefs may be added or removed since the count was computed.
+    while (num_added < count && current != NULL) {
+        if (_Py_TryIncref((PyObject**) &current, (PyObject *) current)) {
+            PyList_SET_ITEM(result, num_added, current);
+            num_added++;
+        }
+        current = current->wr_next;
+    }
+    UNLOCK_WEAKREFS(object);
+
+    // Don't return an incomplete list
+    if (num_added != count) {
+        PyObject *new_list = PyList_New(num_added);
+        if (new_list == NULL) {
+            Py_DECREF(result);
+            return NULL;
+        }
+        for (Py_ssize_t i = 0; i < num_added; i++) {
+            PyObject *obj = PyList_GET_ITEM(result, i);
+            PyList_SET_ITEM(new_list, i, obj);
+            PyList_SET_ITEM(result, i, NULL);
+        }
+        Py_DECREF(result);
+        result = new_list;
+    }
+#else
     PyWeakReference *current = *list;
     for (Py_ssize_t i = 0; i < count; ++i) {
         PyList_SET_ITEM(result, i, Py_NewRef(current));
         current = current->wr_next;
     }
+#endif
     return result;
 }