Skip to content

Commit 80dfaea

Browse files
committed
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.
1 parent 0739d4c commit 80dfaea

File tree

4 files changed

+48
-50
lines changed

4 files changed

+48
-50
lines changed

include/pybind11/pybind11.h

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2626,21 +2626,18 @@ void print(Args &&...args) {
26262626
}
26272627

26282628
inline error_already_set::~error_already_set() {
2629-
if (!m_fetched_error.has_py_object_references()) {
2629+
if (m_fetched_error.use_count() != 1 || !m_fetched_error->has_py_object_references()) {
26302630
return; // Avoid gil and scope overhead if there is nothing to release.
26312631
}
26322632
gil_scoped_acquire gil;
26332633
error_scope scope;
2634-
m_fetched_error.release_py_object_references();
2634+
m_fetched_error->release_py_object_references();
26352635
}
26362636

2637-
inline error_already_set::error_already_set(const error_already_set &other)
2638-
: m_fetched_error(gil_scoped_acquire(), other.m_fetched_error) {}
2639-
26402637
inline const char *error_already_set::what() const noexcept {
26412638
gil_scoped_acquire gil;
26422639
error_scope scope;
2643-
return m_fetched_error.error_string().c_str();
2640+
return m_fetched_error->error_string().c_str();
26442641
}
26452642

26462643
PYBIND11_NAMESPACE_BEGIN(detail)

include/pybind11/pytypes.h

Lines changed: 25 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -435,11 +435,8 @@ struct error_fetch_and_normalize {
435435
}
436436
}
437437

438-
template <typename GilScopedAcquire>
439-
error_fetch_and_normalize(const GilScopedAcquire &, const error_fetch_and_normalize &other)
440-
: m_type{other.m_type}, m_value{other.m_value}, m_trace{other.m_trace},
441-
m_lazy_error_string{other.m_lazy_error_string},
442-
m_lazy_error_string_completed{other.m_lazy_error_string_completed} {}
438+
error_fetch_and_normalize(const error_fetch_and_normalize &) = delete;
439+
error_fetch_and_normalize(error_fetch_and_normalize &&) = delete;
443440

