diff --git a/CHANGES.rst b/CHANGES.rst index daacbefd..6b2d2895 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -5,7 +5,15 @@ 3.5.4 (unreleased) ================== -- Nothing changed yet. +- Fix a deadlock on free-threaded builds when a greenlet switch happened + while a ``PyCriticalSection`` was held -- for example inside asyncio's + ``Task.__step``, which holds one on the running task for the duration of + the step. Because a switch swaps C stacks, the locks stayed held on the + greenlet we left, so the greenlet we switched to blocked forever trying to + take one of them. greenlet now suspends a thread's critical sections when + switching away and resumes them when switching back, the same way the + interpreter does when a thread detaches and reattaches. This is what caused + Playwright's synchronous API to hang under free-threading. 3.5.3 (2026-06-26) diff --git a/src/greenlet/TGreenlet.hpp b/src/greenlet/TGreenlet.hpp index 764d46f7..68591b0a 100644 --- a/src/greenlet/TGreenlet.hpp +++ b/src/greenlet/TGreenlet.hpp @@ -39,6 +39,7 @@ using greenlet::refs::BorrowedGreenlet; #endif #ifdef Py_GIL_DISABLED # include "internal/pycore_tstate.h" +# include "internal/pycore_critical_section.h" #endif #endif diff --git a/src/greenlet/TPythonState.cpp b/src/greenlet/TPythonState.cpp index bdc0eef2..321c0465 100644 --- a/src/greenlet/TPythonState.cpp +++ b/src/greenlet/TPythonState.cpp @@ -192,6 +192,19 @@ void PythonState::operator<<(const PyThreadState *const tstate) noexcept // ``greenlet.tests.test_greenlet_trash`` tries, but under 3.14, // at least, fails to do so. this->delete_later = Py_XNewRef(tstate->delete_later); +#ifdef Py_GIL_DISABLED + // Switching greenlets swaps C stacks, which to the free-threaded runtime is + // the same predicament as detaching the thread: the PyCriticalSection nodes + // chained off tstate->critical_section live on the stack we're leaving, and + // their PyMutexes would stay locked behind our back. The greenlet we switch + // to could then block forever taking one of those same locks -- e.g. an + // asyncio event dispatched onto another fiber re-enters a Task/Future that + // the suspended fiber is mid-step on. So drop the locks here the way + // _PyThreadState_Detach() does and let operator>> re-take them on resume. + if (tstate->critical_section != 0) { + _PyCriticalSection_SuspendAll(const_cast(tstate)); + } +#endif this->critical_section = tstate->critical_section; #elif GREENLET_PY312 this->trash_delete_nesting = tstate->trash.delete_nesting; @@ -301,6 +314,16 @@ void PythonState::operator>>(PyThreadState *const tstate) noexcept Py_CLEAR(this->delete_later); } tstate->critical_section = this->critical_section; +#ifdef Py_GIL_DISABLED + // Re-acquire whatever operator<< suspended when this greenlet last yielded. + // A no-op for a greenlet that held no locks, and for a brand-new one whose + // chain starts empty. Mirrors the resume in _PyThreadState_Attach(); note + // _PyCriticalSection_Resume() dereferences the head, so the != 0 guard is + // load-bearing, not just a fast path. + if (tstate->critical_section != 0) { + _PyCriticalSection_Resume(tstate); + } +#endif #elif GREENLET_PY312 tstate->trash.delete_nesting = this->trash_delete_nesting; diff --git a/src/greenlet/tests/fail_switch_critical_section.py b/src/greenlet/tests/fail_switch_critical_section.py new file mode 100644 index 00000000..b8f2c325 --- /dev/null +++ b/src/greenlet/tests/fail_switch_critical_section.py @@ -0,0 +1,38 @@ +"""Out-of-process payload for the free-threaded switch/critical-section deadlock. + +See ``freethread_switch_deadlock.py`` in the repo root for the full write-up. +Before the fix, a greenlet switch swapped C stacks but left any held +``PyCriticalSection`` locks (tracked in ``tstate->critical_section``) locked. +asyncio's ``Task.__step`` holds such a lock on the running task across the step, +so switching into a child greenlet and touching that task from there blocked +forever. A fixed build (and any GIL-enabled build) prints the sentinel; a +regressed free-threaded build deadlocks, and the watchdog turns that hang into a +non-zero exit so the test fails loudly instead of stalling the whole suite. +""" +import asyncio +import faulthandler +import sys + +import greenlet + +# The deadlock is immediate when present; the generous timeout is only so a +# genuinely regressed build still exits on the slowest CI, never on a good one. +faulthandler.dump_traceback_later(15, exit=True) + + +async def main(): + task = asyncio.current_task() # its running __step holds a lock on `task` + + def in_child_fiber(): + # Fresh C stack; the task's lock is still held by the fiber we left. + # A regressed build never returns from this first call. + task.add_done_callback(lambda _: None) + task.remove_done_callback(lambda _: None) + + greenlet.greenlet(in_child_fiber).switch() + + +asyncio.run(main()) +faulthandler.cancel_dump_traceback_later() +print("SWITCH CS OK") +sys.exit(0) diff --git a/src/greenlet/tests/test_greenlet.py b/src/greenlet/tests/test_greenlet.py index 7350fc3e..1a701688 100644 --- a/src/greenlet/tests/test_greenlet.py +++ b/src/greenlet/tests/test_greenlet.py @@ -1468,6 +1468,20 @@ def test_reentrant_switch_run_callable_has_del(self): output ) + def test_switch_leaves_no_critical_section_held(self): + # Free-threaded builds take per-object locks via + # Py_BEGIN_CRITICAL_SECTION, tracked in tstate->critical_section. A + # switch swaps C stacks, so leaving those locks held strands them on the + # fiber we left: asyncio's Task.__step holds one on the running task + # across the step, and touching that task from a child fiber then + # deadlocked re-taking it. Out of process because a regression is a + # hang, not a catchable error. + # (repro: freethread_switch_deadlock.py in the repo root) + if not RUNNING_ON_FREETHREAD_BUILD: + self.skipTest("Only free-threaded builds take critical sections") + output = self.run_script('fail_switch_critical_section.py') + self.assertIn('SWITCH CS OK', output) + class TestModule(TestCase): @unittest.skipUnless(hasattr(sys, '_is_gil_enabled'),