From 9eded3629ac3bbe0277013654eed7fd613636191 Mon Sep 17 00:00:00 2001 From: vfdev-5 Date: Mon, 9 Sep 2024 22:16:30 +0200 Subject: [PATCH 1/5] Added exception translator specific mutex used with try_translate_exceptions Fixes #5346 --- include/pybind11/detail/exception_translation.h | 2 +- include/pybind11/detail/internals.h | 10 ++++++++++ include/pybind11/pybind11.h | 4 ++-- 3 files changed, 13 insertions(+), 3 deletions(-) diff --git a/include/pybind11/detail/exception_translation.h b/include/pybind11/detail/exception_translation.h index 2764180bb0..0e4c22328d 100644 --- a/include/pybind11/detail/exception_translation.h +++ b/include/pybind11/detail/exception_translation.h @@ -50,7 +50,7 @@ inline void try_translate_exceptions() { - delegate translation to the next translator by throwing a new type of exception. */ - bool handled = with_internals([&](internals &internals) { + bool handled = with_internals_for_exception_translator([&](internals &internals) { auto &local_exception_translators = get_local_internals().registered_exception_translators; if (detail::apply_exception_translators(local_exception_translators)) { return true; diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 7cc6f53adf..2af6c78758 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -177,6 +177,7 @@ static_assert(sizeof(instance_map_shard) % 64 == 0, struct internals { #ifdef Py_GIL_DISABLED pymutex mutex; + pymutex exception_translator_mutex; #endif // std::type_index -> pybind11's type information type_map registered_types_cpp; @@ -643,6 +644,15 @@ inline auto with_internals(const F &cb) -> decltype(cb(get_internals())) { return cb(internals); } +template +inline auto with_internals_for_exception_translator(const F &cb) -> decltype(cb(get_internals())) { + auto &internals = get_internals(); +#ifdef Py_GIL_DISABLED + std::unique_lock lock((internals).exception_translator_mutex); +#endif + return cb(internals); +} + inline std::uint64_t mix64(std::uint64_t z) { // David Stafford's variant 13 of the MurmurHash3 finalizer popularized // by the SplitMix PRNG. diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index c52df969e6..f30c4e3a68 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2586,7 +2586,7 @@ void implicitly_convertible() { } inline void register_exception_translator(ExceptionTranslator &&translator) { - detail::with_internals([&](detail::internals &internals) { + detail::with_internals_for_exception_translator([&](detail::internals &internals) { internals.registered_exception_translators.push_front( std::forward(translator)); }); @@ -2599,7 +2599,7 @@ inline void register_exception_translator(ExceptionTranslator &&translator) { * the exception. */ inline void register_local_exception_translator(ExceptionTranslator &&translator) { - detail::with_internals([&](detail::internals &internals) { + detail::with_internals_for_exception_translator([&](detail::internals &internals) { (void) internals; detail::get_local_internals().registered_exception_translators.push_front( std::forward(translator)); From a546f532f1c6db2e2e2457233a072b3994e1173e Mon Sep 17 00:00:00 2001 From: vfdev-5 Date: Tue, 10 Sep 2024 00:52:32 +0200 Subject: [PATCH 2/5] - Replaced with_internals_for_exception_translator by with_exception_translators - Incremented PYBIND11_INTERNALS_VERSION - Added a test --- .../pybind11/detail/exception_translation.h | 22 +++++------ include/pybind11/detail/internals.h | 14 +++++-- include/pybind11/pybind11.h | 21 +++++----- tests/custom_exceptions.py | 9 +++++ tests/test_exceptions.cpp | 38 +++++++++++++++++++ tests/test_exceptions.py | 5 +++ 6 files changed, 86 insertions(+), 23 deletions(-) create mode 100644 tests/custom_exceptions.py diff --git a/include/pybind11/detail/exception_translation.h b/include/pybind11/detail/exception_translation.h index 0e4c22328d..22ae8a1c94 100644 --- a/include/pybind11/detail/exception_translation.h +++ b/include/pybind11/detail/exception_translation.h @@ -50,17 +50,17 @@ inline void try_translate_exceptions() { - delegate translation to the next translator by throwing a new type of exception. */ - bool handled = with_internals_for_exception_translator([&](internals &internals) { - auto &local_exception_translators = get_local_internals().registered_exception_translators; - if (detail::apply_exception_translators(local_exception_translators)) { - return true; - } - auto &exception_translators = internals.registered_exception_translators; - if (detail::apply_exception_translators(exception_translators)) { - return true; - } - return false; - }); + bool handled = with_exception_translators( + [&](std::forward_list &exception_translators, + std::forward_list &local_exception_translators) { + if (detail::apply_exception_translators(local_exception_translators)) { + return true; + } + if (detail::apply_exception_translators(exception_translators)) { + return true; + } + return false; + }); if (!handled) { set_error(PyExc_SystemError, "Exception escaped from default exception translator!"); diff --git a/include/pybind11/detail/internals.h b/include/pybind11/detail/internals.h index 2af6c78758..fa224e0326 100644 --- a/include/pybind11/detail/internals.h +++ b/include/pybind11/detail/internals.h @@ -39,7 +39,11 @@ # if PY_VERSION_HEX >= 0x030C0000 || defined(_MSC_VER) // Version bump for Python 3.12+, before first 3.12 beta release. // Version bump for MSVC piggy-backed on PR #4779. See comments there. -# define PYBIND11_INTERNALS_VERSION 5 +# ifdef Py_GIL_DISABLED +# define PYBIND11_INTERNALS_VERSION 6 +# else +# define PYBIND11_INTERNALS_VERSION 5 +# endif # else # define PYBIND11_INTERNALS_VERSION 4 # endif @@ -645,12 +649,16 @@ inline auto with_internals(const F &cb) -> decltype(cb(get_internals())) { } template -inline auto with_internals_for_exception_translator(const F &cb) -> decltype(cb(get_internals())) { +inline auto with_exception_translators(const F &cb) + -> decltype(cb(get_internals().registered_exception_translators, + get_local_internals().registered_exception_translators)) { auto &internals = get_internals(); #ifdef Py_GIL_DISABLED std::unique_lock lock((internals).exception_translator_mutex); #endif - return cb(internals); + auto &local_internals = get_local_internals(); + return cb(internals.registered_exception_translators, + local_internals.registered_exception_translators); } inline std::uint64_t mix64(std::uint64_t z) { diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index f30c4e3a68..bcdf641f43 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -2586,10 +2586,12 @@ void implicitly_convertible() { } inline void register_exception_translator(ExceptionTranslator &&translator) { - detail::with_internals_for_exception_translator([&](detail::internals &internals) { - internals.registered_exception_translators.push_front( - std::forward(translator)); - }); + detail::with_exception_translators( + [&](std::forward_list &exception_translators, + std::forward_list &local_exception_translators) { + (void) local_exception_translators; + exception_translators.push_front(std::forward(translator)); + }); } /** @@ -2599,11 +2601,12 @@ inline void register_exception_translator(ExceptionTranslator &&translator) { * the exception. */ inline void register_local_exception_translator(ExceptionTranslator &&translator) { - detail::with_internals_for_exception_translator([&](detail::internals &internals) { - (void) internals; - detail::get_local_internals().registered_exception_translators.push_front( - std::forward(translator)); - }); + detail::with_exception_translators( + [&](std::forward_list &exception_translators, + std::forward_list &local_exception_translators) { + (void) exception_translators; + local_exception_translators.push_front(std::forward(translator)); + }); } /** diff --git a/tests/custom_exceptions.py b/tests/custom_exceptions.py new file mode 100644 index 0000000000..271e08ff59 --- /dev/null +++ b/tests/custom_exceptions.py @@ -0,0 +1,9 @@ + +class PythonMyException7(Exception): + def __init__(self, message): + self.message = message + super().__init__(message) + + def __str__(self): + s = "[PythonMyException7]: " + self.message.a + return s diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index c1d05bb241..24ffd91f74 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -111,6 +111,16 @@ struct PythonAlreadySetInDestructor { py::str s; }; +struct CustomData { + CustomData(const std::string &a) : a(a) {} + std::string a; +}; + +struct MyException7 { + MyException7(const CustomData &message) : message(message) {} + CustomData message; +}; + TEST_SUBMODULE(exceptions, m) { m.def("throw_std_exception", []() { throw std::runtime_error("This exception was intentionally thrown."); }); @@ -385,4 +395,32 @@ TEST_SUBMODULE(exceptions, m) { // m.def("pass_exception_void", [](const py::exception&) {}); // Does not compile. m.def("return_exception_void", []() { return py::exception(); }); + + m.def("throws7", []() { + auto data = CustomData("abc"); + throw MyException7(data); + }); + + py::class_(m, "CustomData", py::module_local()) + .def(py::init()) + .def_readwrite("a", &CustomData::a); + + PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store storage; + storage.call_once_and_store_result([&]() { + auto mod = py::module_::import("custom_exceptions"); + py::object obj = mod.attr("PythonMyException7"); + return obj; + }); + + py::register_local_exception_translator([](std::exception_ptr p) { + try { + if (p) { + std::rethrow_exception(p); + } + } catch (const MyException7 &e) { + auto obj = storage.get_stored(); + py::object obj2 = obj(e.message); + PyErr_SetObject(PyExc_Exception, obj2.ptr()); + } + }); } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index ba5063a749..a8fd105ea0 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -3,6 +3,7 @@ import sys import pytest +from custom_exceptions import PythonMyException7 import env import pybind11_cross_module_tests as cm @@ -195,6 +196,10 @@ def test_custom(msg): raise RuntimeError("Exception error: caught child from parent") from err assert msg(excinfo.value) == "this is a helper-defined translated exception" + with pytest.raises(PythonMyException7) as excinfo: + m.throws7() + assert msg(excinfo.value) == "[PythonMyException7]: abc" + def test_nested_throws(capture): """Tests nested (e.g. C++ -> Python -> C++) exception handling""" From 418251041c7cba689c0ba8e6044a8849c7386229 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Mon, 9 Sep 2024 22:54:33 +0000 Subject: [PATCH 3/5] style: pre-commit fixes --- tests/custom_exceptions.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/custom_exceptions.py b/tests/custom_exceptions.py index 271e08ff59..2bb07e8273 100644 --- a/tests/custom_exceptions.py +++ b/tests/custom_exceptions.py @@ -1,3 +1,5 @@ +from __future__ import annotations + class PythonMyException7(Exception): def __init__(self, message): From 6e3f68e7c1296543a343c9a7b2befab7509e5036 Mon Sep 17 00:00:00 2001 From: vfdev-5 Date: Tue, 10 Sep 2024 00:56:41 +0200 Subject: [PATCH 4/5] Fixed formatting and added explicit to ctors --- tests/custom_exceptions.py | 3 +-- tests/test_exceptions.cpp | 4 ++-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/tests/custom_exceptions.py b/tests/custom_exceptions.py index 2bb07e8273..b1a092d761 100644 --- a/tests/custom_exceptions.py +++ b/tests/custom_exceptions.py @@ -7,5 +7,4 @@ def __init__(self, message): super().__init__(message) def __str__(self): - s = "[PythonMyException7]: " + self.message.a - return s + return "[PythonMyException7]: " + self.message.a diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 24ffd91f74..126ff87ae2 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -112,12 +112,12 @@ struct PythonAlreadySetInDestructor { }; struct CustomData { - CustomData(const std::string &a) : a(a) {} + explicit CustomData(const std::string &a) : a(a) {} std::string a; }; struct MyException7 { - MyException7(const CustomData &message) : message(message) {} + explicit MyException7(const CustomData &message) : message(message) {} CustomData message; }; From 8275a6a2e538a7d7c4cadf3de6a103c45dabdec0 Mon Sep 17 00:00:00 2001 From: vfdev-5 Date: Mon, 16 Sep 2024 10:46:12 +0200 Subject: [PATCH 5/5] Addressed PR review comments --- tests/test_exceptions.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 126ff87ae2..0a970065b4 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -405,8 +405,9 @@ TEST_SUBMODULE(exceptions, m) { .def(py::init()) .def_readwrite("a", &CustomData::a); - PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store storage; - storage.call_once_and_store_result([&]() { + PYBIND11_CONSTINIT static py::gil_safe_call_once_and_store + PythonMyException7_storage; + PythonMyException7_storage.call_once_and_store_result([&]() { auto mod = py::module_::import("custom_exceptions"); py::object obj = mod.attr("PythonMyException7"); return obj; @@ -418,9 +419,9 @@ TEST_SUBMODULE(exceptions, m) { std::rethrow_exception(p); } } catch (const MyException7 &e) { - auto obj = storage.get_stored(); - py::object obj2 = obj(e.message); - PyErr_SetObject(PyExc_Exception, obj2.ptr()); + auto exc_type = PythonMyException7_storage.get_stored(); + py::object exc_inst = exc_type(e.message); + PyErr_SetObject(PyExc_Exception, exc_inst.ptr()); } }); }