Use shared_ptr for m_fetched_error => 1. non-racy, copy ctor that doe…

…s not need the GIL; 2. enables guard against duplicate restore() calls.

include/pybind11/pybind11.h

@@ -2626,21 +2626,18 @@ void print(Args &&...args) {
 }
 
 inline error_already_set::~error_already_set() {
-    if (!m_fetched_error.has_py_object_references()) {
+    if (m_fetched_error.use_count() != 1 || !m_fetched_error->has_py_object_references()) {
         return; // Avoid gil and scope overhead if there is nothing to release.
     }
     gil_scoped_acquire gil;
     error_scope scope;
-    m_fetched_error.release_py_object_references();
+    m_fetched_error->release_py_object_references();
 }
 
-inline error_already_set::error_already_set(const error_already_set &other)
-    : m_fetched_error(gil_scoped_acquire(), other.m_fetched_error) {}
-
 inline const char *error_already_set::what() const noexcept {
     gil_scoped_acquire gil;
     error_scope scope;
-    return m_fetched_error.error_string().c_str();
+    return m_fetched_error->error_string().c_str();
 }
 
 PYBIND11_NAMESPACE_BEGIN(detail)

include/pybind11/pytypes.h

@@ -435,11 +435,8 @@ struct error_fetch_and_normalize {
         }
     }
 
