error_already_set::what() is now constructed lazily

include/pybind11/detail/internals.h

@@ -410,11 +410,7 @@ PYBIND11_NOINLINE internals &get_internals() {
 
     // Ensure that the GIL is held since we will need to make Python calls.
     // Cannot use py::gil_scoped_acquire here since that constructor calls get_internals.
-    struct gil_scoped_acquire_local {
-        gil_scoped_acquire_local() : state(PyGILState_Ensure()) {}
-        ~gil_scoped_acquire_local() { PyGILState_Release(state); }
-        const PyGILState_STATE state;
-    } gil;
+    gil_scoped_acquire_simple gil;
 
     PYBIND11_STR_TYPE id(PYBIND11_INTERNALS_ID);
     auto builtins = handle(PyEval_GetBuiltins());

include/pybind11/detail/type_caster_base.h

@@ -470,41 +470,37 @@ PYBIND11_NOINLINE bool isinstance_generic(handle obj, const std::type_info &tp)
     return isinstance(obj, type);
 }
 
-PYBIND11_NOINLINE std::string error_string() {
-    if (!PyErr_Occurred()) {
-        PyErr_SetString(PyExc_RuntimeError, "Unknown internal error occurred");
-        return "Unknown internal error occurred";
+// NOTE: This function may have the side-effect of normalizing the passed Python exception
+//       (if it is not normalized already), which will change some or all of the three passed
+//       pointers.
+PYBIND11_NOINLINE std::string
+error_string(PyObject *&exc_type, PyObject *&exc_value, PyObject *&exc_trace) {
+    if (exc_type == nullptr) {
+        pybind11_fail(
+            "Internal error: pybind11::detail::error_string() called with exc_type == nullptr");
     }
 
-    error_scope scope; // Preserve error state
-
-    std::string errorString;
-    if (scope.type) {
-        errorString += handle(scope.type).attr("__name__").cast<std::string>();
-        errorString += ": ";
-    }
-    if (scope.value) {
-        errorString += (std::string) str(scope.value);
-    }
+    PyErr_NormalizeException(&exc_type, &exc_value, &exc_trace);
 
-    PyErr_NormalizeException(&scope.type, &scope.value, &scope.trace);
+    auto result = handle(exc_type).attr("__name__").cast<std::string>();
+    result += ": ";
 
-    if (scope.trace != nullptr) {
-        PyException_SetTraceback(scope.value, scope.trace);
+    if (exc_value) {
+        result += str(exc_value).cast<std::string>();
     }
 
+    if (exc_trace) {
 #if !defined(PYPY_VERSION)
-    if (scope.trace) {
-        auto *trace = (PyTracebackObject *) scope.trace;
+        auto *tb = reinterpret_cast<PyTracebackObject *>(exc_trace);
 
-        /* Get the deepest trace possible */
-        while (trace->tb_next) {
-            trace = trace->tb_next;
+        // Get the deepest trace possible.
+        while (tb->tb_next) {
+            tb = tb->tb_next;
         }
 
-        PyFrameObject *frame = trace->tb_frame;
+        PyFrameObject *frame = tb->tb_frame;
         Py_XINCREF(frame);
-        errorString += "\n\nAt:\n";
+        result += "\n\nAt:\n";
         while (frame) {
 #    if PY_VERSION_HEX >= 0x030900B1
             PyCodeObject *f_code = PyFrame_GetCode(frame);
@@ -513,13 +509,13 @@ PYBIND11_NOINLINE std::string error_string() {
             Py_INCREF(f_code);
 #    endif
             int lineno = PyFrame_GetLineNumber(frame);
-            errorString += "  ";
-            errorString += handle(f_code->co_filename).cast<std::string>();
-            errorString += '(';
-            errorString += std::to_string(lineno);
-            errorString += "): ";
-            errorString += handle(f_code->co_name).cast<std::string>();
-            errorString += '\n';
+            result += "  ";
+            result += handle(f_code->co_filename).cast<std::string>();
+            result += '(';
+            result += std::to_string(lineno);
+            result += "): ";
+            result += handle(f_code->co_name).cast<std::string>();
+            result += '\n';
             Py_DECREF(f_code);
 #    if PY_VERSION_HEX >= 0x030900B1
             auto *b_frame = PyFrame_GetBack(frame);
@@ -530,10 +526,17 @@ PYBIND11_NOINLINE std::string error_string() {
             Py_DECREF(frame);
             frame = b_frame;
         }
+#endif //! defined(PYPY_VERSION)
     }
-#endif
 
-    return errorString;
+    return result;
+}
+
+// NOTE: This function may have the side-effect of normalizing the current Python exception
+//       (if it is not normalized already).
+PYBIND11_NOINLINE std::string error_string() {
+    error_scope scope; // Fetch error state.
+    return error_string(scope.type, scope.value, scope.trace);
 }
 
 PYBIND11_NOINLINE handle get_object_handle(const void *ptr, const detail::type_info *type) {

include/pybind11/pybind11.h

@@ -2612,13 +2612,28 @@ void print(Args &&...args) {
 }
 
 error_already_set::~error_already_set() {
-    if (m_type) {
-        gil_scoped_acquire gil;
-        error_scope scope;
-        m_type.release().dec_ref();
-        m_value.release().dec_ref();
-        m_trace.release().dec_ref();
+    if (!(m_type || m_value || m_trace)) {
+        return; // Avoid gil and scope overhead if there is nothing to release.
+    }
+#if PY_VERSION_HEX >= 0x03070000
+    if (PyGILState_Check() == 0 && _Py_IsFinalizing() != 0) {
+        // Leak m_type, m_value, m_trace rather than crashing the process.
+        // https://docs.python.org/3/c-api/init.html#c.PyGILState_Ensure
+        return;
     }
+#endif
+    // Not using py::gil_scoped_acquire here since that calls get_internals,
+    // which is known to trigger failures in the wild (a full explanation is
+    // currently unknown).
+    // Note that py::gil_scoped_acquire acquires the GIL first in detail/internals.h
+    // exactly like we do here now, releases it, then acquires it again in gil.h.
+    // Using gil_scoped_acquire_simple cuts out the get_internals overhead and
+    // fixes the failures observed in the wild. See PR #1895 for more background.
+    detail::gil_scoped_acquire_simple gil;
+    error_scope scope;
+    m_type.release().dec_ref();
+    m_value.release().dec_ref();
+    m_trace.release().dec_ref();
 }
 
 PYBIND11_NAMESPACE_BEGIN(detail)

include/pybind11/pytypes.h

@@ -12,6 +12,9 @@
 #include "detail/common.h"
 #include "buffer_info.h"
 
+#include <cstdio>
+#include <exception>
+#include <string>
 #include <type_traits>
 #include <utility>
 
@@ -361,6 +364,27 @@ T reinterpret_steal(handle h) {
 
 PYBIND11_NAMESPACE_BEGIN(detail)
 std::string error_string();
+std::string error_string(PyObject *&, PyObject *&, PyObject *&);
+
+// Equivalent to obj.__class__.__name__ (or obj.__name__ if obj is a class).
+inline const char *obj_class_name(PyObject *obj) {
+    if (Py_TYPE(obj) == &PyType_Type) {
+        return reinterpret_cast<PyTypeObject *>(obj)->tp_name;
+    }
+    return Py_TYPE(obj)->tp_name;
+}
+
+// For situations in which the more complex gil_scoped_acquire cannot be used.
+// Note that gil_scoped_acquire calls get_internals(), which uses gil_scoped_acquire_simple.
+class gil_scoped_acquire_simple {
+public:
+    gil_scoped_acquire_simple() : state(PyGILState_Ensure()) {}
+    ~gil_scoped_acquire_simple() { PyGILState_Release(state); }
+
+private:
+    const PyGILState_STATE state;
+};
+
 PYBIND11_NAMESPACE_END(detail)
 
 #if defined(_MSC_VER)
@@ -373,38 +397,156 @@ PYBIND11_NAMESPACE_END(detail)
 /// thrown to propagate python-side errors back through C++ which can either be caught manually or
 /// else falls back to the function dispatcher (which then raises the captured error back to
 /// python).
-class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::runtime_error {
+class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::exception {
 public:
-    /// Constructs a new exception from the current Python error indicator, if any.  The current
-    /// Python error indicator will be cleared.
-    error_already_set() : std::runtime_error(detail::error_string()) {
+    /// Fetches the current Python exception (using PyErr_Fetch()), which will clear the
+    /// current Python error indicator.
+    error_already_set() {
         PyErr_Fetch(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr());
+        if (!m_type) {
+            pybind11_fail("Internal error: pybind11::detail::error_already_set called while "
+                          "Python error indicator not set.");
+        }
     }
 
-    error_already_set(const error_already_set &) = default;
-    error_already_set(error_already_set &&) = default;
+#if ((defined(__clang__) && __clang_major__ >= 9) || (defined(__GNUC__) && __GNUC__ >= 10)        \
+     || (defined(_MSC_VER) && _MSC_VER >= 1920))                                                  \
+    && !defined(__INTEL_COMPILER)
+    /// WARNING: The GIL must be held when the copy ctor is used!
+    error_already_set(const error_already_set &) noexcept = default;
+    error_already_set(error_already_set &&) noexcept = default;
+#else
+    /// Workaround for old compilers:
+    /// Copying/moving the members one-by-one to be able to specify noexcept.
+    error_already_set(const error_already_set &e) noexcept
+        : std::exception{e}, m_type{e.m_type}, m_value{e.m_value}, m_trace{e.m_trace},
+          m_lazy_what{e.m_lazy_what} {};
+    error_already_set(error_already_set &&e) noexcept
+        : std::exception{std::move(e)}, m_type{std::move(e.m_type)}, m_value{std::move(e.m_value)},
+          m_trace{std::move(e.m_trace)}, m_lazy_what{std::move(e.m_lazy_what)} {};
+#endif
 
+    // Note that the dtor acquires the GIL, unless the Python interpreter is finalizing (in which
+    // case the Python exception is leaked, to not crash the process).
     inline ~error_already_set() override;
 
-    /// Give the currently-held error back to Python, if any.  If there is currently a Python error
-    /// already set it is cleared first.  After this call, the current object no longer stores the
-    /// error variables (but the `.what()` string is still available).
+    /// The what() result is built lazily on demand. To build the result, it is necessary to
+    /// acquire the Python GIL. If that is not possible because the Python interpreter is
+    /// finalizing, the Python exception is unrecoverable and a static message is returned. Any
+    /// other errors processing the Python exception lead to process termination. If possible, the
+    /// original Python exception is written to stderr & stdout before the process is terminated.
+    /// NOTE: This member function may have the side-effect of normalizing the held Python
+    ///       exception (if it is not normalized already).
+    const char *what() const noexcept override {
+        if (m_lazy_what.empty()) {
+#if PY_VERSION_HEX >= 0x03070000
+            if (PyGILState_Check() == 0 && _Py_IsFinalizing() != 0) {
+                // At this point there is no way the original Python exception can still be
+                // reported, therefore it is best to let the shutdown continue.
+                return "Python exception is UNRECOVERABLE because the Python interpreter is "
+                       "finalizing.";
+            }
+#endif
+            std::string failure_info;
+            detail::gil_scoped_acquire_simple gil;
+            try {
+                m_lazy_what = detail::error_string(m_type.ptr(), m_value.ptr(), m_trace.ptr());
+                if (m_lazy_what.empty()) { // Negate condition for manual testing.
+                    failure_info = "m_lazy_what.empty()";
+                }
+                // throw std::runtime_error("Uncomment for manual testing.");
+#ifdef _MSC_VER
+            } catch (const std::exception &e) {
+                failure_info = "std::exception::what(): ";
+                try {
+                    failure_info += e.what();
+                } catch (...) {
+                    failure_info += "UNRECOVERABLE";
+                }
+#endif
+            } catch (...) {
+#ifdef _MSC_VER
+                failure_info = "Unknown C++ exception";
+#else
+                failure_info = "C++ exception"; // std::terminate will report the details.
+#endif
+            }
+            if (!failure_info.empty()) {
+                // Terminating the process, to not mask the original error by errors in the error
+                // handling. Reporting the original error on stderr & stdout. Intentionally using
+                // the Python C API directly, to maximize reliability.
+                std::string msg
+                    = "FATAL failure building pybind11::detail::error_already_set what() ["
+                      + failure_info + "] while processing Python exception: ";
+                if (m_type.ptr() == nullptr) {
+                    msg += "PYTHON_EXCEPTION_TYPE_IS_NULLPTR";
+                } else {
+                    const char *class_name = detail::obj_class_name(m_type.ptr());
+                    if (class_name == nullptr) {
+                        msg += "PYTHON_EXCEPTION_CLASS_NAME_IS_NULLPTR";
+                    } else {
+                        msg += class_name;
+                    }
+                }
+                msg += ": ";
+                PyObject *val_str = PyObject_Str(m_value.ptr());
+                if (val_str == nullptr) {
+                    msg += "PYTHON_EXCEPTION_VALUE_IS_NULLPTR";
+                } else {
+                    Py_ssize_t utf8_str_size = 0;
+                    const char *utf8_str = PyUnicode_AsUTF8AndSize(val_str, &utf8_str_size);
+                    if (utf8_str == nullptr) {
+                        msg += "PYTHON_EXCEPTION_VALUE_AS_UTF8_FAILURE";
+                    } else {
+                        msg += '"' + std::string(utf8_str, static_cast<std::size_t>(utf8_str_size))
+                               + '"';
+                    }
+                }
+                // Intentionally using C calls to maximize reliability
+                // (and to avoid #include <iostream>).
+                fprintf(stderr, "\n%s [STDERR]\n", msg.c_str());
+                fflush(stderr);
+                fprintf(stdout, "\n%s [STDOUT]\n", msg.c_str());
+                fflush(stdout);
+#ifdef _MSC_VER
+                exit(-1); // Sadly. std::terminate() may pop up an interactive dialog box.
+#else
+                std::terminate();
+#endif
+            }
+        }
+        return m_lazy_what.c_str();
+    }
+
+    /// Restores the currently-held Python error (which will clear the Python error indicator first
+    /// if already set).
+    /// WARNING: The GIL must be held when this member function is called!
+    /// NOTE: This member function will not necessarily restore the original Python exception, but
+    ///       may restore the normalized exception if what() or discard_as_unraisable() were called
+    ///       prior to restore().
     void restore() {
-        PyErr_Restore(m_type.release().ptr(), m_value.release().ptr(), m_trace.release().ptr());
+        // As long as this type is copyable, there is no point in releasing m_type, m_value,
+        // m_trace, but simply holding on the the references makes it possible to produce
+        // what() even after restore().
+        PyErr_Restore(m_type.inc_ref().ptr(), m_value.inc_ref().ptr(), m_trace.inc_ref().ptr());
     }
 
     /// If it is impossible to raise the currently-held error, such as in a destructor, we can
     /// write it out using Python's unraisable hook (`sys.unraisablehook`). The error context
     /// should be some object whose `repr()` helps identify the location of the error. Python
-    /// already knows the type and value of the error, so there is no need to repeat that. After
-    /// this call, the current object no longer stores the error variables, and neither does
-    /// Python.
+    /// already knows the type and value of the error, so there is no need to repeat that.
+    /// NOTE: This member function may have the side-effect of normalizing the held Python
+    ///       exception (if it is not normalized already).
     void discard_as_unraisable(object err_context) {
+#if PY_VERSION_HEX < 0x03080000
+        PyErr_NormalizeException(&m_type.ptr(), &m_value.ptr(), &m_trace.ptr());
+#endif
         restore();
         PyErr_WriteUnraisable(err_context.ptr());
     }
     /// An alternate version of `discard_as_unraisable()`, where a string provides information on
     /// the location of the error. For example, `__func__` could be helpful.
+    /// WARNING: The GIL must be held when this member function is called!
     void discard_as_unraisable(const char *err_context) {
         discard_as_unraisable(reinterpret_steal<object>(PYBIND11_FROM_STRING(err_context)));
     }
@@ -425,7 +567,8 @@ class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::runtime_error {
     const object &trace() const { return m_trace; }
 
 private:
-    object m_type, m_value, m_trace;
+    mutable object m_type, m_value, m_trace;
+    mutable std::string m_lazy_what;
 };
 #if defined(_MSC_VER)
 #    pragma warning(pop)
@@ -461,8 +604,7 @@ inline void raise_from(PyObject *type, const char *message) {
 
 /// Sets the current Python error indicator with the chosen error, performing a 'raise from'
 /// from the error contained in error_already_set to indicate that the chosen error was
-/// caused by the original error. After this function is called error_already_set will
-/// no longer contain an error.
+/// caused by the original error.
 inline void raise_from(error_already_set &err, PyObject *type, const char *message) {
     err.restore();
     raise_from(type, message);