From 748a9e064f42a04e208c7c32e1316b44ced18db9 Mon Sep 17 00:00:00 2001 From: Jason Madden Date: Tue, 12 Sep 2023 07:59:19 -0500 Subject: [PATCH] Fix an intermittent error during process termination on GCC/Linux/libstdc++. --- CHANGES.rst | 3 ++- setup.py | 13 +++++++--- src/greenlet/TUserGreenlet.cpp | 38 +++++++++++++++++++++++----- src/greenlet/greenlet_exceptions.hpp | 14 +++++++--- src/greenlet/greenlet_greenlet.hpp | 3 ++- 5 files changed, 54 insertions(+), 17 deletions(-) diff --git a/CHANGES.rst b/CHANGES.rst index 568d482e..08b4b8cd 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -5,7 +5,8 @@ 3.0.0rc3 (unreleased) ===================== -- Nothing changed yet. +- Fix an intermittent error during process termination on some + platforms (GCC/Linux/libstdc++). 3.0.0rc2 (2023-09-09) diff --git a/setup.py b/setup.py index b399d9ea..60a67760 100755 --- a/setup.py +++ b/setup.py @@ -67,20 +67,25 @@ cpp_compile_args.append("--std=gnu++11") elif is_win and "MSC" in plat_compiler: # Older versions of MSVC (Python 2.7) don't handle C++ exceptions - # correctly by default. While newer versions do handle exceptions by default, - # they don't do it fully correctly. So we need an argument on all versions. + # correctly by default. While newer versions do handle exceptions + # by default, they don't do it fully correctly ("By default....the + # compiler generates code that only partially supports C++ + # exceptions."). So we need an argument on all versions. + #"/EH" == exception handling. # "s" == standard C++, # "c" == extern C functions don't throw # OR # "a" == standard C++, and Windows SEH; anything may throw, compiler optimizations - # around try blocks are less aggressive. + # around try blocks are less aggressive. Because this catches SEH, + # which Windows uses internally, the MS docs say this can be a security issue. + # DO NOT USE. # /EHsc is suggested, and /EHa isn't supposed to be linked to other things not built # with it. Leaving off the "c" should just result in slower, safer code. # Other options: # "r" == Always generate standard confirming checks for noexcept blocks, terminating # if violated. IMPORTANT: We rely on this. - # See https://docs.microsoft.com/en-us/cpp/build/reference/eh-exception-handling-model?view=msvc-160 + # See https://docs.microsoft.com/en-us/cpp/build/reference/eh-exception-handling-model?view=msvc-170 handler = "/EHsr" cpp_compile_args.append(handler) # To disable most optimizations: diff --git a/src/greenlet/TUserGreenlet.cpp b/src/greenlet/TUserGreenlet.cpp index cb8b21e2..975b29cc 100644 --- a/src/greenlet/TUserGreenlet.cpp +++ b/src/greenlet/TUserGreenlet.cpp @@ -302,6 +302,8 @@ UserGreenlet::g_initialstub(void* mark) constructed for the second time until the switch actually happens. */ if (err.status == 1) { + // In the new greenlet. + // 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. @@ -314,16 +316,38 @@ UserGreenlet::g_initialstub(void* mark) // C++ extension. We're going to abort anyway, but try to // display some nice information if possible. // - // The catching is tested by ``test_cpp.CPPTests.test_unhandled_exception_in_greenlet_aborts``. + // The catching is tested by + // ``test_cpp.CPPTests.test_unhandled_exception_in_greenlet_aborts``. + // + // PyErrOccurred can theoretically be thrown by + // inner_bootstrap() -> g_switch_finish(), but that should + // never make it back to here. It is a std::exception and + // would be caught if it is. catch (const std::exception& e) { std::string base = "greenlet: Unhandled C++ exception: "; base += e.what(); Py_FatalError(base.c_str()); } catch (...) { - Py_FatalError("greenlet: inner_bootstrap terminated with unknown C++ exception\n"); + // Some compilers/runtimes use exceptions internally. + // It appears that GCC on Linux with libstdc++ throws an + // exception internally at process shutdown time to unwind + // stacks and clean up resources. Depending on exactly + // where we are when the process exits, that could result + // in an unknown exception getting here. If we + // Py_FatalError() or abort() here, we interfere with + // orderly process shutdown. Throwing the exception on up + // is the right thing to do. + // + // gevent's ``examples/dns_mass_resolve.py`` demonstrates this. +#ifndef NDEBUG + fprintf(stderr, + "greenlet: inner_bootstrap threw unknown exception; " + "is the process terminating?\n"); +#endif + throw; } - Py_FatalError("greenlet: inner_bootstrap returned\n"); + Py_FatalError("greenlet: inner_bootstrap returned with no exception.\n"); } @@ -354,7 +378,7 @@ UserGreenlet::g_initialstub(void* mark) void -GREENLET_NOINLINE(UserGreenlet::inner_bootstrap)(PyGreenlet* origin_greenlet, PyObject* run) +UserGreenlet::inner_bootstrap(PyGreenlet* origin_greenlet, PyObject* run) { // The arguments here would be another great place for move. // As it is, we take them as a reference so that when we clear @@ -435,7 +459,7 @@ GREENLET_NOINLINE(UserGreenlet::inner_bootstrap)(PyGreenlet* origin_greenlet, Py // GC, which may run arbitrary Python code. result = OwnedObject::consuming(PyObject_Call(run, args.args().borrow(), args.kwargs().borrow())); } - catch(...) { + catch (...) { // Unhandled C++ exception! // If we declare ourselves as noexcept, if we don't catch @@ -481,7 +505,7 @@ GREENLET_NOINLINE(UserGreenlet::inner_bootstrap)(PyGreenlet* origin_greenlet, Py #endif } } - // These lines may run arbitrary code + // These lines may run arbitrary code args.CLEAR(); Py_CLEAR(run); @@ -525,7 +549,7 @@ GREENLET_NOINLINE(UserGreenlet::inner_bootstrap)(PyGreenlet* origin_greenlet, Py result = parent->g_switch(); } catch (const PyErrOccurred&) { - // Ignore. + // Ignore, keep passing the error on up. } /* Return here means switch to parent failed, diff --git a/src/greenlet/greenlet_exceptions.hpp b/src/greenlet/greenlet_exceptions.hpp index 01c7039b..3807018b 100644 --- a/src/greenlet/greenlet_exceptions.hpp +++ b/src/greenlet/greenlet_exceptions.hpp @@ -18,7 +18,7 @@ namespace greenlet { public: // CAUTION: In debug builds, may run arbitrary Python code. - static PyErrOccurred + static const PyErrOccurred from_current() { assert(PyErr_Occurred()); @@ -35,12 +35,18 @@ namespace greenlet { PyObject* tb; PyErr_Fetch(&typ, &val, &tb); - PyObject* s = PyObject_Str(PyErr_Occurred()); - const char* msg = PyUnicode_AsUTF8(s); + PyObject* typs = PyObject_Str(typ); + PyObject* vals = PyObject_Str(val ? val : typ); + const char* typ_msg = PyUnicode_AsUTF8(typs); + const char* val_msg = PyUnicode_AsUTF8(vals); PyErr_Restore(typ, val, tb); + std::string msg(typ_msg); + msg += ": "; + msg += val_msg; PyErrOccurred ex(msg); - Py_XDECREF(s); + Py_XDECREF(typs); + Py_XDECREF(vals); return ex; #else diff --git a/src/greenlet/greenlet_greenlet.hpp b/src/greenlet/greenlet_greenlet.hpp index f9d0b92e..a3c2d5df 100644 --- a/src/greenlet/greenlet_greenlet.hpp +++ b/src/greenlet/greenlet_greenlet.hpp @@ -657,9 +657,10 @@ class TracingGuard protected: virtual switchstack_result_t g_initialstub(void* mark); private: + // This function isn't meant to return. // This accepts raw pointers and the ownership of them at the // same time. The caller should use ``inner_bootstrap(origin.relinquish_ownership())``. - void GREENLET_NOINLINE(inner_bootstrap)(PyGreenlet* origin_greenlet, PyObject* run); + void inner_bootstrap(PyGreenlet* origin_greenlet, PyObject* run); }; class BrokenGreenlet : public UserGreenlet