[Python-checkins] [3.13] gh-119369: Fix deadlock during thread exit in free-threaded build (GH-119528) (#119868)

[colesbury]

https://github.com/python/cpython/commit/a7e81fdfc175bd9cf82dfd8f1e1853cb942bf0f0
commit: a7e81fdfc175bd9cf82dfd8f1e1853cb942bf0f0
branch: 3.13
author: Miss Islington (bot) <31488909+miss-isling...@users.noreply.github.com>
committer: colesbury <colesb...@gmail.com>
date: 2024-05-31T14:19:38-04:00
summary:

[3.13] gh-119369: Fix deadlock during thread exit in free-threaded build 
(GH-119528) (#119868)

Release the GIL before calling `_Py_qsbr_unregister`.

The deadlock could occur when the GIL was enabled at runtime. The
`_Py_qsbr_unregister` call might block while holding the GIL because the
thread state was not active, but the GIL was still held.
(cherry picked from commit 078b8c8cf2bf68f7484cc4d2e3dd74b6fab55664)

Co-authored-by: Sam Gross <colesb...@gmail.com>

files:
A Misc/NEWS.d/next/Core and 
Builtins/2024-05-24-21-16-52.gh-issue-119369.qBThho.rst
M Python/pystate.c
M Python/qsbr.c

diff --git a/Misc/NEWS.d/next/Core and 
Builtins/2024-05-24-21-16-52.gh-issue-119369.qBThho.rst b/Misc/NEWS.d/next/Core 
and Builtins/2024-05-24-21-16-52.gh-issue-119369.qBThho.rst
new file mode 100644
index 00000000000000..7abdd5cd85ccd6
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and 
Builtins/2024-05-24-21-16-52.gh-issue-119369.qBThho.rst 
@@ -0,0 +1,2 @@
+Fix deadlock during thread deletion in free-threaded build, which could
+occur when the GIL was enabled at runtime.
diff --git a/Python/pystate.c b/Python/pystate.c
index ad7e082ce0d37e..36e4206b4a282e 100644
--- a/Python/pystate.c
+++ b/Python/pystate.c
@@ -1751,7 +1751,7 @@ decrement_stoptheworld_countdown(struct 
_stoptheworld_state *stw);
 
 /* Common code for PyThreadState_Delete() and PyThreadState_DeleteCurrent() */
 static void
-tstate_delete_common(PyThreadState *tstate)
+tstate_delete_common(PyThreadState *tstate, int release_gil)
 {
     assert(tstate->_status.cleared && !tstate->_status.finalized);
     tstate_verify_not_active(tstate);
@@ -1793,10 +1793,6 @@ tstate_delete_common(PyThreadState *tstate)
 
     HEAD_UNLOCK(runtime);
 
-#ifdef Py_GIL_DISABLED
-    _Py_qsbr_unregister(tstate);
-#endif
-
     // XXX Unbind in PyThreadState_Clear(), or earlier
     // (and assert not-equal here)?
     if (tstate->_status.bound_gilstate) {
@@ -1807,6 +1803,14 @@ tstate_delete_common(PyThreadState *tstate)
     // XXX Move to PyThreadState_Clear()?
     clear_datastack(tstate);
 
+    if (release_gil) {
+        _PyEval_ReleaseLock(tstate->interp, tstate, 1);
+    }
+
+#ifdef Py_GIL_DISABLED
+    _Py_qsbr_unregister(tstate);
+#endif
+
     tstate->_status.finalized = 1;
 }
 
@@ -1818,7 +1822,7 @@ zapthreads(PyInterpreterState *interp)
        when the threads are all really dead (XXX famous last words). */
     while ((tstate = interp->threads.head) != NULL) {
         tstate_verify_not_active(tstate);
-        tstate_delete_common(tstate);
+        tstate_delete_common(tstate, 0);
         free_threadstate((_PyThreadStateImpl *)tstate);
     }
 }
@@ -1829,7 +1833,7 @@ PyThreadState_Delete(PyThreadState *tstate)
 {
     _Py_EnsureTstateNotNULL(tstate);
     tstate_verify_not_active(tstate);
-    tstate_delete_common(tstate);
+    tstate_delete_common(tstate, 0);
     free_threadstate((_PyThreadStateImpl *)tstate);
 }
 
@@ -1842,8 +1846,7 @@ _PyThreadState_DeleteCurrent(PyThreadState *tstate)
     _Py_qsbr_detach(((_PyThreadStateImpl *)tstate)->qsbr);
 #endif
     current_fast_clear(tstate->interp->runtime);
-    tstate_delete_common(tstate);
-    _PyEval_ReleaseLock(tstate->interp, tstate, 1);
+    tstate_delete_common(tstate, 1);  // release GIL as part of call
     free_threadstate((_PyThreadStateImpl *)tstate);
 }
 
diff --git a/Python/qsbr.c b/Python/qsbr.c
index 1e02ff9c2e45f0..9cbce9044e2941 100644
--- a/Python/qsbr.c
+++ b/Python/qsbr.c
@@ -236,6 +236,11 @@ _Py_qsbr_unregister(PyThreadState *tstate)
     struct _qsbr_shared *shared = &tstate->interp->qsbr;
     struct _PyThreadStateImpl *tstate_imp = (_PyThreadStateImpl*) tstate;
 
+    // gh-119369: GIL must be released (if held) to prevent deadlocks, because
+    // we might not have an active tstate, which means taht blocking on PyMutex
+    // locks will not implicitly release the GIL.
+    assert(!tstate->_status.holds_gil);
+
     PyMutex_Lock(&shared->mutex);
     // NOTE: we must load (or reload) the thread state's qbsr inside the mutex
     // because the array may have been resized (changing tstate->qsbr) while

_______________________________________________
Python-checkins mailing list -- python-checkins@python.org
To unsubscribe send an email to python-checkins-le...@python.org
https://mail.python.org/mailman3/lists/python-checkins.python.org/
Member address: arch...@mail-archive.com