-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
Description
Feature or enhancement
We should make _Py_TryIncref public as function with the following signature:
int PyUnstable_TryIncref(PyObject *op);EDIT: Renamed to PyUnstable_TryIncref in accordance with Victor's suggestion.
The function increments the reference count if it's not zero in a thread-safe way. It's logically equivalent to the following snippet and in the default (GIL-enabled) build it's implemented as such:
if (Py_REFCNT(op) > 0) {
Py_INCREF(op);
return 1;
}
return 0;Additionally, we should make _PyObject_SetMaybeWeakref public as PyUnstable_Object_EnableTryIncRef. This function has no equivalent in the GIL-enabled build (it's a no-op), but it's important for making TryIncref work reliably with our biased reference counting implementation.
Motivation
The TryIncref primitive is a building block for handling borrowed and unowned references. It addresses an issue that generally cannot be solved by adding extra synchronization like mutexes because it handles the race between the reference count reaching zero (which is outside developers' control) and the TryIncref.
We use it internally in three subsystems:
- To implement weak references
- In asyncio to access the borrowed/unowned list of tasks
- In the MRO cache, to safely access the borrowed/unowned cached
PyObject *entries.
Recently, we discovered a thread safety bug in pybind11 related to the use of borrowed/unowned references. Using _Py_TryIncref in place of Py_INCREF would fix the bug. I think nanobind probably has a similar issue.
Alternatives
- Use actual weak reference objects instead of borrowed/unowned references. This is cleaner, but is not practical for performance reasons in the above use cases. Using
PyWeakRefobjects increases the overhead of pybind11 bindings by 30% in some simple tests. - Implement something like
_Py_TryIncrefin extensions. I think this is much worse than making the function public as an unstable API because it requires direct access to the reference count fields -- the implementation is tied to the implementation of biased reference counting -- and I'd like to avoid extensions depending directly on those details.
See also
- Add a public, fail-able
Py_INCREFfor the free-threaded build #113920 - [BUG]: Data race when using static variables with free-threaded Python pybind/pybind11#5489