Skip to content

gh-148953: Fix memory allocation deadlock in qsbr code under free-threading. - #149100

Closed
AnnaAr321 wants to merge 4 commits into
python:mainfrom
AnnaAr321:patch-1
Closed

gh-148953: Fix memory allocation deadlock in qsbr code under free-threading.#149100
AnnaAr321 wants to merge 4 commits into
python:mainfrom
AnnaAr321:patch-1

Conversation

@AnnaAr321

@AnnaAr321AnnaAr321 commented Apr 28, 2026

Copy link
Copy Markdown

Previously, a thread attempting to allocate a QSBR thread state entry would acquire a lock (shared->mutex). If no free entry was immediately available, this thread, while still holding the lock, would trigger a "Stop The World" event to resize the underlying array. This could lead to a deadlock if other threads needed to acquire shared->mutex to park themselves as part of the "Stop The World" process.

Previously, a thread attempting to allocate a QSBR thread state entry would acquire a lock (`shared->mutex`). If no free entry was immediately available, this thread, while still holding the lock, would trigger a "Stop The World" event to resize the underlying array. This could lead to a deadlock if other threads needed to acquire `shared->mutex` to park themselves as part of the "Stop The World" process.
See python#148953
@python-cla-bot

python-cla-botBot commented Apr 28, 2026

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

CLA signed

@bedevere-app

Copy link
Copy Markdown

Most changes to Python require a NEWS entry. Add one using the blurb_it web app or the blurb command-line tool.

If this change has little impact on Python users, wait for a maintainer to apply the skip news label instead.

@naschemenascheme self-assigned this Jul 3, 2026
@nascheme

Copy link
Copy Markdown
Member

This looks like a good change to me. You should add a news item (can use the blurb tool). One small suggestion: the comment says threads may need shared->mutex “to park”, which seems a little misleading. Maybe
something like:

 // Unlock before stopping the world to avoid deadlocks. If we hold
// shared->mutex while waiting for the world to stop, another thread
// blocked on this mutex may be unable to detach/suspend and let the
// stop-the-world request complete.

@nascheme

Copy link
Copy Markdown
Member

Thanks for digging into this, but I don't think this change is correct, and I don't think the original code has the problem it's meant to fix.

The patch introduces a deadlock. When applied, PyMutex_Lock(&shared->mutex) runs while this thread owns the stopped world. PyMutex hands off ownership to a waiter that has waited past TIME_TO_BE_FAIR_NS (Python/lock.c:187-190), and the waiter re-attaches its thread state only after it already owns the lock (Python/parking_lot.c:363-365). So:

  1. Thread A unlocks at the top of the loop; parked thread B is handed the mutex while still detached.
  2. A calls _PyEval_StopTheWorld(). B is DETACHED, so park_detached_threads() flips it to SUSPENDED and counts it stopped.
  3. A blocks re-acquiring a mutex now owned by B.
  4. B wakes, tries to re-attach, and parks until start_the_world(), which only A can call.

Neither wait has a timeout, so the whole interpreter hangs. _Py_LOCK_DONT_DETACH doesn't help; the handoff comes from the other side.

Holding the mutex across the stop-the-world is not a deadlock. Every acquirer of shared->mutex uses plain PyMutex_Lock, i.e. _PY_LOCK_DETACH. A thread that blocks on it detaches and is immediately counted as stopped, so it can never stall the STW. The comment in the patch ("we might block a thread that needs to acquire shared->mutex to park") isn't accurate — no thread needs that mutex in order to park; park_detached_threads() transitions other threads with a plain CAS.

The mutex also can't simply be re-acquired after _PyEval_StartTheWorld() instead. grow_thread_array() has to run under it: tstate_delete_common() unlinks a dying thread from interp->threads and decrements the STW countdown before calling _Py_qsbr_unregister() (Python/pystate.c:1980), so that path runs at full speed while the world is stopped. Without the mutex it would race the freelist rebuild and read tstate_imp->qsbr after PyMem_RawFree(old_raw). The existing comment at qsbr.c:264-267 is there for exactly this reason.

Finally, the two stacks in the issue don't show a cycle. A thread blocked on stw->mutex while another has the world stopped is ordinary contention. The thread that matters is whichever one failed to park for the PyEvent_WaitTimed in thread 1, and it isn't in the excerpt.

Closing this. The underlying report is still open.

(Minor, for future reference: Python/qsbr.c is core runtime, so the NEWS entry belongs in Misc/NEWS.d/next/Core_and_Builtins/, and reST wants double backticks rather than ASCII quotes.)

Note: this code review was assisted by Claude Opus.

@naschemenascheme closed this Aug 7, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@AnnaAr321@nascheme