From 80dfaea1b93ae2222f079d95e9d740d8089bd2c1 Mon Sep 17 00:00:00 2001 From: "Ralf W. Grosse-Kunstleve" Date: Wed, 1 Jun 2022 12:01:40 -0700 Subject: [PATCH] Use shared_ptr for m_fetched_error => 1. non-racy, copy ctor that does not need the GIL; 2. enables guard against duplicate restore() calls. --- include/pybind11/pybind11.h | 9 +++----- include/pybind11/pytypes.h | 46 ++++++++++++++++++++----------------- tests/test_exceptions.cpp | 28 ++++++++-------------- tests/test_exceptions.py | 15 ++++++++---- 4 files changed, 48 insertions(+), 50 deletions(-) diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index cef8f38c9f..1f7a0631f4 100644 --- a/include/pybind11/pybind11.h +++ b/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) diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index cf46a136c1..7106c1fc0b 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -435,11 +435,8 @@ struct error_fetch_and_normalize { } } - template - 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("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 m_fetched_error; }; #if defined(_MSC_VER) # pragma warning(pop) diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 1fc2e0ee86..3ec999d1dc 100644 --- a/tests/test_exceptions.cpp +++ b/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(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(); + } + }); } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index 392a3ad3ed..639fc05fca 100644 --- a/tests/test_exceptions.py +++ b/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." + )