Merge pull request #504 from python-greenlet/freethreading-fixes

Various free-threading fixes

Author: jamadden

CHANGES.rst

@@ -16,12 +16,20 @@
   <https://github.com/python-greenlet/greenlet/pull/499>`_ 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 <https://github.com/python-greenlet/greenlet/pull/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.
+
+  See `issue 503
+  <https://github.com/python-greenlet/greenlet/issues/503>`_, with
+  thanks to Nitay Dariel and Daniel Diniz.
+
 3.3.2 (2026-02-20)
 ==================
 

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:

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*
@@ -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;
@@ -489,6 +522,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 +536,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 +544,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 +568,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 +587,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 +601,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,13 +614,15 @@ 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();
 }
 
 
 static int
 green_setparent(PyGreenlet* self, PyObject* nparent, void* UNUSED(context))
 {
+    PyCriticalObjectSection cs(self);
     try {
         BorrowedGreenlet(self)->parent(nparent);
     }
@@ -598,6 +636,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 +650,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 +664,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();
 }

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;
@@ -230,18 +230,18 @@ 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.
 
-    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);
     }
 }
 

src/greenlet/TGreenlet.hpp

@@ -7,6 +7,8 @@
 #define PY_SSIZE_T_CLEAN
 #include <Python.h>
 
+#include <atomic>
+
 #include "greenlet_compiler_compat.hpp"
 #include "greenlet_refs.hpp"
 #include "greenlet_cpython_compat.hpp"
@@ -401,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
@@ -498,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();
 
@@ -564,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:
@@ -754,7 +768,7 @@ class TracingGuard
     private:
         static greenlet::PythonAllocator<MainGreenlet> allocator;
         refs::BorrowedMainGreenlet _self;
-        ThreadState* _thread_state;
+        std::atomic<ThreadState*> _thread_state;
         G_NO_COPIES_OF_CLS(MainGreenlet);
     public:
         static void* operator new(size_t UNUSED(count));

src/greenlet/TGreenletGlobals.cpp

@@ -12,6 +12,8 @@
 #ifndef T_GREENLET_GLOBALS
 #define T_GREENLET_GLOBALS
 
+#include <algorithm>
+
 #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<greenlet::cleanup_queue_t&>(this->thread_states_to_destroy);
+        greenlet::cleanup_queue_t& q = const_cast<greenlet::cleanup_queue_t&>(
+            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<greenlet::cleanup_queue_t&>(this->thread_states_to_destroy);
+        greenlet::cleanup_queue_t& q = const_cast<greenlet::cleanup_queue_t&>(
+            this->thread_states_to_destroy);
+        if (q.empty()) {
+            return nullptr;
+        }
         ThreadState* result = q.back();
         q.pop_back();
         return result;

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;
         }