444441
std::string complete_lazy_error_string() const {
445442
std::string result;
@@ -521,10 +518,13 @@ struct error_fetch_and_normalize {
521518
}
522519

523520
void restore() {
524-
// As long as this type is copyable, there is no point in releasing m_type, m_value,
525-
// m_trace, but simply holding on the the references makes it possible to produce
526-
// what() even after restore().
521+
if (m_restore_called) {
522+
pybind11_fail("Internal error: pybind11::detail::error_fetch_and_normalize::restore() "
523+
"called a second time. ORIGINAL ERROR: "
524+
+ error_string());
525+
}
527526
PyErr_Restore(m_type.inc_ref().ptr(), m_value.inc_ref().ptr(), m_trace.inc_ref().ptr());
527+
m_restore_called = true;
528528
}
529529

530530
bool matches(handle exc) const {
@@ -542,6 +542,7 @@ struct error_fetch_and_normalize {
542542
object m_type, m_value, m_trace;
543543
mutable std::string m_lazy_error_string;
544544
mutable bool m_lazy_error_string_completed = false;
545+
mutable bool m_restore_called = false;
545546
};
546547

547548
inline std::string error_string() {
@@ -564,18 +565,21 @@ class PYBIND11_EXPORT_EXCEPTION error_already_set : public std::exception {
564565
public:
565566
/// Fetches the current Python exception (using PyErr_Fetch()), which will clear the
566567
/// current Python error indicator.
567-
error_already_set() : m_fetched_error{"pybind11::error_already_set"} {}
568+
error_already_set()
569+
: m_fetched_error{
570+
std::make_shared<detail::error_fetch_and_normalize>("pybind11::error_already_set")} {}
568571

569572
/// WARNING: This destructor needs to acquire the Python GIL. This can lead to
570573
/// crashes (undefined behavior) if the Python interpreter is finalizing.
571-
~error_already_set();
574+
~error_already_set() override;
572575

573-
/// The C++ standard explicitly prohibits deleting this copy ctor: C++17 18.1.5.
574-
/// WARNING: This copy constructor needs to acquire the Python GIL. This can lead to
575-
/// crashes (undefined behavior) if the Python interpreter is finalizing.
576-
error_already_set(const error_already_set &);
576+
// This copy ctor does not need the GIL because it simply increments a shared_ptr use_count.
577+
error_already_set(const error_already_set &) noexcept = default;
577578

578-
error_already_set(error_already_set &&) = default;
579+
// This move ctor cannot easily be deleted (some compilers need it).
580+
// It is the responsibility of the caller to not use the moved-from object.
581+
// For simplicity, guarding ifs are omitted.
582+
error_already_set(error_already_set &&) noexcept = default;
579583

580584
/// The what() result is built lazily on demand.
581585
/// 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 {
587591
/// NOTE: This member function will always restore the normalized exception, which may or may
588592
/// not be the original Python exception.
589593
/// WARNING: The GIL must be held when this member function is called!
590-
void restore() { m_fetched_error.restore(); }
594+
void restore() { m_fetched_error->restore(); }
591595

592596
/// If it is impossible to raise the currently-held error, such as in a destructor, we can
593597
/// 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 {
611615
/// Check if the currently trapped error type matches the given Python exception class (or a
612616
/// subclass thereof). May also be passed a tuple to search for any exception class matches in
613617
/// the given tuple.
614-
bool matches(handle exc) const { return m_fetched_error.matches(exc); }
618+
bool matches(handle exc) const { return m_fetched_error->matches(exc); }
615619

616-
const object &type() const { return m_fetched_error.m_type; }
617-
const object &value() const { return m_fetched_error.m_value; }
618-
const object &trace() const { return m_fetched_error.m_trace; }
620+
const object &type() const { return m_fetched_error->m_type; }
621+
const object &value() const { return m_fetched_error->m_value; }
622+
const object &trace() const { return m_fetched_error->m_trace; }
619623

620624
private:
621-
detail::error_fetch_and_normalize m_fetched_error;
625+
std::shared_ptr<detail::error_fetch_and_normalize> m_fetched_error;
622626
};
623627
#if defined(_MSC_VER)
624628
# pragma warning(pop)

tests/test_exceptions.cpp

Lines changed: 10 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -310,24 +310,6 @@ TEST_SUBMODULE(exceptions, m) {
310310
}
311311
});
312312

313-
m.def("move_error_already_set", [](bool use_move) {
314-
try {
315-
PyErr_SetString(PyExc_RuntimeError, use_move ? "To be moved." : "To be copied.");
316-
throw py::error_already_set();
317-
} catch (py::error_already_set &caught) {
318-
if (use_move) {
319-
py::error_already_set moved_to{std::move(caught)};
320-
return std::string(moved_to.what()); // Both destructors run.
321-
}
322-
// NOLINTNEXTLINE(performance-unnecessary-copy-initialization)
323-
py::error_already_set copied_to{caught};
324-
return std::string(copied_to.what()); // Both destructors run.
325-
}
326-
#if !defined(_MSC_VER) // MSVC detects that this is unreachable.
327-
return std::string("Unreachable.");
328-
#endif
329-
});
330-
331313
m.def("error_already_set_what", [](const py::object &exc_type, const py::object &exc_value) {
332314
PyErr_SetObject(exc_type.ptr(), exc_value.ptr());
333315
std::string what = py::error_already_set().what();
@@ -342,4 +324,14 @@ TEST_SUBMODULE(exceptions, m) {
342324
= reinterpret_cast<void (*)()>(PyLong_AsVoidPtr(cm.attr("funcaddr").ptr()));
343325
interleaved_error_already_set();
344326
});
327+
328+
m.def("test_error_already_set_double_restore", [](bool dry_run) {
329+
PyErr_SetString(PyExc_ValueError, "Random error.");
330+
py::error_already_set e;
331+
e.restore();
332+
PyErr_Clear();
333+
if (!dry_run) {
334+
e.restore();
335+
}
336+
});
345337
}

tests/test_exceptions.py

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -275,11 +275,6 @@ def test_local_translator(msg):
275275
assert msg(excinfo.value) == "this mod"
276276

277277

278-
@pytest.mark.parametrize("use_move, expected", ((False, "copied."), (True, "moved.")))
279-
def test_error_already_set_copy_move(use_move, expected):
280-
assert m.move_error_already_set(use_move) == "RuntimeError: To be " + expected
281-
282-
283278
class FlakyException(Exception):
284279
def __init__(self, failure_point):
285280
if failure_point == "failure_point_init":
@@ -348,3 +343,13 @@ def test_cross_module_interleaved_error_already_set():
348343
"2nd error.", # Almost all platforms.
349344
"RuntimeError: 2nd error.", # Some PyPy builds (seen under macOS).
350345
)
346+
347+
348+
def test_error_already_set_double_restore():
349+
m.test_error_already_set_double_restore(True) # dry_run
350+
with pytest.raises(RuntimeError) as excinfo:
351+
m.test_error_already_set_double_restore(False)
352+
assert str(excinfo.value) == (
353+
"Internal error: pybind11::detail::error_fetch_and_normalize::restore()"
354+
" called a second time. ORIGINAL ERROR: ValueError: Random error."
355+
)

0 commit comments

Comments
 (0)