Skip to content

Add support for nested exceptions in custom exceptions #4366

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Add support for nested exceptions in custom exceptions #4366

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e9eb4ad
Add support for nested exceptions in custom exceptions
cbalioglu Nov 28, 2022
d068cc6
Ensure that all translators are considered for nested exceptions
cbalioglu Nov 28, 2022
f51c638
Re-organize internals.h
cbalioglu Feb 7, 2023
beaaa1c
style: pre-commit fixes
pre-commit-ci[bot] Feb 7, 2023
b2b6744
Add constness to the function signatures
cbalioglu Feb 7, 2023
8984743
style: pre-commit fixes
pre-commit-ci[bot] Feb 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 41 additions & 4 deletions include/pybind11/detail/internals.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -314,16 +314,45 @@ inline internals **&get_internals_pp() {
return internals_pp;
}

// forward decl
inline void translate_exception(std::exception_ptr);
const std::forward_list<ExceptionTranslator> &get_exception_translators();
const std::forward_list<ExceptionTranslator> &get_local_exception_translators();

// Apply all the extensions translators from a list
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is extensions correct here? I see this comment is just moved from pybind11.h, but maybe it's a long-standing oversight?

// Return true if one of the translators completed without raising an exception
// itself. Return of false indicates that if there are other translators
// available, they should be tried.
inline bool apply_exception_translators(const std::forward_list<ExceptionTranslator> &translators,
std::exception_ptr last_exception) {
for (const auto &translator : translators) {
try {
translator(last_exception);
return true;
} catch (...) {
last_exception = std::current_exception();
}
}
return false;
}

inline bool
apply_exception_translators(const std::forward_list<ExceptionTranslator> &translators) {
return apply_exception_translators(translators, std::current_exception());
}

template <class T,
enable_if_t<std::is_same<std::nested_exception, remove_cvref_t<T>>::value, int> = 0>
bool handle_nested_exception(const T &exc, const std::exception_ptr &p) {
std::exception_ptr nested = exc.nested_ptr();
if (nested != nullptr && nested != p) {
translate_exception(nested);
return true;
const auto &local_translators = get_local_exception_translators();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the temporary variables (local_translators, translators). Simply inlining the function calls would seem nicer (less code and very clear, too).

if (apply_exception_translators(local_translators, nested)) {
return true;
}

const auto &translators = get_exception_translators();
if (apply_exception_translators(translators, nested)) {
return true;
}
}
return false;
}
Expand Down Expand Up @@ -493,6 +522,10 @@ PYBIND11_NOINLINE internals &get_internals() {
return **internals_pp;
}

inline const std::forward_list<ExceptionTranslator> &get_exception_translators() {
return get_internals().registered_exception_translators;
}

// the internals struct (above) is shared between all the modules. local_internals are only
// for a single module. Any changes made to internals may require an update to
// PYBIND11_INTERNALS_VERSION, breaking backwards compatibility. local_internals is, by design,
Expand Down Expand Up @@ -549,6 +582,10 @@ inline local_internals &get_local_internals() {
return *locals;
}

inline const std::forward_list<ExceptionTranslator> &get_local_exception_translators() {
return get_local_internals().registered_exception_translators;
}

/// Constructs a std::string with the given arguments, stores it in `internals`, and returns its
/// `c_str()`. Such strings objects have a long storage duration -- the internal strings are only
/// cleared when the program exits or after interpreter shutdown (when embedding), and so are
Expand Down
22 changes: 3 additions & 19 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,24 +52,6 @@ PYBIND11_WARNING_DISABLE_MSVC(4127)

PYBIND11_NAMESPACE_BEGIN(detail)

// Apply all the extensions translators from a list
// Return true if one of the translators completed without raising an exception
// itself. Return of false indicates that if there are other translators
// available, they should be tried.
inline bool apply_exception_translators(std::forward_list<ExceptionTranslator> &translators) {
auto last_exception = std::current_exception();

for (auto &translator : translators) {
try {
translator(last_exception);
return true;
} catch (...) {
last_exception = std::current_exception();
}
}
return false;
}

#if defined(_MSC_VER)
# define PYBIND11_COMPAT_STRDUP _strdup
#else
Expand Down Expand Up @@ -2541,7 +2523,7 @@ class exception : public object {
}

