Make _Py_TryIncref public as an unstable API as PyUnstable_Object_TryIncref()

[colesbury]

Feature or enhancement

We should make _Py_TryIncref public as function with the following signature:

int PyUnstable_Object_TryIncref(PyObject *op);

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 PyWeakRef objects increases the overhead of pybind11 bindings by 30% in some simple tests.
• Implement something like _Py_TryIncref in 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_INCREF for the free-threaded build #113920
• [BUG]: Data race when using static variables with free-threaded Python pybind/pybind11#5489

[corona10]

cc @encukou @vstinner

[vstinner]

Since Py_INCREF() and Py_NewRef() omit "PyObject" in their name, I suggest PyUnstable_TryIncref() name for the new function.

Additionally, we should make _PyObject_SetMaybeWeakref public as PyUnstable_Object_EnableTryIncRef.

Would you mind to elaborate this function? What is its API? What is its usage?

[encukou]

One thing is not clear to me.
Once the refcount hits zero, the object can start getting deallocated, and the memory can be reused for something else, right? If that happens, and the memory that was the refcount now holds an unrelated nonzero integer, couldn't PyUnstable_Object_TryIncref increment that integer?

I suspect there's a mechanism to prevent that in free-threaded builds, but I'd like to know what it is and how it applies.

[colesbury]

Would you mind to elaborate this function? What is its API? What is its usage?

When called on an object, PyUnstable_Object_EnableTryIncRef allows subsequent calls to PyUnstable_TryIncref. Without it, calls to PyUnstable_TryIncref() may spuriously return 0 in the free threading build. The implementation is a no-op in the default build. Like most Python C API functions, you must hold a strong reference to the object when calling PyUnstable_Object_EnableTryIncRef, so it can't be folded into PyUnstable_TryIncref(), which can take a borrowed/unowned reference.

The API is a bit of a "wart", but I don't see a way to avoid it.

// Enable subsequent calls to `PyUnstable_TryIncref` on `op`
void PyUnstable_Object_EnableTryIncRef(PyObject *op);

Once the refcount hits zero, the object can start getting deallocated, and the memory can be reused for something else, right?

In the asyncio and pybind11 use cases, there are already locks during deallocation and around the _Py_TryIncref() call, so the object isn't deallocated yet. Avoiding locking (like in our dictionary implementation) is out of scope for now -- it would need some extra APIs to be exposed.

Here's an example in psuedo-code based on pybind11's use case. Note that when TryIncref() is called, any concurrent deallocation can't have gone past the lock acquisition in the first line of wrapper_dealloc. (Otherwise, the wrapper would have already been removed from registered_instances).

The TryIncref() is still needed (in the free threading build) so that we don't incref an object with zero refcount that is subsequently, unconditionally deallocated.

// map from C++ pointers to Python wrapper objects
std::unordered_map<void *, PyObject *> registered_instances;
std::mutex mutex;


// Find an existing Python wrapper for a C++ pointer
PyObject *
get_wrapper(void *ptr)
{
    std::unique_lock<std::mutex> lk(mutex);
    if (auto it = registered_instances.find(ptr); it != registered_instances.end()) {
        PyObject *wrapper = it->second;
        if (_Py_TryIncref(wrapper)) { // NOTE: `Py_INCREF()` here would not be safe in the free threading build!
            return wrapper;
        }
    }
}

void
wrapper_dealloc(PyObject *wrapper)
{
    std::unique_lock<std::mutex> lk(mutex);
    void *ptr = get_cpp_ptr_from_wrapper(wrapper);
    registered_instances.erase(ptr);

    ...
}

[vstinner]

Ok, I'm fine with adding these two PyUnstable functions.