Skip to content

gh-127582: Make object resurrection thread-safe for free threading. #127612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 44 additions & 0 deletions Include/internal/pycore_object.h
Original file line number Diff line number Diff line change
Expand Up @@ -697,8 +697,52 @@ _PyObject_SetMaybeWeakref(PyObject *op)
}
}

extern int _PyObject_ResurrectEndSlow(PyObject *op);
#endif

// Temporarily resurrects an object during deallocation. The refcount is set
// to one.
static inline void
_PyObject_ResurrectStart(PyObject *op)
{
assert(Py_REFCNT(op) == 0);
#ifdef Py_REF_DEBUG
_Py_IncRefTotal(_PyThreadState_GET());
#endif
#ifdef Py_GIL_DISABLED
_Py_atomic_store_uintptr_relaxed(&op->ob_tid, _Py_ThreadId());
_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, 1);
_Py_atomic_store_ssize_relaxed(&op->ob_ref_shared, 0);
#else
Py_SET_REFCNT(op, 1);
#endif
}

// Undoes an object resurrection by decrementing the refcount without calling
// _Py_Dealloc(). Returns 0 if the object is dead (the normal case), and
// deallocation should continue. Returns 1 if the object is still alive.
static inline int
_PyObject_ResurrectEnd(PyObject *op)
{
#ifdef Py_REF_DEBUG
_Py_DecRefTotal(_PyThreadState_GET());
#endif
#ifndef Py_GIL_DISABLED
Py_SET_REFCNT(op, Py_REFCNT(op) - 1);
return Py_REFCNT(op) != 0;
#else
uint32_t local = _Py_atomic_load_uint32_relaxed(&op->ob_ref_local);
Py_ssize_t shared = _Py_atomic_load_ssize_acquire(&op->ob_ref_shared);
if (_Py_IsOwnedByCurrentThread(op) && local == 1 && shared == 0) {
// Fast-path: object has a single refcount and is owned by this thread
_Py_atomic_store_uint32_relaxed(&op->ob_ref_local, 0);
return 0;
}
// Slow-path: object has a shared refcount or is not owned by this thread
return _PyObject_ResurrectEndSlow(op);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I presume the slow path isn't inline so that compiler will inline the fast path and code will be smaller?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that exact split of what's in the "fast-path" vs. "slow-path" doesn't matter too much here, but it would be unwieldy to try to put everything in the inline function. In particular, _Py_DecRefSharedIsDead is big and not exposed outside of object.c.

#endif
}

