From f840e00dea524c20801bcb4f8764b968590eb6ba Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 6 Apr 2026 12:57:46 -0500 Subject: [PATCH 1/6] Add critical sections to greenlet attribute accessors. --- src/greenlet/PyGreenlet.cpp | 16 ++++++++++++---- src/greenlet/greenlet_refs.hpp | 33 +++++++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/src/greenlet/PyGreenlet.cpp b/src/greenlet/PyGreenlet.cpp index 95cf6992..20d4d08e 100644 --- a/src/greenlet/PyGreenlet.cpp +++ b/src/greenlet/PyGreenlet.cpp @@ -47,7 +47,7 @@ using greenlet::MainGreenlet; using greenlet::BrokenGreenlet; using greenlet::ThreadState; using greenlet::PythonState; - +using greenlet::refs::PyCriticalObjectSection; static PyGreenlet* @@ -489,6 +489,7 @@ green_bool(PyGreenlet* self) static PyObject* green_getdict(PyGreenlet* self, void* UNUSED(context)) { + PyCriticalObjectSection cs(self); if (self->dict == NULL) { self->dict = PyDict_New(); if (self->dict == NULL) { @@ -502,8 +503,6 @@ green_getdict(PyGreenlet* self, void* UNUSED(context)) static int green_setdict(PyGreenlet* self, PyObject* val, void* UNUSED(context)) { - PyObject* tmp; - if (val == NULL) { PyErr_SetString(PyExc_TypeError, "__dict__ may not be deleted"); return -1; @@ -512,7 +511,8 @@ green_setdict(PyGreenlet* self, PyObject* val, void* UNUSED(context)) PyErr_SetString(PyExc_TypeError, "__dict__ must be a dictionary"); return -1; } - tmp = self->dict; + PyCriticalObjectSection cs(self); + PyObject* tmp = self->dict; Py_INCREF(val); self->dict = val; Py_XDECREF(tmp); @@ -535,6 +535,7 @@ _green_not_dead(BorrowedGreenlet self) static PyObject* green_getdead(PyGreenlet* self, void* UNUSED(context)) { + PyCriticalObjectSection cs(self); if (_green_not_dead(self)) { Py_RETURN_FALSE; } @@ -553,6 +554,7 @@ green_get_stack_saved(PyGreenlet* self, void* UNUSED(context)) static PyObject* green_getrun(PyGreenlet* self, void* UNUSED(context)) { + PyCriticalObjectSection cs(self); try { OwnedObject result(BorrowedGreenlet(self)->run()); return result.relinquish_ownership(); @@ -566,6 +568,7 @@ green_getrun(PyGreenlet* self, void* UNUSED(context)) static int green_setrun(PyGreenlet* self, PyObject* nrun, void* UNUSED(context)) { + PyCriticalObjectSection cs(self); try { BorrowedGreenlet(self)->run(nrun); return 0; @@ -578,6 +581,7 @@ green_setrun(PyGreenlet* self, PyObject* nrun, void* UNUSED(context)) static PyObject* green_getparent(PyGreenlet* self, void* UNUSED(context)) { + PyCriticalObjectSection cs(self); return BorrowedGreenlet(self)->parent().acquire_or_None(); } @@ -585,6 +589,7 @@ green_getparent(PyGreenlet* self, void* UNUSED(context)) static int green_setparent(PyGreenlet* self, PyObject* nparent, void* UNUSED(context)) { + PyCriticalObjectSection cs(self); try { BorrowedGreenlet(self)->parent(nparent); } @@ -598,6 +603,7 @@ green_setparent(PyGreenlet* self, PyObject* nparent, void* UNUSED(context)) static PyObject* green_getcontext(const PyGreenlet* self, void* UNUSED(context)) { + PyCriticalObjectSection cs(self); const Greenlet *const g = self->pimpl; try { OwnedObject result(g->context()); @@ -611,6 +617,7 @@ green_getcontext(const PyGreenlet* self, void* UNUSED(context)) static int green_setcontext(PyGreenlet* self, PyObject* nctx, void* UNUSED(context)) { + PyCriticalObjectSection cs(self); try { BorrowedGreenlet(self)->context(nctx); return 0; @@ -624,6 +631,7 @@ green_setcontext(PyGreenlet* self, PyObject* nctx, void* UNUSED(context)) static PyObject* green_getframe(PyGreenlet* self, void* UNUSED(context)) { + PyCriticalObjectSection cs(self); const PythonState::OwnedFrame& top_frame = BorrowedGreenlet(self)->top_frame(); return top_frame.acquire_or_None(); } diff --git a/src/greenlet/greenlet_refs.hpp b/src/greenlet/greenlet_refs.hpp index 49778cc8..7a87863f 100644 --- a/src/greenlet/greenlet_refs.hpp +++ b/src/greenlet/greenlet_refs.hpp @@ -1134,6 +1134,39 @@ namespace greenlet { } }; +#ifdef Py_GIL_DISABLED + // building on 3.13 or newer, free-threaded + class PyCriticalObjectSection { + private: + G_NO_COPIES_OF_CLS(PyCriticalObjectSection); + PyCriticalSection _py_cs; + public: + explicit PyCriticalObjectSection(PyObject* p) + { + PyCriticalSection_Begin(&this->_py_cs, p); + } + explicit PyCriticalObjectSection(const PyGreenlet* p) + : PyCriticalObjectSection( + reinterpret_cast( + const_cast(p))) + {} + ~PyCriticalObjectSection() + { + PyCriticalSection_End(&this->_py_cs); + } + }; +#else + class PyCriticalObjectSection { + public: + explicit PyCriticalObjectSection(PyObject* UNUSED(p)) + {} + explicit PyCriticalObjectSection(const PyGreenlet* UNUSED(p)) + {} + }; + +#endif + + };}; #endif From 449c76045b71f7f96c48e8d62672e5382b17cc3d Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 6 Apr 2026 16:03:42 -0500 Subject: [PATCH 2/6] Make MainGreenlet._thread_state atomic; we use it for cross thread checking all the time. Broaden an assert in ~TThreadState to allow for the thread being dead. Slightly better atomicity for marking a thread dead. Turn an assertion into an explicit error check in UserGreenlet.g_switch. Add a test that was hitting some invalid assertions and crashing, which it no longer does. --- src/greenlet/TGreenlet.cpp | 2 +- src/greenlet/TGreenlet.hpp | 2 +- src/greenlet/TGreenletGlobals.cpp | 23 +++++++++- src/greenlet/TMainGreenlet.cpp | 7 ++- src/greenlet/TThreadState.hpp | 25 ++++++++++- src/greenlet/TThreadStateCreator.hpp | 3 ++ src/greenlet/TThreadStateDestroy.cpp | 35 +++++++-------- src/greenlet/TUserGreenlet.cpp | 9 +++- src/greenlet/greenlet_refs.hpp | 2 + src/greenlet/tests/test_greenlet.py | 65 ++++++++++++++++++++++++++++ 10 files changed, 144 insertions(+), 29 deletions(-) diff --git a/src/greenlet/TGreenlet.cpp b/src/greenlet/TGreenlet.cpp index 2d1f71da..e3a9b237 100644 --- a/src/greenlet/TGreenlet.cpp +++ b/src/greenlet/TGreenlet.cpp @@ -230,7 +230,7 @@ Greenlet::check_switch_allowed() const // If the thread this greenlet was running in is dead, // we'll still have a reference to a main greenlet, but the - // thread state pointer we have is bogus. + // thread state pointer we have is bogus (should be nullptr) // TODO: Give the objects an API to determine if they belong // to a dead thread. diff --git a/src/greenlet/TGreenlet.hpp b/src/greenlet/TGreenlet.hpp index e9372c55..af602092 100644 --- a/src/greenlet/TGreenlet.hpp +++ b/src/greenlet/TGreenlet.hpp @@ -754,7 +754,7 @@ class TracingGuard private: static greenlet::PythonAllocator allocator; refs::BorrowedMainGreenlet _self; - ThreadState* _thread_state; + std::atomic _thread_state; G_NO_COPIES_OF_CLS(MainGreenlet); public: static void* operator new(size_t UNUSED(count)); diff --git a/src/greenlet/TGreenletGlobals.cpp b/src/greenlet/TGreenletGlobals.cpp index 0087d2ff..a67797ad 100644 --- a/src/greenlet/TGreenletGlobals.cpp +++ b/src/greenlet/TGreenletGlobals.cpp @@ -12,6 +12,8 @@ #ifndef T_GREENLET_GLOBALS #define T_GREENLET_GLOBALS +#include + #include "greenlet_refs.hpp" #include "greenlet_exceptions.hpp" #include "greenlet_thread_support.hpp" @@ -66,6 +68,9 @@ class GreenletGlobals // do any deallocation.) } + /** + * Must be holding the ``thread_states_to_destroy`` lock. + */ void queue_to_destroy(ThreadState* ts) const { // we're currently accessed through a static const object, @@ -74,13 +79,27 @@ class GreenletGlobals // const. // // Do that for callers. - greenlet::cleanup_queue_t& q = const_cast(this->thread_states_to_destroy); + greenlet::cleanup_queue_t& q = const_cast( + this->thread_states_to_destroy); + // make sure we don't ever try to clean up a state more than + // once. Because they're thread-local, and we ultimately call this + // method from the destructor of the thread local variable, + // we should never find the item already present. This check + // is nominally O(n) in the size of the vector. + assert(std::find(q.begin(), q.end(), ts) == q.end()); q.push_back(ts); } + /** + * Must be holding the ``thread_states_to_destroy`` lock. + */ ThreadState* take_next_to_destroy() const { - greenlet::cleanup_queue_t& q = const_cast(this->thread_states_to_destroy); + greenlet::cleanup_queue_t& q = const_cast( + this->thread_states_to_destroy); + if (q.empty()) { + return nullptr; + } ThreadState* result = q.back(); q.pop_back(); return result; diff --git a/src/greenlet/TMainGreenlet.cpp b/src/greenlet/TMainGreenlet.cpp index ee014812..e0db0a4f 100644 --- a/src/greenlet/TMainGreenlet.cpp +++ b/src/greenlet/TMainGreenlet.cpp @@ -66,6 +66,8 @@ MainGreenlet::thread_state() const noexcept void MainGreenlet::thread_state(ThreadState* t) noexcept { + // this method is only used during thread tear down, when it is + // called with nullptr, signalling the thread is dead. assert(!t); this->_thread_state = t; } @@ -118,9 +120,10 @@ MainGreenlet::g_switch() int MainGreenlet::tp_traverse(visitproc visit, void* arg) { - if (this->_thread_state) { + ThreadState* thread_state = this->_thread_state.load(); + if (thread_state) { // we've already traversed main, (self), don't do it again. - int result = this->_thread_state->tp_traverse(visit, arg, false); + int result = thread_state->tp_traverse(visit, arg, false); if (result) { return result; } diff --git a/src/greenlet/TThreadState.hpp b/src/greenlet/TThreadState.hpp index 98ee526f..fe74ce36 100644 --- a/src/greenlet/TThreadState.hpp +++ b/src/greenlet/TThreadState.hpp @@ -248,6 +248,23 @@ class ThreadState { return this->main_greenlet; } + /** + * If we have a main greenlet, mark it as dead by setting its + * thread_state to null (this part is atomic with respect to other + * threads looking at the main greenlet's thread_state). + */ + inline bool mark_main_greenlet_dead() noexcept + { + PyGreenlet* main_greenlet = this->main_greenlet.borrow(); + if (!main_greenlet) { + return false; + } + assert(main_greenlet->pimpl->thread_state() == this + || main_greenlet->pimpl->thread_state() == nullptr); + dynamic_cast(main_greenlet->pimpl)->thread_state(nullptr); + return true; + } + /** * In addition to returning a new reference to the currunt * greenlet, this performs any maintenance needed. @@ -440,6 +457,9 @@ class ThreadState { #endif } + // Runs in some arbitrary thread that Python is using to invoke + // pending callbacks. This may not be the thread that was + // running the greenlets. ~ThreadState() { if (!PyInterpreterState_Head()) { @@ -485,7 +505,10 @@ class ThreadState { // switched to us, leaving a reference to the main greenlet // on the stack, somewhere uncollectible. Try to detect that. if (this->current_greenlet == this->main_greenlet && this->current_greenlet) { - assert(this->current_greenlet->is_currently_running_in_some_thread()); + assert( + this->current_greenlet->is_currently_running_in_some_thread() + || this->current_greenlet->was_running_in_dead_thread() + ); // Drop one reference we hold. this->current_greenlet.CLEAR(); assert(!this->current_greenlet); diff --git a/src/greenlet/TThreadStateCreator.hpp b/src/greenlet/TThreadStateCreator.hpp index ebd33a3b..b6d7d597 100644 --- a/src/greenlet/TThreadStateCreator.hpp +++ b/src/greenlet/TThreadStateCreator.hpp @@ -16,6 +16,9 @@ namespace greenlet { typedef void (*ThreadStateDestructor)(ThreadState* const); // Only one of these, auto created per thread as a thread_local. +// This means we don't have to worry about atomic access to the +// internals, because by definition all access is happening on a +// single thread. // Constructing the state constructs the MainGreenlet. template class ThreadStateCreator diff --git a/src/greenlet/TThreadStateDestroy.cpp b/src/greenlet/TThreadStateDestroy.cpp index 268bc818..be2d9643 100644 --- a/src/greenlet/TThreadStateDestroy.cpp +++ b/src/greenlet/TThreadStateDestroy.cpp @@ -75,18 +75,15 @@ struct ThreadState_DestroyNoGIL return false; } LockGuard cleanup_lock(*mod_globs->thread_states_to_destroy_lock); - if (state->has_main_greenlet()) { - // mark the thread as dead ASAP. - // this is racy! If we try to throw or switch to a - // greenlet from this thread from some other thread before - // we clear the state pointer, it won't realize the state - // is dead which can crash the process. - PyGreenlet* p(state->borrow_main_greenlet().borrow()); - assert(p->pimpl->thread_state() == state || p->pimpl->thread_state() == nullptr); - dynamic_cast(p->pimpl)->thread_state(nullptr); - return true; - } - return false; + // mark the thread as dead ASAP. + // TODO: While the state variable tracking the death is + // atomic, and used with the strictest memory ordering, could + // this still be hiding race conditions? Specifically, is + // there a scenario where a thread is dying and thread local + // variables are being deconstructed, and some other thread + // tries to switch/throw to a greenlet owned by this thread, + // such that we think the switch will work but it won't? + return state->mark_main_greenlet_dead(); } static void @@ -161,15 +158,13 @@ struct ThreadState_DestroyNoGIL // May or may not be holding the GIL (depending on Py_GIL_DISABLED). // Passed a non-shared pointer to the actual thread state. // state -> main greenlet - assert(state->has_main_greenlet()); + // + // The thread_state in the main greenlet has already been + // cleared by the time this function runs from our pending + // callback, but the greenlet itself is still there. PyGreenlet* main(state->borrow_main_greenlet()); - // When we need to do cross-thread operations, we check this. - // A NULL value means the thread died some time ago. - // We do this here, rather than in a Python dealloc function - // for the greenlet, in case there's still a reference out - // there. - dynamic_cast(main->pimpl)->thread_state(nullptr); - + assert(main); + assert(main->pimpl->thread_state() == nullptr); delete state; // Deleting this runs the destructor, DECREFs the main greenlet. } diff --git a/src/greenlet/TUserGreenlet.cpp b/src/greenlet/TUserGreenlet.cpp index 73a81330..bb939e80 100644 --- a/src/greenlet/TUserGreenlet.cpp +++ b/src/greenlet/TUserGreenlet.cpp @@ -117,9 +117,14 @@ UserGreenlet::was_running_in_dead_thread() const noexcept OwnedObject UserGreenlet::g_switch() { - assert(this->args() || PyErr_Occurred()); - try { + if (!this->args() && !PyErr_Occurred()) { + // we have nothing to send as the result of switching, most + // likely the result of trying to switch to a dead + // greenlet. + throw PyErrOccurred(mod_globs->PyExc_GreenletError, + "cannot switch with no pending arguments or exception"); + } this->check_switch_allowed(); } catch (const PyErrOccurred&) { diff --git a/src/greenlet/greenlet_refs.hpp b/src/greenlet/greenlet_refs.hpp index 7a87863f..55466e00 100644 --- a/src/greenlet/greenlet_refs.hpp +++ b/src/greenlet/greenlet_refs.hpp @@ -446,6 +446,8 @@ namespace greenlet { void CLEAR() { Py_CLEAR(this->p); + // XXX: Have failed this assertion in free-threaded builds + // using multiple threads assert(this->p == nullptr); } }; diff --git a/src/greenlet/tests/test_greenlet.py b/src/greenlet/tests/test_greenlet.py index a0fd4193..5e184518 100644 --- a/src/greenlet/tests/test_greenlet.py +++ b/src/greenlet/tests/test_greenlet.py @@ -15,6 +15,7 @@ from . import PY314 from . import RUNNING_ON_FREETHREAD_BUILD from .leakcheck import fails_leakcheck +from .leakcheck import ignores_leakcheck # We manually manage locks in many tests @@ -148,6 +149,70 @@ def f(): th.join(10) self.assertEqual(len(success), len(ths)) + @ignores_leakcheck + def test_switching_many_threads(self): + # This can expose issues on the free-threaded build. + def creates_greenlet(greenlets, wait_to_die_until): + def body(): + while True: + greenlet.getcurrent().parent.switch() + g = greenlet.greenlet(body) + g.switch() + greenlets.append(g) + wait_to_die_until.wait() + + def switches_from_other_thread(greenlets, wait_to_check_until, quit_after): + wait_to_check_until.wait() + while not quit_after.is_set(): + for g in list(greenlets): + try: + if not g.dead: + g.switch() + except greenlet.error: + # Every attempt where the greenlet isn't dead should + # raise this + pass + + def run_it(thread_count=16): + greenlets = [] + greenlets_all_created = threading.Event() + all_threads_dead = threading.Event() + + creators = [ + threading.Thread(target=creates_greenlet, args=(greenlets, + greenlets_all_created)) + for _ + in range(thread_count) + ] + + switchers = [ + threading.Thread(target=switches_from_other_thread, + args=(greenlets, greenlets_all_created, + all_threads_dead)) + for _ + in range(thread_count) + ] + + for t in (creators + switchers): + t.start() + + # Simple polling loop + while len(greenlets) < thread_count: + time.sleep(0.0001) + greenlets_all_created.set() + + for t in creators: + t.join(1.0) + all_threads_dead.set() + for t in switchers: + t.join(1.0) + + # enough reps to have a chance of repeating, not so many it + # takes forever + for _ in range(20): + run_it() + gc.collect() + def test_exception(self): seen = [] g1 = RawGreenlet(fmain) From 8688581392187d68f35180148fcd6fb4fd9a972f Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 6 Apr 2026 16:06:34 -0500 Subject: [PATCH 3/6] gcc was complaining about an incomplete std::atomic type. make sure we include the header before using it. --- src/greenlet/TGreenlet.hpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/greenlet/TGreenlet.hpp b/src/greenlet/TGreenlet.hpp index af602092..9713821b 100644 --- a/src/greenlet/TGreenlet.hpp +++ b/src/greenlet/TGreenlet.hpp @@ -7,6 +7,8 @@ #define PY_SSIZE_T_CLEAN #include +#include + #include "greenlet_compiler_compat.hpp" #include "greenlet_refs.hpp" #include "greenlet_cpython_compat.hpp" From a0c2a2a7519985d5fe2c034a54f1a0fed82a5905 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 6 Apr 2026 16:09:18 -0500 Subject: [PATCH 4/6] Fix unused variable warning when asserts are disabled. --- src/greenlet/TThreadStateDestroy.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/greenlet/TThreadStateDestroy.cpp b/src/greenlet/TThreadStateDestroy.cpp index be2d9643..5562a736 100644 --- a/src/greenlet/TThreadStateDestroy.cpp +++ b/src/greenlet/TThreadStateDestroy.cpp @@ -162,9 +162,11 @@ struct ThreadState_DestroyNoGIL // The thread_state in the main greenlet has already been // cleared by the time this function runs from our pending // callback, but the greenlet itself is still there. +#ifndef NDEBUG PyGreenlet* main(state->borrow_main_greenlet()); assert(main); assert(main->pimpl->thread_state() == nullptr); +#endif delete state; // Deleting this runs the destructor, DECREFs the main greenlet. } From 2f4a1cf53fa282ab28ea4815164a9cb09b9320ce Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Tue, 7 Apr 2026 09:21:41 -0500 Subject: [PATCH 5/6] Make green_switch (python level greenlet.switch) and green_throw check for (prohibit) cross-thread switches early on free-threaded builds. This fixes some corruption around argument handling. The TLBC issue may also be corrected; at least, I don't reproduce it anymore. --- .github/workflows/tests.yml | 14 -------------- CHANGES.rst | 12 +++++++++++- docs/caveats.rst | 7 +++++-- src/greenlet/PyGreenlet.cpp | 33 +++++++++++++++++++++++++++++++++ src/greenlet/TGreenlet.cpp | 18 +++++++++--------- src/greenlet/TGreenlet.hpp | 22 +++++++++++++++++----- src/greenlet/TUserGreenlet.cpp | 13 ++++++++++--- src/greenlet/greenlet_refs.hpp | 2 -- tox.ini | 2 +- 9 files changed, 86 insertions(+), 37 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index cdb57409..2651641a 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -114,20 +114,6 @@ jobs: python -c 'import greenlet._greenlet as G; assert G.GREENLET_USE_STANDARD_THREADING' python -m unittest discover -v greenlet.tests - name: Doctest - env: - # XXX: On 3.14t, when the thread-local bytecode cache is - # enabled, sphinx crashes on module cleanup: there is a - # reference to pdb.set_trace in ``glob``, and when the - # shutdown sequence tries to clear that value, it segfaults - # dec-reffing it after taking it out of the module dict. The - # object appears to be corrupt, but it is utterly unclear how - # we could have done this. It is 100% reliable on bath Mac and - # Linux. It can be traced down to a very simple doctest - # snippet in greenlet_gc.rst, but running that same snippet - # standalone in a unit test doesn't produce the error. - # - # So this is a temporary workaround. - PYTHON_TLBC: "0" run: | sphinx-build -b doctest -d docs/_build/doctrees2 docs docs/_build/doctest2 - name: Lint diff --git a/CHANGES.rst b/CHANGES.rst index 6fdc96df..587175b8 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -16,12 +16,22 @@ `_ by Nicolas Bouvrette. - Address the results of an automated code audit performed by - devdanzin. This includes several minor correctness changes that + Daniel Diniz. This includes several minor correctness changes that theoretically could have been crashing bugs, but typically only in very rare circumstances. See `PR 502 `_. +- Fix several race conditions that could arise in free-threaded + builds when using greenlet objects from multiple threads, some of + which could lead to assertion failures or interpreter crashes. On CI, + this also eliminated the need to disable the thread-local bytecode + cache. + + See `issue 503 + `_, with + thanks to Nitay Dariel and Daniel Diniz. + 3.3.2 (2026-02-20) ================== diff --git a/docs/caveats.rst b/docs/caveats.rst index 7d773cfb..1335d8ab 100644 --- a/docs/caveats.rst +++ b/docs/caveats.rst @@ -37,7 +37,8 @@ Free-threading Is Experimental ============================== Beginning with greenlet 3.3.0, support for Python 3.14's free-threaded -mode is enabled. Use caution, as it has only limited testing. +mode is enabled; greenlet 3.4.0 expands this support and fixes several +race conditions. Use caution, as it has only limited testing. There are known issues running greenlets in a free-threaded CPython. These include: @@ -52,4 +53,6 @@ These include: hence the specializing interpreter) to avoid a rare crash. If your process crashes on accessing an attribute or object, or at shutdown during module cleanup, try setting the environment variable - ``PYTHON_TLBC=0`` or using the ``-X tlbc=0`` argument. + ``PYTHON_TLBC=0`` or using the ``-X tlbc=0`` argument. (If you + encounter this with greenlet 3.4, please open an issue and let the + maintainers know.) diff --git a/src/greenlet/PyGreenlet.cpp b/src/greenlet/PyGreenlet.cpp index 20d4d08e..b022d5c0 100644 --- a/src/greenlet/PyGreenlet.cpp +++ b/src/greenlet/PyGreenlet.cpp @@ -372,6 +372,29 @@ PyDoc_STRVAR( static PyObject* green_switch(PyGreenlet* self, PyObject* args, PyObject* kwargs) { + // Our use of Greenlet::args() makes this method non-reentrant. + // Therefore, check to be sure the switch will be allowed --- + // we're calling from the same thread that ``self`` belongs to --- + // BEFORE doing anything with args(). If we don't do this, we can + // find args() getting clobbered by switches that will never + // succeed. + // + // TODO: We're only doing this for free-threaded builds because + // those are the only ones that have demonstrated an issue, + // trusting our later checks in g_switch to perform the same + // function and the GIL to keep us from being reentered in regular + // builds. BUT should we always do this as an extra measure of + // safety in case we run code at unexpected times (e.g., a GC?) +#ifdef Py_GIL_DISABLED + try { + self->pimpl->check_switch_allowed(); + } + catch (const PyErrOccurred&) { + return nullptr; + } +#endif + + using greenlet::SwitchingArgs; SwitchingArgs switch_args(OwnedObject::owning(args), OwnedObject::owning(kwargs)); self->pimpl->may_switch_away(); @@ -454,6 +477,16 @@ PyDoc_STRVAR( static PyObject* green_throw(PyGreenlet* self, PyObject* args) { + // See green_switch for why we call this early. +#ifdef Py_GIL_DISABLED + try { + self->pimpl->check_switch_allowed(); + } + catch (const PyErrOccurred&) { + return nullptr; + } +#endif + PyArgParseParam typ(mod_globs->PyExc_GreenletExit); PyArgParseParam val; PyArgParseParam tb; diff --git a/src/greenlet/TGreenlet.cpp b/src/greenlet/TGreenlet.cpp index e3a9b237..7ec86f98 100644 --- a/src/greenlet/TGreenlet.cpp +++ b/src/greenlet/TGreenlet.cpp @@ -198,7 +198,7 @@ Greenlet::g_switchstack(void) // No stack-based variables are valid anymore. - // But the global is volatile so we can reload it without the + // But the global is thread_local volatile so we can reload it without the // compiler caching it from earlier. Greenlet* greenlet_that_switched_in = switching_thread_state; // aka this switching_thread_state = nullptr; @@ -234,14 +234,14 @@ Greenlet::check_switch_allowed() const // TODO: Give the objects an API to determine if they belong // to a dead thread. - const BorrowedMainGreenlet main_greenlet = this->find_main_greenlet_in_lineage(); + const BorrowedMainGreenlet my_main_greenlet = this->find_main_greenlet_in_lineage(); - if (!main_greenlet) { + if (!my_main_greenlet) { throw PyErrOccurred(mod_globs->PyExc_GreenletError, "cannot switch to a garbage collected greenlet"); } - if (!main_greenlet->thread_state()) { + if (!my_main_greenlet->thread_state()) { throw PyErrOccurred(mod_globs->PyExc_GreenletError, "cannot switch to a different thread (which happens to have exited)"); } @@ -255,26 +255,26 @@ Greenlet::check_switch_allowed() const // may not be visible yet. So we need to check against the // current thread state (once the cheaper checks are out of // the way) - const BorrowedMainGreenlet current_main_greenlet = GET_THREAD_STATE().state().borrow_main_greenlet(); + const BorrowedMainGreenlet main_greenlet_cur_thread = GET_THREAD_STATE().state().borrow_main_greenlet(); if ( // lineage main greenlet is not this thread's greenlet - current_main_greenlet != main_greenlet + main_greenlet_cur_thread != my_main_greenlet || ( // atteched to some thread this->main_greenlet() // XXX: Same condition as above. Was this supposed to be // this->main_greenlet()? - && current_main_greenlet != main_greenlet) + && main_greenlet_cur_thread != my_main_greenlet) // switching into a known dead thread (XXX: which, if we get here, // is bad, because we just accessed the thread state, which is // gone!) - || (!current_main_greenlet->thread_state())) { + || (!main_greenlet_cur_thread->thread_state())) { // CAUTION: This may trigger memory allocations, gc, and // arbitrary Python code. throw PyErrOccurred( mod_globs->PyExc_GreenletError, "Cannot switch to a different thread\n\tCurrent: %R\n\tExpected: %R", - current_main_greenlet, main_greenlet); + main_greenlet_cur_thread, my_main_greenlet); } } diff --git a/src/greenlet/TGreenlet.hpp b/src/greenlet/TGreenlet.hpp index 9713821b..0d4e0d9c 100644 --- a/src/greenlet/TGreenlet.hpp +++ b/src/greenlet/TGreenlet.hpp @@ -403,6 +403,18 @@ namespace greenlet } virtual OwnedObject throw_GreenletExit_during_dealloc(const ThreadState& current_thread_state); + + /** + * Depends on the state of this->args() or the current Python + * error indicator. Thus, it is not threadsafe or reentrant. + * You (you being ``green_switch``, the Python-level + * ``greenlet.switch`` method) should call + * ``check_switch_allowed`` in free-threaded builds before + * calling this method and catch ``PyErrOccurred`` if it isn't + * a valid switch. This method should also call that method + * because there are places where we can switch internally + * without going through the Python method. + */ virtual OwnedObject g_switch() = 0; /** * Force the greenlet to appear dead. Used when it's not @@ -500,6 +512,11 @@ namespace greenlet // slp_switch() failed. virtual bool force_slp_switch_error() const noexcept; + // Check the preconditions for switching to this greenlet; if they + // aren't met, throws PyErrOccurred. Most callers will want to + // catch this and clear the arguments if they've been set. + inline void check_switch_allowed() const; + protected: inline void release_args(); @@ -566,11 +583,6 @@ namespace greenlet // Returns the previous greenlet we just switched away from. virtual OwnedGreenlet g_switchstack_success() noexcept; - - // Check the preconditions for switching to this greenlet; if they - // aren't met, throws PyErrOccurred. Most callers will want to - // catch this and clear the arguments - inline void check_switch_allowed() const; class GreenletStartedWhileInPython : public std::runtime_error { public: diff --git a/src/greenlet/TUserGreenlet.cpp b/src/greenlet/TUserGreenlet.cpp index bb939e80..a8eeabb3 100644 --- a/src/greenlet/TUserGreenlet.cpp +++ b/src/greenlet/TUserGreenlet.cpp @@ -119,9 +119,16 @@ UserGreenlet::g_switch() { try { if (!this->args() && !PyErr_Occurred()) { - // we have nothing to send as the result of switching, most - // likely the result of trying to switch to a dead - // greenlet. + // we have nothing to send as the result of switching, + // most likely because we've somehow allowed concurrent + // uses of switch from multiple threads (which may or may + // not be allowed by check_switch_allowed) + // ``green_switch`` defends against this by calling + // ``check_switch_allowed`` before messing with + // ``args()``, but we have at least one internal caller + // (``throw_GreenletExit_during_dealloc``) so we keep both + // this explicit check and our call to + // ``check_switch_allowed`` throw PyErrOccurred(mod_globs->PyExc_GreenletError, "cannot switch with no pending arguments or exception"); } diff --git a/src/greenlet/greenlet_refs.hpp b/src/greenlet/greenlet_refs.hpp index 55466e00..7a87863f 100644 --- a/src/greenlet/greenlet_refs.hpp +++ b/src/greenlet/greenlet_refs.hpp @@ -446,8 +446,6 @@ namespace greenlet { void CLEAR() { Py_CLEAR(this->p); - // XXX: Have failed this assertion in free-threaded builds - // using multiple threads assert(this->p == nullptr); } }; diff --git a/tox.ini b/tox.ini index 2983e7c8..3b510703 100644 --- a/tox.ini +++ b/tox.ini @@ -1,6 +1,6 @@ [tox] envlist = - py{37,38,39,310,311,312,313,314},py{310,311,312,313,314}-ns,docs,py314t,tsan-314#,tsan-314t + py{310,311,312,313,314},docs,py314t,tsan-314,tsan-314t [testenv] commands = From 459657482f3efaee294edff672bde45ac3fac208 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Tue, 7 Apr 2026 09:42:41 -0500 Subject: [PATCH 6/6] TLBC: crash appears to still happen on CI 3.14t ubuntu. Re-enable workaround. --- .github/workflows/tests.yml | 14 ++++++++++++++ CHANGES.rst | 4 +--- docs/caveats.rst | 4 +--- 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 2651641a..cdb57409 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -114,6 +114,20 @@ jobs: python -c 'import greenlet._greenlet as G; assert G.GREENLET_USE_STANDARD_THREADING' python -m unittest discover -v greenlet.tests - name: Doctest + env: + # XXX: On 3.14t, when the thread-local bytecode cache is + # enabled, sphinx crashes on module cleanup: there is a + # reference to pdb.set_trace in ``glob``, and when the + # shutdown sequence tries to clear that value, it segfaults + # dec-reffing it after taking it out of the module dict. The + # object appears to be corrupt, but it is utterly unclear how + # we could have done this. It is 100% reliable on bath Mac and + # Linux. It can be traced down to a very simple doctest + # snippet in greenlet_gc.rst, but running that same snippet + # standalone in a unit test doesn't produce the error. + # + # So this is a temporary workaround. + PYTHON_TLBC: "0" run: | sphinx-build -b doctest -d docs/_build/doctrees2 docs docs/_build/doctest2 - name: Lint diff --git a/CHANGES.rst b/CHANGES.rst index 587175b8..eec7bb5b 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -24,9 +24,7 @@ - Fix several race conditions that could arise in free-threaded builds when using greenlet objects from multiple threads, some of - which could lead to assertion failures or interpreter crashes. On CI, - this also eliminated the need to disable the thread-local bytecode - cache. + which could lead to assertion failures or interpreter crashes. See `issue 503 `_, with diff --git a/docs/caveats.rst b/docs/caveats.rst index 1335d8ab..993b8c17 100644 --- a/docs/caveats.rst +++ b/docs/caveats.rst @@ -53,6 +53,4 @@ These include: hence the specializing interpreter) to avoid a rare crash. If your process crashes on accessing an attribute or object, or at shutdown during module cleanup, try setting the environment variable - ``PYTHON_TLBC=0`` or using the ``-X tlbc=0`` argument. (If you - encounter this with greenlet 3.4, please open an issue and let the - maintainers know.) + ``PYTHON_TLBC=0`` or using the ``-X tlbc=0`` argument.