From 0d16498246f4c733f9327a0722e2b904b15b9f3a Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Mon, 31 Oct 2022 11:47:43 -0500 Subject: [PATCH] FIx 325 by more careful manual handling of a reference. --- CHANGES.rst | 3 ++- src/greenlet/greenlet.cpp | 21 ++++++++++++++++----- 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 1b6c9a3..5a8f7a0 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -5,7 +5,8 @@ 2.0.0rc5 (unreleased) ===================== -- Nothing changed yet. +- Linux: Fix another group of rare crashes that could occur when shutting down an + interpeter running multiple threads. See `issue 325 `_. 2.0.0rc4 (2022-10-30) diff --git a/src/greenlet/greenlet.cpp b/src/greenlet/greenlet.cpp index 7777e9f..5990323 100644 --- a/src/greenlet/greenlet.cpp +++ b/src/greenlet/greenlet.cpp @@ -1273,11 +1273,15 @@ UserGreenlet::g_initialstub(void* mark) constructed for the second time until the switch actually happens. */ if (err.status == 1) { - // This never returns! + // This never returns! Calling inner_bootstrap steals + // the contents of our run object within this stack frame, so + // it is not valid to do anything with it. this->inner_bootstrap(err.origin_greenlet, run); + Py_FatalError("greenlet: inner_bootstrap returned\n"); } // The child will take care of decrefing this. run.relinquish_ownership(); + // In contrast, notice that we're keeping the origin greenlet // around as an owned reference; we need it to call the trace // function for the switch back into the parent. It was only @@ -1291,17 +1295,23 @@ UserGreenlet::g_initialstub(void* mark) // XXX: This code path is not tested. this->stack_state = StackState(); this->_main_greenlet.CLEAR(); + fprintf(stderr, "greenlet: g_initialstub: starting child failed.\n"); } return err; } void -UserGreenlet::inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& run) G_NOEXCEPT_WIN32 +UserGreenlet::inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& _run) G_NOEXCEPT_WIN32 { // The arguments here would be another great place for move. // As it is, we take them as a reference so that when we clear - // them we clear what's on the stack above us. + // them we clear what's on the stack above us. Do that NOW, and + // without using a C++ RAII object, + // so there's no way that exiting the parent frame can clear it, + // or we clear it unexpectedly. This arises in the context of the + // interpreter shutting down. See https://github.com/python-greenlet/greenlet/issues/325 + PyObject* run = _run.relinquish_ownership(); /* in the new greenlet */ assert(this->thread_state()->borrow_current() == this->_self); @@ -1365,7 +1375,8 @@ UserGreenlet::inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& run) /* call g.run(*args, **kwargs) */ // This could result in further switches try { - result = run.PyCall(args.args(), args.kwargs()); + //result = run.PyCall(args.args(), args.kwargs()); + result = OwnedObject::consuming(PyObject_Call(run, args.args().borrow(), args.kwargs().borrow())); } catch(...) { // Unhandled C++ exception! @@ -1414,7 +1425,7 @@ UserGreenlet::inner_bootstrap(OwnedGreenlet& origin_greenlet, OwnedObject& run) } } args.CLEAR(); - run.CLEAR(); + Py_CLEAR(run); if (!result && mod_globs.PyExc_GreenletExit.PyExceptionMatches()