/* Tries to incref op and returns 1 if successful or 0 otherwise. */
static inline int
_Py_TryIncref(PyObject *op)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Fix non-thread-safe object resurrection when calling finalizers and watcher
callbacks in the free threading build.
7 changes: 2 additions & 5 deletions Objects/codeobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1867,14 +1867,11 @@ free_monitoring_data(_PyCoMonitoringData *data)
static void
code_dealloc(PyCodeObject *co)
{
assert(Py_REFCNT(co) == 0);
Py_SET_REFCNT(co, 1);
_PyObject_ResurrectStart((PyObject *)co);
notify_code_watchers(PY_CODE_EVENT_DESTROY, co);
if (Py_REFCNT(co) > 1) {
Py_SET_REFCNT(co, Py_REFCNT(co) - 1);
if (_PyObject_ResurrectEnd((PyObject *)co)) {
return;
}
Py_SET_REFCNT(co, 0);

#ifdef Py_GIL_DISABLED
PyObject_GC_UnTrack(co);
Expand Down
7 changes: 2 additions & 5 deletions Objects/dictobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -3162,14 +3162,11 @@ dict_dealloc(PyObject *self)
{
PyDictObject *mp = (PyDictObject *)self;
PyInterpreterState *interp = _PyInterpreterState_GET();
assert(Py_REFCNT(mp) == 0);
Py_SET_REFCNT(mp, 1);
_PyObject_ResurrectStart(self);
_PyDict_NotifyEvent(interp, PyDict_EVENT_DEALLOCATED, mp, NULL, NULL);
if (Py_REFCNT(mp) > 1) {
Py_SET_REFCNT(mp, Py_REFCNT(mp) - 1);
if (_PyObject_ResurrectEnd(self)) {
return;
}
Py_SET_REFCNT(mp, 0);
PyDictValues *values = mp->ma_values;
PyDictKeysObject *keys = mp->ma_keys;
Py_ssize_t i, n;
Expand Down
7 changes: 2 additions & 5 deletions Objects/funcobject.c
Original file line number Diff line number Diff line change
Expand Up @@ -1092,14 +1092,11 @@ static void
func_dealloc(PyObject *self)
{
PyFunctionObject *op = _PyFunction_CAST(self);
assert(Py_REFCNT(op) == 0);
Py_SET_REFCNT(op, 1);
_PyObject_ResurrectStart(self);
handle_func_event(PyFunction_EVENT_DESTROY, op, NULL);
if (Py_REFCNT(op) > 1) {
Py_SET_REFCNT(op, Py_REFCNT(op) - 1);
if (_PyObject_ResurrectEnd(self)) {
return;
}
Py_SET_REFCNT(op, 0);
_PyObject_GC_UNTRACK(op);
if (op->func_weakreflist != NULL) {
PyObject_ClearWeakRefs((PyObject *) op);
Expand Down
40 changes: 35 additions & 5 deletions Objects/object.c
Original file line number Diff line number Diff line change
Expand Up @@ -362,8 +362,10 @@ is_dead(PyObject *o)
}
# endif

void
_Py_DecRefSharedDebug(PyObject *o, const char *filename, int lineno)
// Decrement the shared reference count of an object. Return 1 if the object
// is dead and should be deallocated, 0 otherwise.
static int
_Py_DecRefSharedIsDead(PyObject *o, const char *filename, int lineno)
{
// Should we queue the object for the owning thread to merge?
int should_queue;
Expand Down Expand Up @@ -404,6 +406,15 @@ _Py_DecRefSharedDebug(PyObject *o, const char *filename, int lineno)
}
else if (new_shared == _Py_REF_MERGED) {
// refcount is zero AND merged
return 1;
}
return 0;
}

void
_Py_DecRefSharedDebug(PyObject *o, const char *filename, int lineno)
{
if (_Py_DecRefSharedIsDead(o, filename, lineno)) {
_Py_Dealloc(o);
}
}
Expand Down Expand Up @@ -472,6 +483,26 @@ _Py_ExplicitMergeRefcount(PyObject *op, Py_ssize_t extra)
&shared, new_shared));
return refcnt;
}

// The more complicated "slow" path for undoing the resurrection of an object.
int
_PyObject_ResurrectEndSlow(PyObject *op)
{
if (_Py_IsImmortal(op)) {
return 1;
}
if (_Py_IsOwnedByCurrentThread(op)) {
// If the object is owned by the current thread, give up ownership and
// merge the refcount. This isn't necessary in all cases, but it
// simplifies the implementation.
Py_ssize_t refcount = _Py_ExplicitMergeRefcount(op, -1);
return refcount != 0;
}
int is_dead = _Py_DecRefSharedIsDead(op, NULL, 0);
return !is_dead;
}


#endif /* Py_GIL_DISABLED */


Expand Down Expand Up @@ -550,7 +581,7 @@ PyObject_CallFinalizerFromDealloc(PyObject *self)
}

/* Temporarily resurrect the object. */
Py_SET_REFCNT(self, 1);
_PyObject_ResurrectStart(self);

PyObject_CallFinalizer(self);

Expand All @@ -560,8 +591,7 @@ PyObject_CallFinalizerFromDealloc(PyObject *self)

/* Undo the temporary resurrection; can't use DECREF here, it would
* cause a recursive call. */
Py_SET_REFCNT(self, Py_REFCNT(self) - 1);
if (Py_REFCNT(self) == 0) {
if (!_PyObject_ResurrectEnd(self)) {
return 0; /* this is the normal path out */
}

Expand Down
Loading