summaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorSam Gross <colesbury@gmail.com>2024-05-31 17:04:59 (GMT)
committerGitHub <noreply@github.com>2024-05-31 17:04:59 (GMT)
commit078b8c8cf2bf68f7484cc4d2e3dd74b6fab55664 (patch)
treed77ecd24857b1a6240db0edebcda80a69b24b7a7
parent64ff1e217d963b48140326e8b63c62f4b306f4a0 (diff)
downloadcpython-078b8c8cf2bf68f7484cc4d2e3dd74b6fab55664.zip
cpython-078b8c8cf2bf68f7484cc4d2e3dd74b6fab55664.tar.gz
cpython-078b8c8cf2bf68f7484cc4d2e3dd74b6fab55664.tar.bz2
gh-119369: Fix deadlock during thread exit in free-threaded build (#119528)
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.
-rw-r--r--Misc/NEWS.d/next/Core and Builtins/2024-05-24-21-16-52.gh-issue-119369.qBThho.rst2
-rw-r--r--Python/pystate.c21
-rw-r--r--Python/qsbr.c5
3 files changed, 19 insertions, 9 deletions
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 0000000..7abdd5c
--- /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 ad7e082..36e4206 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 1e02ff9..9cbce90 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