From d7e458c987ea876b655ec1fe2e3ed4b19a802872 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 14 Jan 2025 20:32:09 +0000 Subject: [PATCH 01/12] gh-128844: Make `_Py_TryIncref` public as an unstable API. This exposes `_Py_TryIncref` as `PyUnstable_TryIncref()` and the helper function `_PyObject_SetMaybeWeakref` as `PyUnstable_EnableTryIncRef`. These are helpers for dealing with unowned references in a safe way, particularly in the free threading build. --- Doc/c-api/object.rst | 34 +++++++++++++ Include/cpython/object.h | 6 +++ ...-01-16-21-56-49.gh-issue-128844.ZPiJuo.rst | 3 ++ Modules/_testcapi/object.c | 51 +++++++++++++++++++ Objects/object.c | 14 +++++ 5 files changed, 108 insertions(+) create mode 100644 Misc/NEWS.d/next/C_API/2025-01-16-21-56-49.gh-issue-128844.ZPiJuo.rst diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index 3a434a4173eafa..d1291d30861e9a 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -613,3 +613,37 @@ Object Protocol .. versionadded:: 3.14 +.. c:function:: int PyUnstable_TryIncRef(PyObject *obj) + + Increments the reference count of *obj* if it is not zero. Returns ``1`` + if the object's reference count was successfully incremented. Otherwise, + this function returns ``0``. + + :c:function:`PyUnstable_EnableTryIncRef` must have been called + earlier on *obj* or this function may spuriously return ``0`` in the + :term:`free threading` build. + + This function is logically equivalent to the following C code, except that + it behaves atomically in the :term:`free threading` build:: + + if (Py_REFCNT(op) > 0) { + Py_INCREF(op); + return 1; + } + return 0; + + This is intended as a building block for safely dealing with unowned + references without the overhead of creating a :c:type:`!PyWeakReference`. + + .. note:: + + **obj** must not be freed. + + .. versionadded:: 3.14 + +.. c:function:: int PyUnstable_EnableTryIncRef(PyObject *obj) + + Enables subsequent uses of :c:func:`PyUnstable_TryIncRef` on *obj*. The + caller must hold a :term:`strong reference` to *obj* when calling this. + + .. versionadded:: 3.14 diff --git a/Include/cpython/object.h b/Include/cpython/object.h index ba31e2464abf84..0b0b6ada6d1586 100644 --- a/Include/cpython/object.h +++ b/Include/cpython/object.h @@ -541,3 +541,9 @@ PyAPI_FUNC(PyRefTracer) PyRefTracer_GetTracer(void**); * 0 if the runtime ignored it. This function cannot fail. */ PyAPI_FUNC(int) PyUnstable_Object_EnableDeferredRefcount(PyObject *); + +// Increments the reference count of the object, if it's not zero. +// PyUnstable_EnableTryIncRef() should be called on the object +// before calling this function in order to avoid spurious failures. +PyAPI_FUNC(int) PyUnstable_TryIncRef(PyObject *); +PyAPI_FUNC(void) PyUnstable_EnableTryIncRef(PyObject *); diff --git a/Misc/NEWS.d/next/C_API/2025-01-16-21-56-49.gh-issue-128844.ZPiJuo.rst b/Misc/NEWS.d/next/C_API/2025-01-16-21-56-49.gh-issue-128844.ZPiJuo.rst new file mode 100644 index 00000000000000..d9e1962631026a --- /dev/null +++ b/Misc/NEWS.d/next/C_API/2025-01-16-21-56-49.gh-issue-128844.ZPiJuo.rst @@ -0,0 +1,3 @@ +Add :c:func:`PyUnstable_TryIncRef` and :c:func:`PyUnstable_EnableTryIncRef` +unstable APIs. These are helpers for dealing with unowned references in +a thread-safe way, particularly in the free threading build. diff --git a/Modules/_testcapi/object.c b/Modules/_testcapi/object.c index 841410c52b3ce2..f5c7d66c5dae4d 100644 --- a/Modules/_testcapi/object.c +++ b/Modules/_testcapi/object.c @@ -131,6 +131,56 @@ pyobject_enable_deferred_refcount(PyObject *self, PyObject *obj) return PyLong_FromLong(result); } +static int MyObject_dealloc_called = 0; + +static void +MyObject_dealloc(PyObject *op) +{ + // PyUnstable_TryIncRef should return 0 if object is being deallocated + assert(Py_REFCNT(op) == 0); + assert(!PyUnstable_TryIncRef(op)); + assert(Py_REFCNT(op) == 0); + + MyObject_dealloc_called++; + Py_TYPE(op)->tp_free(op); +} + +static PyTypeObject MyType = { + PyVarObject_HEAD_INIT(NULL, 0) + .tp_name = "MyType", + .tp_basicsize = sizeof(PyObject), + .tp_dealloc = MyObject_dealloc, +}; + +static PyObject * +test_py_try_inc_ref(PyObject *self, PyObject *unused) +{ + if (PyType_Ready(&MyType) < 0) { + return NULL; + } + + PyObject *op = PyObject_New(PyObject, &MyType); + if (op == NULL) { + return NULL; + } + + PyUnstable_EnableTryIncRef(op); +#ifdef Py_GIL_DISABLED + // PyUnstable_EnableTryIncRef sets the shared flags to *at least* + assert((op->ob_ref_shared & _Py_REF_SHARED_FLAG_MASK) >= _Py_REF_MAYBE_WEAKREF); +#endif + + if (!PyUnstable_TryIncRef(op)) { + PyErr_SetString(PyExc_AssertionError, "PyUnstable_TryIncRef failed"); + Py_DECREF(op); + return NULL; + } + Py_DECREF(op); // undo try-incref + Py_DECREF(op); // dealloc + assert(MyObject_dealloc_called == 1); + Py_RETURN_NONE; +} + static PyMethodDef test_methods[] = { {"call_pyobject_print", call_pyobject_print, METH_VARARGS}, {"pyobject_print_null", pyobject_print_null, METH_VARARGS}, @@ -138,6 +188,7 @@ static PyMethodDef test_methods[] = { {"pyobject_print_os_error", pyobject_print_os_error, METH_VARARGS}, {"pyobject_clear_weakrefs_no_callbacks", pyobject_clear_weakrefs_no_callbacks, METH_O}, {"pyobject_enable_deferred_refcount", pyobject_enable_deferred_refcount, METH_O}, + {"test_py_try_inc_ref", test_py_try_inc_ref, METH_NOARGS}, {NULL}, }; diff --git a/Objects/object.c b/Objects/object.c index 9befd92e3231c8..16d2ffe266674f 100644 --- a/Objects/object.c +++ b/Objects/object.c @@ -2590,6 +2590,20 @@ PyUnstable_Object_EnableDeferredRefcount(PyObject *op) #endif } +int +PyUnstable_TryIncRef(PyObject *op) +{ + return _Py_TryIncref(op); +} + +void +PyUnstable_EnableTryIncRef(PyObject *op) +{ +#ifdef Py_GIL_DISABLED + _PyObject_SetMaybeWeakref(op); +#endif +} + void _Py_ResurrectReference(PyObject *op) { From 12ea2123b4cfaa2272ed1fff6fb91c93dfcffc9e Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 16 Jan 2025 22:22:04 +0000 Subject: [PATCH 02/12] Fix doc --- Doc/c-api/object.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index d1291d30861e9a..30cdac8e94d0de 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -619,7 +619,7 @@ Object Protocol if the object's reference count was successfully incremented. Otherwise, this function returns ``0``. - :c:function:`PyUnstable_EnableTryIncRef` must have been called + :c:func:`PyUnstable_EnableTryIncRef` must have been called earlier on *obj* or this function may spuriously return ``0`` in the :term:`free threading` build. From 2e18b455f4da0b2bd4aabe23e88e0a22bc8aa848 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 16 Jan 2025 22:37:44 +0000 Subject: [PATCH 03/12] Ignore global variables in test code --- Tools/c-analyzer/cpython/ignored.tsv | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Tools/c-analyzer/cpython/ignored.tsv b/Tools/c-analyzer/cpython/ignored.tsv index 213165b06866b5..503d9c96a52b65 100644 --- a/Tools/c-analyzer/cpython/ignored.tsv +++ b/Tools/c-analyzer/cpython/ignored.tsv @@ -446,6 +446,8 @@ Modules/_testcapi/exceptions.c - PyRecursingInfinitelyError_Type - Modules/_testcapi/heaptype.c - _testcapimodule - Modules/_testcapi/mem.c - FmData - Modules/_testcapi/mem.c - FmHook - +Modules/_testcapi/object.c - MyObject_dealloc_called - +Modules/_testcapi/object.c - MyType - Modules/_testcapi/structmember.c - test_structmembersType_OldAPI - Modules/_testcapi/watchers.c - g_dict_watch_events - Modules/_testcapi/watchers.c - g_dict_watchers_installed - From cba2d541c3807a8855393d13a78271ffc0e84461 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Thu, 16 Jan 2025 22:38:37 +0000 Subject: [PATCH 04/12] Remove incomplete sentence from docs --- Doc/c-api/object.rst | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index 30cdac8e94d0de..c1a3cba4ab1ec2 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -635,10 +635,6 @@ Object Protocol This is intended as a building block for safely dealing with unowned references without the overhead of creating a :c:type:`!PyWeakReference`. - .. note:: - - **obj** must not be freed. - .. versionadded:: 3.14 .. c:function:: int PyUnstable_EnableTryIncRef(PyObject *obj) From 31d054840b3cab85d324597b2f04d4e5e9478583 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 17 Jan 2025 13:01:23 -0500 Subject: [PATCH 05/12] Fix return type in docs --- Doc/c-api/object.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index c1a3cba4ab1ec2..2e6fa3a64057ba 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -637,7 +637,7 @@ Object Protocol .. versionadded:: 3.14 -.. c:function:: int PyUnstable_EnableTryIncRef(PyObject *obj) +.. c:function:: void PyUnstable_EnableTryIncRef(PyObject *obj) Enables subsequent uses of :c:func:`PyUnstable_TryIncRef` on *obj*. The caller must hold a :term:`strong reference` to *obj* when calling this. From 9ec47b30d9ab0ad23bdf49fd4ec30ffe3bc9382e Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 17 Jan 2025 13:02:18 -0500 Subject: [PATCH 06/12] Reset MyObject_dealloc_called at start of test_py_try_inc_ref --- Modules/_testcapi/object.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Modules/_testcapi/object.c b/Modules/_testcapi/object.c index f5c7d66c5dae4d..42789ba74dfa0a 100644 --- a/Modules/_testcapi/object.c +++ b/Modules/_testcapi/object.c @@ -159,6 +159,8 @@ test_py_try_inc_ref(PyObject *self, PyObject *unused) return NULL; } + MyObject_dealloc_called = 0; + PyObject *op = PyObject_New(PyObject, &MyType); if (op == NULL) { return NULL; From 8e53a2e5c3eb4b152a963dadb5f903e62bca6b56 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 17 Jan 2025 19:59:48 +0000 Subject: [PATCH 07/12] Fixup comment --- Modules/_testcapi/object.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Modules/_testcapi/object.c b/Modules/_testcapi/object.c index 42789ba74dfa0a..409b0c83c18cfd 100644 --- a/Modules/_testcapi/object.c +++ b/Modules/_testcapi/object.c @@ -168,7 +168,9 @@ test_py_try_inc_ref(PyObject *self, PyObject *unused) PyUnstable_EnableTryIncRef(op); #ifdef Py_GIL_DISABLED - // PyUnstable_EnableTryIncRef sets the shared flags to *at least* + // PyUnstable_EnableTryIncRef sets the shared flags to + // `_Py_REF_MAYBE_WEAKREF` if the flags are currently zero to ensure that + // the shared reference count is merged on deallocation. assert((op->ob_ref_shared & _Py_REF_SHARED_FLAG_MASK) >= _Py_REF_MAYBE_WEAKREF); #endif From 37a08dc24bb81c2fa94855dde102b210a3bdf1d3 Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Fri, 17 Jan 2025 20:10:14 +0000 Subject: [PATCH 08/12] s/unowned/borrowed --- Doc/c-api/object.rst | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index 2e6fa3a64057ba..b0a017d4b2365e 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -632,8 +632,9 @@ Object Protocol } return 0; - This is intended as a building block for safely dealing with unowned - references without the overhead of creating a :c:type:`!PyWeakReference`. + This is intended as a building block for safely dealing with + :term:`borrowed references ` without the overhead of + creating a :c:type:`!PyWeakReference`. .. versionadded:: 3.14 From 405952c2653d63260000dcfa62a72d912e6f0b78 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Mon, 20 Jan 2025 10:06:21 -0500 Subject: [PATCH 09/12] Add an example sketch from Petr Viktorin --- Doc/c-api/object.rst | 42 +++++++++++++++++++++++++++++++++++++++--- 1 file changed, 39 insertions(+), 3 deletions(-) diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index b0a017d4b2365e..58d00d551837cb 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -632,9 +632,45 @@ Object Protocol } return 0; - This is intended as a building block for safely dealing with - :term:`borrowed references ` without the overhead of - creating a :c:type:`!PyWeakReference`. + This is intended as a building block for managing weak references + without the overhead of a Python :c:type:`!PyWeakReference`. + Typically, correct use of this function requires support from *obj*'s + deallocator (:c:member:`~PyTypeObject.tp_dealloc`). + For example, the following sketch could be adapted to implement a + "weakmap" that works like a :py:class:`~weakref.WeakValueDictionary` + for a specific type: + + .. code-block:: c + + PyObject * + get_value(weakmap_key_type *key) + { + weakmap_type weakmap = ...; + weakmap_lock_mutex(weakmap); + PyObject *result = weakmap_find(weakmap, key); + if (PyUnstable_TryIncRef(result)) { + // `result` is safe to use + weakmap_unlock_mutex(weakmap); + return result; + } + // if we get here, `result` is starting to be garbage-collected, + // but has not been removed from the weakmap yet + weakmap_unlock_mutex(weakmap->mutex); + return NULL; + } + + // tp_dealloc function for weakmap values + void + value_dealloc(PyObject *value) + { + weakmap_type weakmap = ...; + weakmap_lock_mutex(weakmap); + weakmap_remove_value(weakmap, value); + + ... + weakmap_unlock_mutex(weakmap); + } + .. versionadded:: 3.14 From cac2eb44aeaed7ade8602dcd626db8491f05db24 Mon Sep 17 00:00:00 2001 From: Petr Viktorin Date: Tue, 21 Jan 2025 12:01:50 +0100 Subject: [PATCH 10/12] Add PyUnstable_EnableTryIncRef to the example --- Doc/c-api/object.rst | 22 ++++++++++++++++------ 1 file changed, 16 insertions(+), 6 deletions(-) diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index 58d00d551837cb..2ac239f2634d26 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -642,20 +642,31 @@ Object Protocol .. code-block:: c + PyMutex mutex; + + PyObject * + add_entry(weakmap_key_type *key, PyObject* value) + { + PyUnstable_EnableTryIncRef(value); + weakmap_type weakmap = ...; + weakmap_add_entry(weakmap, key, value); + Py_RETURN_NONE; + } + PyObject * get_value(weakmap_key_type *key) { weakmap_type weakmap = ...; - weakmap_lock_mutex(weakmap); + PyMutex_Lock(mutex); PyObject *result = weakmap_find(weakmap, key); if (PyUnstable_TryIncRef(result)) { // `result` is safe to use - weakmap_unlock_mutex(weakmap); + PyMutex_Unlock(mutex); return result; } // if we get here, `result` is starting to be garbage-collected, // but has not been removed from the weakmap yet - weakmap_unlock_mutex(weakmap->mutex); + PyMutex_Unlock(mutex); return NULL; } @@ -664,14 +675,13 @@ Object Protocol value_dealloc(PyObject *value) { weakmap_type weakmap = ...; - weakmap_lock_mutex(weakmap); + PyMutex_Lock(mutex); weakmap_remove_value(weakmap, value); ... - weakmap_unlock_mutex(weakmap); + PyMutex_Unlock(mutex); } - .. versionadded:: 3.14 .. c:function:: void PyUnstable_EnableTryIncRef(PyObject *obj) From 887ab77322e40005d212e3b321662eb1e8d697aa Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 21 Jan 2025 17:22:39 +0000 Subject: [PATCH 11/12] Add locking to add_entry --- Doc/c-api/object.rst | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index 2ac239f2634d26..9970171fd20402 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -645,11 +645,13 @@ Object Protocol PyMutex mutex; PyObject * - add_entry(weakmap_key_type *key, PyObject* value) + add_entry(weakmap_key_type *key, PyObject *value) { PyUnstable_EnableTryIncRef(value); weakmap_type weakmap = ...; + PyMutex_Lock(&mutex); weakmap_add_entry(weakmap, key, value); + PyMutex_Unlock(&mutex); Py_RETURN_NONE; } @@ -657,16 +659,16 @@ Object Protocol get_value(weakmap_key_type *key) { weakmap_type weakmap = ...; - PyMutex_Lock(mutex); + PyMutex_Lock(&mutex); PyObject *result = weakmap_find(weakmap, key); if (PyUnstable_TryIncRef(result)) { // `result` is safe to use - PyMutex_Unlock(mutex); + PyMutex_Unlock(&mutex); return result; } // if we get here, `result` is starting to be garbage-collected, // but has not been removed from the weakmap yet - PyMutex_Unlock(mutex); + PyMutex_Unlock(&mutex); return NULL; } @@ -675,11 +677,11 @@ Object Protocol value_dealloc(PyObject *value) { weakmap_type weakmap = ...; - PyMutex_Lock(mutex); + PyMutex_Lock(&mutex); weakmap_remove_value(weakmap, value); ... - PyMutex_Unlock(mutex); + PyMutex_Unlock(&mutex); } .. versionadded:: 3.14 From 4a87acbd57b5d677174ca4a709184f7852673b6d Mon Sep 17 00:00:00 2001 From: Sam Gross Date: Tue, 28 Jan 2025 09:44:29 -0500 Subject: [PATCH 12/12] Update Doc/c-api/object.rst Co-authored-by: Petr Viktorin --- Doc/c-api/object.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Doc/c-api/object.rst b/Doc/c-api/object.rst index 3b63b1eb7bba47..1ba5942c63601d 100644 --- a/Doc/c-api/object.rst +++ b/Doc/c-api/object.rst @@ -645,7 +645,8 @@ Object Protocol return 0; This is intended as a building block for managing weak references - without the overhead of a Python :c:type:`!PyWeakReference`. + without the overhead of a Python :ref:`weak reference object `. + Typically, correct use of this function requires support from *obj*'s deallocator (:c:member:`~PyTypeObject.tp_dealloc`). For example, the following sketch could be adapted to implement a