diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index 71afd61..759dd30 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -21,6 +21,7 @@ jobs: test: runs-on: ${{ matrix.os }} strategy: + fail-fast: false matrix: python-version: - "3.10" @@ -187,6 +188,7 @@ jobs: runs-on: ubuntu-latest # We use a regular Python matrix entry to share as much code as possible. strategy: + fail-fast: false matrix: python-version: - "3.14" diff --git a/src/greenlet/PyGreenlet.cpp b/src/greenlet/PyGreenlet.cpp index b022d5c..b54b742 100644 --- a/src/greenlet/PyGreenlet.cpp +++ b/src/greenlet/PyGreenlet.cpp @@ -808,7 +808,7 @@ PyTypeObject PyGreenlet_Type = { .tp_alloc=PyType_GenericAlloc, /* tp_alloc */ .tp_new=(newfunc)green_new, /* tp_new */ .tp_free=PyObject_GC_Del, /* tp_free */ -#ifndef Py_GIL_DISABLED +#if !GREENLET_PY315 && !(GREENLET_PY314 && defined(Py_GIL_DISABLED)) /* We may have been handling this wrong all along. diff --git a/src/greenlet/TGreenlet.cpp b/src/greenlet/TGreenlet.cpp index 7ec86f9..dcbdc00 100644 --- a/src/greenlet/TGreenlet.cpp +++ b/src/greenlet/TGreenlet.cpp @@ -607,6 +607,10 @@ Greenlet::tp_clear() bool own_top_frame = this->was_running_in_dead_thread(); this->exception_state.tp_clear(); this->python_state.tp_clear(own_top_frame); + if (own_top_frame) { + // Throw away any saved stack state since the owned frame is cleared. + this->stack_state.set_inactive(); + } return 0; } diff --git a/src/greenlet/TPythonState.cpp b/src/greenlet/TPythonState.cpp index cf105fc..9c98d14 100644 --- a/src/greenlet/TPythonState.cpp +++ b/src/greenlet/TPythonState.cpp @@ -397,7 +397,36 @@ void PythonState::tp_clear(bool own_top_frame) noexcept // we got dealloc'd without being finished. We may or may not be // in the same thread. if (own_top_frame) { +#if GREENLET_PY315 + // Release the references held by our suspended frames. + // this->top_frame gets implicitly cleared by the Py_CLEAR(iframe->frame_obj) + // of the first complete frame, so in the end we relinquish ownership of it. + if (this->_top_frame) { + for (_PyInterpreterFrame* iframe = this->_top_frame->f_frame; + iframe != nullptr; iframe = iframe->previous) { + if (iframe->owner != FRAME_OWNED_BY_THREAD) { + continue; + } + // Clear the references held by this frame's evaluation stack. + _PyStackRef* locals = iframe->localsplus; + _PyStackRef* sp = iframe->stackpointer; + if (sp) { + while (sp > locals) { + sp--; + PyStackRef_CLEAR(*sp); + } + iframe->stackpointer = locals; + } + Py_CLEAR(iframe->f_locals); + Py_CLEAR(iframe->frame_obj); + PyStackRef_CLEAR(iframe->f_funcobj); + PyStackRef_CLEAR(iframe->f_executable); + } + } + this->_top_frame.relinquish_ownership(); +#else this->_top_frame.CLEAR(); +#endif } } diff --git a/src/greenlet/TUserGreenlet.cpp b/src/greenlet/TUserGreenlet.cpp index a8eeabb..ce011fd 100644 --- a/src/greenlet/TUserGreenlet.cpp +++ b/src/greenlet/TUserGreenlet.cpp @@ -422,11 +422,13 @@ UserGreenlet::inner_bootstrap(PyGreenlet* origin_greenlet, PyObject* run) args <<= this->args(); assert(!this->args()); - // XXX: We could clear this much earlier, right? - // Or would that introduce the possibility of running Python - // code when we don't want to? - // CAUTION: This may run arbitrary Python code. this->_run_callable.CLEAR(); + // stash the run callable in this->_run_callable to ensure that GC will be + // able to find the object later. + // This is needed for the case of a permanently suspended greenlet + // so that the run callable is not leaked. + this->_run_callable.steal(run); + run = nullptr; // The first switch we need to manually call the trace @@ -467,7 +469,7 @@ UserGreenlet::inner_bootstrap(PyGreenlet* origin_greenlet, PyObject* run) // CAUTION: Just invoking this, before the function even // runs, may cause memory allocations, which may trigger // GC, which may run arbitrary Python code. - result = OwnedObject::consuming(PyObject_Call(run, args.args().borrow(), args.kwargs().borrow())); + result = OwnedObject::consuming(PyObject_Call(this->_run_callable.borrow(), args.args().borrow(), args.kwargs().borrow())); } catch (...) { // Unhandled C++ exception! @@ -517,7 +519,8 @@ UserGreenlet::inner_bootstrap(PyGreenlet* origin_greenlet, PyObject* run) } // These lines may run arbitrary code args.CLEAR(); - Py_CLEAR(run); + assert(run == nullptr); + this->_run_callable.CLEAR(); if (!result && mod_globs->PyExc_GreenletExit.PyExceptionMatches() diff --git a/src/greenlet/greenlet_msvc_compat.hpp b/src/greenlet/greenlet_msvc_compat.hpp index fa25d71..08fca47 100644 --- a/src/greenlet/greenlet_msvc_compat.hpp +++ b/src/greenlet/greenlet_msvc_compat.hpp @@ -85,6 +85,27 @@ PyStackRef_AsPyObjectBorrow(_PyStackRef stackref) return cleared; } +#define Py_TAG_REFCNT 1 +#define BITS_TO_PTR(ref) ((PyObject *)((ref).bits)) + +#define PyStackRef_RefcountOnObject(ref) (((ref).bits & Py_TAG_REFCNT) == 0) + +#define PyStackRef_CLOSE(REF) \ + do { \ + _PyStackRef _close_tmp = (REF); \ + if (PyStackRef_RefcountOnObject(_close_tmp)) { \ + Py_DECREF(BITS_TO_PTR(_close_tmp)); \ + } \ + } while (0) + +#define PyStackRef_CLEAR(REF) \ + do { \ + _PyStackRef* _clear_ptr = &(REF); \ + _PyStackRef _clear_old = (*_clear_ptr); \ + *_clear_ptr = PyStackRef_NULL; \ + PyStackRef_CLOSE(_clear_old); \ + } while (0) + static inline PyCodeObject *_PyFrame_GetCode(_PyInterpreterFrame *f) { assert(!PyStackRef_IsNullOrInt(f->f_executable)); PyObject *executable = PyStackRef_AsPyObjectBorrow(f->f_executable); diff --git a/src/greenlet/tests/leakcheck.py b/src/greenlet/tests/leakcheck.py index 993e3fa..a1037d4 100644 --- a/src/greenlet/tests/leakcheck.py +++ b/src/greenlet/tests/leakcheck.py @@ -86,6 +86,21 @@ class and specify variants of behaviour (such as pool sizes). func.ignore_leakcheck = True return func +def ignores_leakcheck_if(condition, message): + """ + Return a decorator that marks the function to be ignored during + leakchecks (see `ignores_leakcheck`) when *condition* is true. + + *message* describes why the leakcheck is ignored. When *condition* + is false, the function is returned unchanged. + """ + def decorator(func): + if condition: + func = ignores_leakcheck(func) + func.ignore_leakcheck_reason = message + return func + return decorator + def fails_leakcheck(func): """ Mark that the function is known to leak. @@ -95,6 +110,14 @@ def fails_leakcheck(func): func = unittest.skip("Skipping known failures")(func) return func +def fails_leakcheck_on_py314_or_less(func): + """ + Mark the function as known to leak (fails refcount leakchecks) on Python 3.14 or less. + """ + if sys.version_info[:2] <= (3, 14): + return fails_leakcheck(func) + return func + class LeakCheckError(AssertionError): pass @@ -321,6 +344,12 @@ def __call__(self, args, kwargs): def wrap_refcount(method): if getattr(method, 'ignore_leakcheck', False) or SKIP_LEAKCHECKS: + reason = getattr(method, 'ignore_leakcheck_reason', None) + if reason and not SKIP_LEAKCHECKS: + print( + "Ignoring leakchecks for %s: %s" % (method.__name__, reason), + file=sys.stderr, + ) return method @wraps(method) diff --git a/src/greenlet/tests/test_gc.py b/src/greenlet/tests/test_gc.py index b981bc7..340fdbe 100644 --- a/src/greenlet/tests/test_gc.py +++ b/src/greenlet/tests/test_gc.py @@ -6,7 +6,7 @@ from . import TestCase -from .leakcheck import fails_leakcheck +from .leakcheck import fails_leakcheck_on_py314_or_less # These only work with greenlet gc support # which is no longer optional. assert greenlet.GREENLET_USE_GC @@ -43,7 +43,7 @@ def run(self): self.assertIsNone(o()) self.assertFalse(gc.garbage, gc.garbage) - @fails_leakcheck + @fails_leakcheck_on_py314_or_less def test_finalizer_crash(self): # This test is designed to crash when active greenlets # are made garbage collectable, until the underlying diff --git a/src/greenlet/tests/test_greenlet.py b/src/greenlet/tests/test_greenlet.py index 5e18451..36d5d1e 100644 --- a/src/greenlet/tests/test_greenlet.py +++ b/src/greenlet/tests/test_greenlet.py @@ -13,9 +13,12 @@ from . import RUNNING_ON_MANYLINUX from . import PY313 from . import PY314 +from . import WIN from . import RUNNING_ON_FREETHREAD_BUILD from .leakcheck import fails_leakcheck +from .leakcheck import fails_leakcheck_on_py314_or_less from .leakcheck import ignores_leakcheck +from .leakcheck import ignores_leakcheck_if # We manually manage locks in many tests @@ -506,7 +509,7 @@ def __getattribute__(self, name): g = mygreenlet(lambda: None) self.assertRaises(SomeError, g.throw, SomeError()) - @fails_leakcheck + @fails_leakcheck_on_py314_or_less def _do_test_throw_to_dead_thread_doesnt_crash(self, wait_for_cleanup=False): result = [] def worker(): @@ -564,7 +567,11 @@ def creator(): # See issue 252 self.expect_greenlet_leak = True # direct us not to wait for it to go away - @fails_leakcheck + @fails_leakcheck_on_py314_or_less + @ignores_leakcheck_if( + WIN and sys.version_info[:2] == (3, 10), + "does not leaks on Windows 3.10, but does on other platforms" + ) def test_throw_to_dead_thread_doesnt_crash(self): self._do_test_throw_to_dead_thread_doesnt_crash() @@ -1301,6 +1308,15 @@ def test_main_greenlet_is_greenlet(self): self._check_current_is_main() self.assertIsInstance(greenlet.getcurrent(), RawGreenlet) + def test_greenlet_run(self): + def do_it(): + with self.assertRaises(AttributeError): + getattr(g, 'run') + + g = greenlet.greenlet(do_it) + g.switch() + with self.assertRaises(AttributeError): + getattr(g, 'run') class TestBrokenGreenlets(TestCase):