diff --git a/CHANGES.rst b/CHANGES.rst index 309c534..f0a85a4 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -2,10 +2,24 @@ Changes ========= -3.4.1 (unreleased) +3.5.0 (unreleased) ================== -- Nothing changed yet. +- Remove the ``atexit`` callback. This callback caused greenlet APIs + to become unavailable far too soon during interpreter shutdown. Now + they remain available while all ``atexit`` callbacks run. Sometime + after ``Py_IsFinalizing`` becomes true, they may begin misbehaving. + Because the order in which C extensions are finalized is undefined, + C extensions that are sensitive to this need to check the results of + that function before invoking greenlet APIs. As a convenience, + ``PyGreenlet_GetCurrent`` sets an exception and returns ``NULL`` + when this happens (and ``greenlet.getcurrent`` begins returning + ``None``); other greenlet C API functions have undefined behaviour. + Methods invoked directly on pre-existing ``greenlet.greenlet`` + objects will continue to function at least until the greenlet C + extension has been garbage collected and finalized. + + See `issue 507 `_. 3.4.0 (2026-04-08) diff --git a/docs/c_api.rst b/docs/c_api.rst index 487a08b..d5e58fe 100644 --- a/docs/c_api.rst +++ b/docs/c_api.rst @@ -30,6 +30,14 @@ Exceptions Functions ========= +.. important:: + + Because the order in which extension modules are destroyed when the + Python interpreter is finalized is undefined, it is undefined + behaviour to call these APIs when ``Py_IsFinalizing`` returns true, + unless otherwise documented. This is because the internal state of + the greenlet module may have been torn down already. + .. c:function:: void PyGreenlet_Import(void) A macro that imports the greenlet module and initializes the C API. This @@ -67,6 +75,20 @@ Functions Returns the currently active greenlet object. + If called during interpreter finalization, returns ``NULL`` + and raises a :exc:`RuntimeError`. + + .. versionchanged:: 3.4.0 + Began returning ``NULL`` during interpreter shutdown. + This implementation returned ``NULL`` too early, while the + interpreter state was still guaranteed to be valid (during + ``atexit`` handlers). This has been corrected in 3.5. + .. versionchanged:: 3.5.0 + Now sets an exception before returning ``NULL``. This prevents + a :exc:`SystemError` from being generated if this API was + exposed directly to Python, and prevents a crash if this API + was being called by Cython-generated code. + .. c:function:: PyGreenlet* PyGreenlet_New(PyObject* run, PyObject* parent) diff --git a/src/greenlet/CObjects.cpp b/src/greenlet/CObjects.cpp index a5a9921..f595e3b 100644 --- a/src/greenlet/CObjects.cpp +++ b/src/greenlet/CObjects.cpp @@ -30,6 +30,7 @@ static PyGreenlet* PyGreenlet_GetCurrent(void) { if (greenlet::IsShuttingDown()) { + PyErr_SetString(PyExc_RuntimeError, "greenlet is being finalized"); return nullptr; } return GET_THREAD_STATE().state().get_current().relinquish_ownership(); diff --git a/src/greenlet/PyModule.cpp b/src/greenlet/PyModule.cpp index c127d7c..cde2c85 100644 --- a/src/greenlet/PyModule.cpp +++ b/src/greenlet/PyModule.cpp @@ -18,19 +18,6 @@ using greenlet::ThreadState; #endif -static PyObject* -_greenlet_atexit_callback(PyObject* UNUSED(self), PyObject* UNUSED(args)) -{ - greenlet::g_greenlet_shutting_down = 1; - Py_RETURN_NONE; -} - -static PyMethodDef _greenlet_atexit_method = { - "_greenlet_cleanup", _greenlet_atexit_callback, - METH_NOARGS, NULL -}; - - PyDoc_STRVAR(mod_getcurrent_doc, "getcurrent() -> greenlet\n" "\n" diff --git a/src/greenlet/greenlet.cpp b/src/greenlet/greenlet.cpp index b9d9236..02dfa94 100644 --- a/src/greenlet/greenlet.cpp +++ b/src/greenlet/greenlet.cpp @@ -295,25 +295,6 @@ greenlet_internal_mod_init() noexcept PyUnstable_Module_SetGIL(m.borrow(), Py_MOD_GIL_NOT_USED); #endif - // Register an atexit handler that sets - // g_greenlet_shutting_down. Python's atexit is LIFO: - // registered last = called first. By registering here (at - // import time, after most other libraries), our handler runs - // before their cleanup code, which may try to call - // greenlet.getcurrent() on objects whose type has been - // invalidated. _Py_IsFinalizing() alone is insufficient on - // ALL Python versions because it is only set AFTER atexit - // handlers complete inside Py_FinalizeEx. - { - NewReference atexit_mod(Require(PyImport_ImportModule("atexit"))); - OwnedObject register_fn = atexit_mod.PyRequireAttr("register"); - NewReference callback(Require( - PyCFunction_New(&_greenlet_atexit_method, NULL))); - NewReference args(Require(PyTuple_Pack(1, callback.borrow()))); - NewReference result(Require( - PyObject_Call(register_fn.borrow(), args.borrow(), NULL))); - } - return m.borrow(); // But really it's the main reference. } catch (const LockInitError& e) { diff --git a/src/greenlet/greenlet_refs.hpp b/src/greenlet/greenlet_refs.hpp index 7a87863..9b936d6 100644 --- a/src/greenlet/greenlet_refs.hpp +++ b/src/greenlet/greenlet_refs.hpp @@ -27,25 +27,52 @@ using std::endl; namespace greenlet { class Greenlet; - // _Py_IsFinalizing() is only set AFTER atexit handlers complete - // inside Py_FinalizeEx on ALL Python versions (including 3.11+). - // Code running in atexit handlers (e.g. uWSGI plugin cleanup - // calling Py_FinalizeEx, New Relic agent shutdown) can still call - // greenlet.getcurrent(), but by that time type objects or - // internal state may have been invalidated. This flag is set by - // an atexit handler registered at module init (LIFO = runs - // first). - // - // Because this is only set from an atexit handler, by which point - // we're single threaded, there should be no need to make it - // std::atomic. - // TODO: Move this to the GreenletGlobals object? - static int g_greenlet_shutting_down; static inline bool IsShuttingDown() { - return greenlet::g_greenlet_shutting_down || Py_IsFinalizing(); + // This used to check a flag set by an ``atexit`` callback. + // This was wrong: the interpreter is still fully functional + // while *all* atexit callbacks are run, and it is perfectly + // valid for an atexit callback that runs after our atexit + // callback (i.e., registered first/before ours) to want to + // make use of greenlet services --- this comes up easily with + // gevent monkey-patching. Almost immediately after atexit callbacks, + // and before any destructive action is taken, Python arranges + // for Py_IsFinalizing to become true. + + // It may see me could potentially tighten this check even more (and + // eliminate a function call) by setting a flag in a + // destructor function for our PyCapsule object (_C_API) to + // determine when we're shutting down. ``Py_IsFinalizing`` + // becomes true relatively early in the shutdown process, + // while Capsule destructor functions only run when the module + // has actually been torn down --- well, when all of its dicts are + // cleared and collected; recall that because we use + // single-phase init, there is a "hidden" copy of the module + // dict kept by CPython internals used to re-populate a module + // if greenlet is imported twice, so Python code can't trigger + // C_API to get GC'd early without seriously poking at CPython + // internals, e.g., by using `gc.get_referrers` to find the + // hidden dict. However, C extensions could have INCREF the + // capsule object and prevent it from *ever* getting torn + // down, so this isn't reliable. + + // We could probably be even "smarter" and replace values in + // _PyGreenlet_API with different values at destruction time. + // For the PyObject* returning APIs, we could replace them + // with versions that set an exception and return null --- the + // benefit being that we don't have to include a + // Py_IsFinalizing() call in the normal path; int returning + // APIs would be handled on a case-by-case basis; unclear what + // to do with the types. This is of questionable benefit + // though because by the time our destructor is called, our + // module is about to be destroyed which may take our + // allocated storage with it (if CPython ever dynamically + // unloads loaded shared libraries, which as of 3.14 it never + // does). + + return Py_IsFinalizing(); } namespace refs diff --git a/src/greenlet/tests/_test_extension.c b/src/greenlet/tests/_test_extension.c index 91b9fa6..8e71c35 100644 --- a/src/greenlet/tests/_test_extension.c +++ b/src/greenlet/tests/_test_extension.c @@ -189,6 +189,13 @@ test_throw_exact(PyObject* UNUSED(self), PyObject* args) Py_RETURN_NONE; } +static PyObject* +getcurrent_api(PyObject* UNUSED(self)) +{ + return (PyObject*)PyGreenlet_GetCurrent(); + +} + static PyMethodDef test_methods[] = { {"test_switch", (PyCFunction)test_switch, @@ -227,6 +234,11 @@ static PyMethodDef test_methods[] = { (PyCFunction)test_throw_exact, METH_VARARGS, "Throw exactly the arguments given at the provided greenlet"}, + { + "getcurrent_api", + (PyCFunction)getcurrent_api, + METH_NOARGS, + "Direct call to the PyGreenlet_GetCurrent API."}, {NULL, NULL, 0, NULL} }; diff --git a/src/greenlet/tests/test_interpreter_shutdown.py b/src/greenlet/tests/test_interpreter_shutdown.py index 7032d5b..5ac5ab5 100644 --- a/src/greenlet/tests/test_interpreter_shutdown.py +++ b/src/greenlet/tests/test_interpreter_shutdown.py @@ -2,18 +2,6 @@ """ Tests for greenlet behavior during interpreter shutdown (Py_FinalizeEx). -During interpreter shutdown, several greenlet code paths can access -partially-destroyed Python state, leading to SIGSEGV. Two independent -guards protect against this on ALL Python versions: - - 1. g_greenlet_shutting_down — set by an atexit handler registered at - greenlet import time (LIFO = runs before other cleanup). Covers - the atexit phase of Py_FinalizeEx, where _Py_IsFinalizing() is - still False on all Python versions. - - 2. Py_IsFinalizing() — covers the GC collection and later phases of - Py_FinalizeEx, where __del__ methods and destructor code run. - These tests are organized into four groups: A. Core safety (smoke): no crashes with active greenlets at shutdown. @@ -58,14 +46,14 @@ def _run_shutdown_script(self, script_body): # ----------------------------------------------------------------- def test_active_greenlet_at_shutdown_no_crash(self): - """ - An active (suspended) greenlet that is deallocated during - interpreter shutdown should not crash the process. - Before the fix, this would SIGSEGV on Python < 3.11 because - _green_dealloc_kill_started_non_main_greenlet tried to call - g_switch() during Py_FinalizeEx. - """ + # An active (suspended) greenlet that is deallocated during + # interpreter shutdown should not crash the process. + + # Before the fix, this would SIGSEGV on Python < 3.11 because + # _green_dealloc_kill_started_non_main_greenlet tried to call + # g_switch() during Py_FinalizeEx. + rc, stdout, stderr = self._run_shutdown_script("""\ import greenlet @@ -82,10 +70,9 @@ def worker(): self.assertIn("OK: exiting with active greenlet", stdout) def test_multiple_active_greenlets_at_shutdown(self): - """ - Multiple suspended greenlets at shutdown should all be cleaned - up without crashing. - """ + # Multiple suspended greenlets at shutdown should all be cleaned + # up without crashing. + rc, stdout, stderr = self._run_shutdown_script("""\ import greenlet @@ -105,9 +92,8 @@ def worker(name): self.assertIn("OK: 10 active greenlets at shutdown", stdout) def test_nested_greenlets_at_shutdown(self): - """ - Nested (chained parent) greenlets at shutdown should not crash. - """ + # Nested (chained parent) greenlets at shutdown should not crash. + rc, stdout, stderr = self._run_shutdown_script("""\ import greenlet @@ -128,10 +114,9 @@ def outer(): self.assertIn("OK: nested greenlets at shutdown", stdout) def test_threaded_greenlets_at_shutdown(self): - """ - Greenlets in worker threads that are still referenced at - shutdown should not crash. - """ + # Greenlets in worker threads that are still referenced at + # shutdown should not crash. + rc, stdout, stderr = self._run_shutdown_script("""\ import greenlet import threading @@ -171,12 +156,11 @@ def greenlet_func(): # NOT interpreter shutdown; the guards do not fire here. def test_greenlet_cleanup_during_thread_exit(self): - """ - When a thread exits normally while holding active greenlets, - GreenletExit IS thrown and cleanup code runs. This is the - standard cleanup path used in production (e.g. uWSGI worker - threads finishing a request). - """ + # When a thread exits normally while holding active greenlets, + # GreenletExit IS thrown and cleanup code runs. This is the + # standard cleanup path used in production (e.g. uWSGI worker + # threads finishing a request). + rc, stdout, stderr = self._run_shutdown_script("""\ import os import threading @@ -208,10 +192,8 @@ def worker(_w=_write, self.assertIn("CLEANUP: GreenletExit caught", stdout) def test_finally_block_during_thread_exit(self): - """ - try/finally blocks in active greenlets run correctly when the - owning thread exits. - """ + # try/finally blocks in active greenlets run correctly when the + # owning thread exits. rc, stdout, stderr = self._run_shutdown_script("""\ import os import threading @@ -239,10 +221,9 @@ def worker(_w=_write): self.assertIn("FINALLY: cleanup executed", stdout) def test_many_greenlets_with_cleanup_at_shutdown(self): - """ - Stress test: many active greenlets with cleanup code at shutdown. - Ensures no crashes regardless of deallocation order. - """ + # Stress test: many active greenlets with cleanup code at shutdown. + # Ensures no crashes regardless of deallocation order. + rc, stdout, stderr = self._run_shutdown_script("""\ import sys import greenlet @@ -271,10 +252,9 @@ def worker(idx): self.assertIn("OK: 50 greenlets about to shut down", stdout) def test_deeply_nested_greenlets_at_shutdown(self): - """ - Deeply nested greenlet parent chains at shutdown. - Tests that the deallocation order doesn't cause issues. - """ + # Deeply nested greenlet parent chains at shutdown. + # Tests that the deallocation order doesn't cause issues. + rc, stdout, stderr = self._run_shutdown_script("""\ import greenlet @@ -292,10 +272,9 @@ def level(depth, max_depth): self.assertIn("OK: nested to depth 10", stdout) def test_greenlet_with_traceback_at_shutdown(self): - """ - A greenlet that has an active exception context when it's - suspended should not crash during shutdown cleanup. - """ + # A greenlet that has an active exception context when it's + # suspended should not crash during shutdown cleanup. + rc, stdout, stderr = self._run_shutdown_script("""\ import greenlet @@ -318,21 +297,13 @@ def worker(): # ----------------------------------------------------------------- # Group C: getcurrent() / construction / gettrace() / settrace() # during atexit — registered AFTER greenlet import - # - # These atexit handlers are registered AFTER ``import greenlet``, - # so they run BEFORE greenlet's own cleanup handler (LIFO). At - # this point g_greenlet_shutting_down is still 0 and - # _Py_IsFinalizing() is False, so getcurrent() must still return - # a valid greenlet object. These tests guard against the fix - # being too aggressive (over-blocking getcurrent early). # ----------------------------------------------------------------- def test_getcurrent_during_atexit_no_crash(self): - """ - getcurrent() in an atexit handler registered AFTER greenlet - import must return a valid greenlet (not None), because LIFO - ordering means this handler runs BEFORE greenlet's cleanup. - """ + # getcurrent() in an atexit handler registered AFTER greenlet + # import must return a valid greenlet (not None), because LIFO + # ordering means this handler runs BEFORE greenlet's cleanup. + rc, stdout, stderr = self._run_shutdown_script("""\ import atexit import greenlet @@ -359,9 +330,8 @@ def call_getcurrent_at_exit(): "before greenlet's cleanup handler (LIFO ordering)") def test_gettrace_during_atexit_no_crash(self): - """ - Calling greenlet.gettrace() during atexit must not crash. - """ + # Calling greenlet.gettrace() during atexit must not crash. + rc, stdout, stderr = self._run_shutdown_script("""\ import atexit import greenlet @@ -380,9 +350,8 @@ def check_at_exit(): self.assertIn("OK: registered", stdout) def test_settrace_during_atexit_no_crash(self): - """ - Calling greenlet.settrace() during atexit must not crash. - """ + # Calling greenlet.settrace() during atexit must not crash. + rc, stdout, stderr = self._run_shutdown_script("""\ import atexit import greenlet @@ -401,11 +370,10 @@ def check_at_exit(): self.assertIn("OK: registered", stdout) def test_getcurrent_with_active_greenlets_during_atexit(self): - """ - getcurrent() during atexit (registered after import) with active - greenlets must still return a valid greenlet, since LIFO means - this runs before greenlet's cleanup. - """ + # getcurrent() during atexit (registered after import) with active + # greenlets must still return a valid greenlet, since LIFO means + # this runs before greenlet's cleanup. + rc, stdout, stderr = self._run_shutdown_script("""\ import atexit import greenlet @@ -441,10 +409,9 @@ def check_at_exit(): "before greenlet's cleanup handler (LIFO ordering)") def test_greenlet_construction_during_atexit_no_crash(self): - """ - Constructing a new greenlet during atexit (registered after - import) must succeed, since this runs before greenlet's cleanup. - """ + # Constructing a new greenlet during atexit (registered after + # import) must succeed, since this runs before greenlet's cleanup. + rc, stdout, stderr = self._run_shutdown_script("""\ import atexit import greenlet @@ -469,11 +436,10 @@ def noop(): self.assertIn("OK: created greenlet successfully", stdout) def test_greenlet_construction_with_active_greenlets_during_atexit(self): - """ - Constructing new greenlets during atexit when other active - greenlets already exist (maximizes the chance of a non-empty - deleteme list). - """ + # Constructing new greenlets during atexit when other active + # greenlets already exist (maximizes the chance of a non-empty + # deleteme list). + rc, stdout, stderr = self._run_shutdown_script("""\ import atexit import greenlet @@ -504,14 +470,12 @@ def create_at_exit(): self.assertIn("OK: 10 active greenlets, atexit registered", stdout) def test_greenlet_construction_with_cross_thread_deleteme_during_atexit(self): - """ - Create greenlets in a worker thread, transfer them to the main - thread, then drop them — populating the deleteme list. Then - construct a new greenlet during atexit. On Python < 3.11 - clear_deleteme_list() could previously crash if the - PythonAllocator vector copy failed during early Py_FinalizeEx; - using std::swap eliminates that allocation. - """ + # Create greenlets in a worker thread, transfer them to the main + # thread, then drop them — populating the deleteme list. Then + # construct a new greenlet during atexit. On Python < 3.11 + # clear_deleteme_list() could previously crash if the + # PythonAllocator vector copy failed during early Py_FinalizeEx; + # using std::swap eliminates that allocation. rc, stdout, stderr = self._run_shutdown_script("""\ import atexit import greenlet @@ -568,15 +532,13 @@ def create_at_exit(): # ----------------------------------------------------------------- def test_getcurrent_returns_none_during_gc_finalization(self): - """ - greenlet.getcurrent() must return None when called from a - __del__ method during Py_FinalizeEx's GC collection pass. + # greenlet.getcurrent() must return None when called from a + # __del__ method during Py_FinalizeEx's GC collection pass. - On Python >= 3.11, _Py_IsFinalizing() is True during this - phase. Without the Py_IsFinalizing() guard in mod_getcurrent, - this would return a greenlet — the same unguarded code path - that leads to SIGSEGV in production (uWSGI worker recycling). - """ + # On Python >= 3.11, _Py_IsFinalizing() is True during this + # phase. Without the Py_IsFinalizing() guard in mod_getcurrent, + # this would return a greenlet — the same unguarded code path + # that leads to SIGSEGV in production (uWSGI worker recycling). rc, stdout, stderr = self._run_shutdown_script("""\ import gc import os @@ -611,10 +573,9 @@ def __del__(self): "returned a live object instead (missing Py_IsFinalizing guard)") def test_getcurrent_returns_none_during_gc_finalization_with_active_greenlets(self): - """ - Same as above but with active greenlets at shutdown, which - increases the amount of C++ destructor work during finalization. - """ + # Same as above but with active greenlets at shutdown, which + # increases the amount of C++ destructor work during finalization. + rc, stdout, stderr = self._run_shutdown_script("""\ import gc import os @@ -658,13 +619,12 @@ def worker(): "returned a live object instead (missing Py_IsFinalizing guard)") def test_getcurrent_returns_none_during_gc_finalization_cross_thread(self): - """ - Combines cross-thread greenlet deallocation (deleteme list) - with the GC finalization check. This simulates the production - scenario where uWSGI worker threads create greenlets that are - transferred to the main thread, then cleaned up during - Py_FinalizeEx. - """ + # Combines cross-thread greenlet deallocation (deleteme list) + # with the GC finalization check. This simulates the production + # scenario where uWSGI worker threads create greenlets that are + # transferred to the main thread, then cleaned up during + # Py_FinalizeEx. + rc, stdout, stderr = self._run_shutdown_script("""\ import gc import os @@ -735,15 +695,9 @@ def body(): # ----------------------------------------------------------------- def test_getcurrent_returns_none_during_atexit_phase(self): - """ - greenlet.getcurrent() must return None when called from an - atexit handler that runs AFTER greenlet's own atexit handler. + # greenlet.getcurrent() must NOT return None when called from an + # atexit handler that runs AFTER greenlet's own atexit handler. - This tests the g_greenlet_shutting_down flag, which is needed - because _Py_IsFinalizing() is still False during the atexit - phase on ALL Python versions. Without g_greenlet_shutting_down, - getcurrent() proceeds unguarded into partially-torn-down state. - """ rc, stdout, stderr = self._run_shutdown_script("""\ import atexit import os @@ -770,16 +724,11 @@ def late_checker(): """) self.assertEqual(rc, 0, f"Process crashed (rc={rc}):\n{stdout}{stderr}") self.assertIn("OK: atexit registered before greenlet import", stdout) - self.assertIn("GUARDED: getcurrent=None", stdout, - "getcurrent() must return None during atexit phase; " - "returned a live object instead (missing " - "g_greenlet_shutting_down atexit handler)") + self.assertIn("UNGUARDED", stdout) + def test_getcurrent_returns_none_during_atexit_phase_with_active_greenlets(self): - """ - Same as above but with active greenlets, ensuring the atexit - guard works even when there is greenlet state to clean up. - """ + # Same as above but with active greenlets rc, stdout, stderr = self._run_shutdown_script("""\ import atexit import os @@ -812,10 +761,63 @@ def worker(): """) self.assertEqual(rc, 0, f"Process crashed (rc={rc}):\n{stdout}{stderr}") self.assertIn("OK: 10 active greenlets, atexit registered", stdout) - self.assertIn("GUARDED: getcurrent=None", stdout, - "getcurrent() must return None during atexit phase; " - "returned a live object instead (missing " - "g_greenlet_shutting_down atexit handler)") + self.assertIn("UNGUARDED", stdout) + + def test_api_getcurrent_no_system_error_at_module_gc_time(self): + # If we use the C API directly to return a greenlet AFTER + # atexit threads have been run, we don't crash, we get a + # specific error. We arrange for this by putting a __del__ on + # an object that lives in greenlet's own (extension module) + # dict; this is cleaned out sometime during the module cleanup + # steps. + rc, stdout, stderr = self._run_shutdown_script("""\ + import greenlet + from greenlet.tests import _test_extension + + class WithDel: + # must cache the method we want, because by the time we + # run, module globals may have been cleaned up. + def __del__(self, gc=_test_extension.getcurrent_api): + print('Destructor running') + gc() # Should print an unraisable RuntimeException + + greenlet._greenlet.with_del = WithDel() + """) + self.assertEqual(rc, 0, f"Process crashed (rc={rc}):\n{stdout}{stderr}") + self.assertIn('Destructor running', stdout) + self.assertIn('RuntimeError: greenlet is being finalized', stderr) + + + def test_switch_no_error_at_module_gc_time(self): + # Switching to a greenlet we've captured during + # module tear down doesn't cause a crash + rc, stdout, stderr = self._run_shutdown_script("""\ + import greenlet + from greenlet.tests import _test_extension + + gs = [] + # must cache the objects we want, because by the time we + # run, module globals may have been cleaned up. + def do_it(gs=gs): + print('current', gs) + gs[0].parent.switch(1) + + + gs.append(greenlet.greenlet(do_it)) + gs.append(greenlet.greenlet(do_it)) + gs[1].switch() + + class WithDel: + def __del__(self, gs=gs): + print('Destructor running') + r = gs[0].switch() + print('Result', r) + + greenlet._greenlet.with_del = WithDel() + """) + self.assertEqual(rc, 0, f"Process crashed (rc={rc}):\n{stdout}{stderr}") + self.assertIn('Destructor running', stdout) + self.assertIn('Result 1', stdout) if __name__ == '__main__':