// Sets the current python exception to this exception object with the given message
void operator()(const char *message) { PyErr_SetString(m_ptr, message); }
void operator()(const char *message) { detail::raise_err(m_ptr, message); }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this tested in this PR? This would allow for nested python exception that raised via raise from (ie. if the Python err indicator is already set) etc,

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is essentially tested by the existing unit tests for register_exception() and also by the new test_throw_custom_nested_exception(). It just makes the custom exception translator "nested-aware" by replacing

PyErr_SetString(m_ptr, message);

with

if (PyErr_Occurred()) {
  raise_from(m_ptr, message);
} else {
  PyErr_SetString(m_ptr, message);
}

};

PYBIND11_NAMESPACE_BEGIN(detail)
Expand Down Expand Up @@ -2573,6 +2555,8 @@ register_exception_impl(handle scope, const char *name, handle base, bool isLoca
try {
std::rethrow_exception(p);
} catch (const CppException &e) {
detail::handle_nested_exception(e, p);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wouldn't properly handle nested custom / CppExceptions, properly though, right? Only would work if it's an STL exception, not if it's a nested CppException? We may want to add a test case where it breaks. It's unclear here which exception translator the nested exception should use, but it seems a bit bugprone to use the default one, no?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the catch! I completely oversaw that translate_exception() was only handling the STL exceptions. For some reason I was under the impression that it was handling the translator chain. I just pushed a revision that moves a few stuff to internals so that handle_nested_exception() mimics the same behavior as the default exception handling happening in lines 991-999 of pybind11.h.

Although I am an avid user of pybind11, I have very little experience with its code base. In fact I started fiddling with it just a few hours ago. So please let me know if anything is obviously wrong or needs a different approach.


detail::get_exception_object<CppException>()(e.what());
}
});
Expand Down
30 changes: 27 additions & 3 deletions tests/test_exceptions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,11 +297,35 @@ TEST_SUBMODULE(exceptions, m) {
}
});

m.def("throw_nested_exception", []() {
m.def("throw_stl_nested_exception", []() {
try {
throw std::runtime_error("Inner Exception");
throw std::runtime_error("Inner STL Exception");
} catch (const std::runtime_error &) {
std::throw_with_nested(std::runtime_error("Outer Exception"));
std::throw_with_nested(std::runtime_error("Outer STL Exception"));
}
});

m.def("throw_stl_nested_exception_with_custom_exception", []() {
try {
throw std::runtime_error("Inner STL Exception");
} catch (const std::runtime_error &) {
std::throw_with_nested(MyException5("Outer Custom Exception"));
}
});

m.def("throw_custom_nested_exception_with_stl_exception", []() {
try {
throw MyException5("Inner Custom Exception");
} catch (const MyException5 &) {
std::throw_with_nested(std::runtime_error("Outer STL Exception"));
}
});

m.def("throw_custom_nested_exception_with_custom_exception", []() {
try {
throw MyException5("Inner Custom Exception");
} catch (const MyException5 &) {
std::throw_with_nested(MyException5("Outer Custom Exception"));
}
});

Expand Down
29 changes: 25 additions & 4 deletions tests/test_exceptions.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,11 +241,32 @@ def pycatch(exctype, f, *args):
assert str(excinfo.value) == "this is a helper-defined translated exception"


def test_throw_nested_exception():
def test_throw_stl_nested_exception():
with pytest.raises(RuntimeError) as excinfo:
m.throw_nested_exception()
assert str(excinfo.value) == "Outer Exception"
assert str(excinfo.value.__cause__) == "Inner Exception"
m.throw_stl_nested_exception()
assert str(excinfo.value) == "Outer STL Exception"
assert str(excinfo.value.__cause__) == "Inner STL Exception"


def test_throw_stl_nested_exception_with_custom_exception():
with pytest.raises(m.MyException5) as excinfo:
m.throw_stl_nested_exception_with_custom_exception()
assert str(excinfo.value) == "Outer Custom Exception"
assert str(excinfo.value.__cause__) == "Inner STL Exception"


def test_throw_custom_nested_exception_with_stl_exception():
with pytest.raises(RuntimeError) as excinfo:
m.throw_custom_nested_exception_with_stl_exception()
assert str(excinfo.value) == "Outer STL Exception"
assert str(excinfo.value.__cause__) == "Inner Custom Exception"


def test_throw_custom_nested_exception_with_custom_exception():
with pytest.raises(m.MyException5) as excinfo:
m.throw_custom_nested_exception_with_custom_exception()
assert str(excinfo.value) == "Outer Custom Exception"
assert str(excinfo.value.__cause__) == "Inner Custom Exception"


# This can often happen if you wrap a pybind11 class in a Python wrapper
Expand Down