Merge pull request #519 from ddorian/freethread-switch-critical-section

Fix free-threaded deadlock when switching greenlets holding a critical section

Author: jamadden

CHANGES.rst

@@ -5,8 +5,11 @@
 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. See `PR 519 <https://github.com/python-greenlet/greenlet/pull/519/>`. 
+  Thank to ddorian and Kumar Aditya.
 
 3.5.3 (2026-06-26)
 ==================

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
 

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<PyThreadState*>(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;

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)

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'),