-    template <typename GilScopedAcquire>
-    error_fetch_and_normalize(const GilScopedAcquire &, const error_fetch_and_normalize &other)
-        : m_type{other.m_type}, m_value{other.m_value}, m_trace{other.m_trace},
-          m_lazy_error_string{other.m_lazy_error_string},
-          m_lazy_error_string_completed{other.m_lazy_error_string_completed} {}
+    error_fetch_and_normalize(const error_fetch_and_normalize &) = delete;
+    error_fetch_and_normalize(error_fetch_and_normalize &&) = delete;
 
     std::string complete_lazy_error_string() const {
         std::string result;
@@ -521,10 +518,13 @@ struct error_fetch_and_normalize {
     }
 
     void restore() {
-        // 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().
+        if (m_restore_called) {
+            pybind11_fail("Internal error: pybind11::detail::error_fetch_and_normalize::restore() "
+                          "called a second time. ORIGINAL ERROR: "
+                          + error_string());
+        }
         PyErr_Restore(m_type.inc_ref().ptr(), m_value.inc_ref().ptr(), m_trace.inc_ref().ptr());
+        m_restore_called = true;
     }
 
     bool matches(handle exc) const {
@@ -542,6 +542,7 @@ struct error_fetch_and_normalize {
     object m_type, m_value, m_trace;
     mutable std::string m_lazy_error_string;
     mutable bool m_lazy_error_string_completed = false;
+    mutable bool m_restore_called = false;
 };
 
 inline std::string error_string() {
@@ -564,18 +565,21 @@ class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::exception {
 public:
     /// Fetches the current Python exception (using PyErr_Fetch()), which will clear the
     /// current Python error indicator.
-    error_already_set() : m_fetched_error{"pybind11::error_already_set"} {}
+    error_already_set()
+        : m_fetched_error{
+            std::make_shared<detail::error_fetch_and_normalize>("pybind11::error_already_set")} {}
 
     /// WARNING: This destructor needs to acquire the Python GIL. This can lead to
     ///          crashes (undefined behavior) if the Python interpreter is finalizing.
-    ~error_already_set();
+    ~error_already_set() override;
 
-    /// The C++ standard explicitly prohibits deleting this copy ctor: C++17 18.1.5.
-    /// WARNING: This copy constructor needs to acquire the Python GIL. This can lead to
-    ///          crashes (undefined behavior) if the Python interpreter is finalizing.
-    error_already_set(const error_already_set &);
+    // This copy ctor does not need the GIL because it simply increments a shared_ptr use_count.
+    error_already_set(const error_already_set &) noexcept = default;
 
-    error_already_set(error_already_set &&) = default;
+    // This move ctor cannot easily be deleted (some compilers need it).
+    // It is the responsibility of the caller to not use the moved-from object.
+    // For simplicity, guarding ifs are omitted.
+    error_already_set(error_already_set &&) noexcept = default;
 
     /// The what() result is built lazily on demand.
     /// WARNING: This member function needs to acquire the Python GIL. This can lead to
@@ -587,7 +591,7 @@ class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::exception {
     /// NOTE: This member function will always restore the normalized exception, which may or may
     ///       not be the original Python exception.
     /// WARNING: The GIL must be held when this member function is called!
-    void restore() { m_fetched_error.restore(); }
+    void restore() { m_fetched_error->restore(); }
 
     /// 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
@@ -611,14 +615,14 @@ class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::exception {
     /// Check if the currently trapped error type matches the given Python exception class (or a
     /// subclass thereof).  May also be passed a tuple to search for any exception class matches in
     /// the given tuple.
-    bool matches(handle exc) const { return m_fetched_error.matches(exc); }
+    bool matches(handle exc) const { return m_fetched_error->matches(exc); }
 
-    const object &type() const { return m_fetched_error.m_type; }
-    const object &value() const { return m_fetched_error.m_value; }
-    const object &trace() const { return m_fetched_error.m_trace; }
+    const object &type() const { return m_fetched_error->m_type; }
+    const object &value() const { return m_fetched_error->m_value; }
+    const object &trace() const { return m_fetched_error->m_trace; }
 
 private:
-    detail::error_fetch_and_normalize m_fetched_error;
+    std::shared_ptr<detail::error_fetch_and_normalize> m_fetched_error;
 };
 #if defined(_MSC_VER)
 #    pragma warning(pop)

tests/test_exceptions.cpp

@@ -310,24 +310,6 @@ TEST_SUBMODULE(exceptions, m) {
         }
     });
 
-    m.def("move_error_already_set", [](bool use_move) {
-        try {
-            PyErr_SetString(PyExc_RuntimeError, use_move ? "To be moved." : "To be copied.");
-            throw py::error_already_set();
-        } catch (py::error_already_set &caught) {
-            if (use_move) {
-                py::error_already_set moved_to{std::move(caught)};
-                return std::string(moved_to.what()); // Both destructors run.
-            }
-            // NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
-            py::error_already_set copied_to{caught};
-            return std::string(copied_to.what()); // Both destructors run.
-        }
-#if !defined(_MSC_VER) // MSVC detects that this is unreachable.
-        return std::string("Unreachable.");
-#endif
-    });
-
     m.def("error_already_set_what", [](const py::object &exc_type, const py::object &exc_value) {
         PyErr_SetObject(exc_type.ptr(), exc_value.ptr());
         std::string what = py::error_already_set().what();
@@ -342,4 +324,14 @@ TEST_SUBMODULE(exceptions, m) {
             = reinterpret_cast<void (*)()>(PyLong_AsVoidPtr(cm.attr("funcaddr").ptr()));
         interleaved_error_already_set();
     });
+
+    m.def("test_error_already_set_double_restore", [](bool dry_run) {
+        PyErr_SetString(PyExc_ValueError, "Random error.");
+        py::error_already_set e;
+        e.restore();
+        PyErr_Clear();
+        if (!dry_run) {
+            e.restore();
+        }
+    });
 }

tests/test_exceptions.py

@@ -275,11 +275,6 @@ def test_local_translator(msg):
     assert msg(excinfo.value) == "this mod"
 
 
-@pytest.mark.parametrize("use_move, expected", ((False, "copied."), (True, "moved.")))
-def test_error_already_set_copy_move(use_move, expected):
-    assert m.move_error_already_set(use_move) == "RuntimeError: To be " + expected
-
-
 class FlakyException(Exception):
     def __init__(self, failure_point):
         if failure_point == "failure_point_init":
@@ -348,3 +343,13 @@ def test_cross_module_interleaved_error_already_set():
         "2nd error.",  # Almost all platforms.
         "RuntimeError: 2nd error.",  # Some PyPy builds (seen under macOS).
     )
+
+
+def test_error_already_set_double_restore():
+    m.test_error_already_set_double_restore(True)  # dry_run
+    with pytest.raises(RuntimeError) as excinfo:
+        m.test_error_already_set_double_restore(False)
+    assert str(excinfo.value) == (
+        "Internal error: pybind11::detail::error_fetch_and_normalize::restore()"
+        " called a second time. ORIGINAL ERROR: ValueError: Random error."
+    )