-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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.
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
Add support for nested exceptions in custom exceptions #4366
base: master
Are you sure you want to change the base?
Conversation
@@ -2567,6 +2567,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); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@@ -2535,7 +2535,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); } |
There was a problem hiding this comment.
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,
There was a problem hiding this comment.
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);
}
include/pybind11/detail/internals.h
Outdated
// restricted to a single module. Whether a module has local internals or not should not | ||
// impact any other modules, because the only things accessing the local internals is the | ||
// module that contains them. | ||
struct local_internals { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just moved struct local_internals
right below struct internals
since both types are needed in apply_exception_translators()
. As a side effect I personally found the file easier to read when both structs were nearby.
include/pybind11/detail/internals.h
Outdated
return false; | ||
} | ||
|
||
inline bool apply_exception_translators(std::forward_list<ExceptionTranslator> &translators) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved from pybind11.h
and slightly refactored so that it can be used by both the public API and by handle_nested_exception()
.
include/pybind11/detail/internals.h
Outdated
|
||
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; | ||
auto &local_translators = get_local_internals().registered_exception_translators; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Handling of the translator chain is identical to the lines 991-999 of pybind11.h
.
Thoughts on this @rwgk ? |
By the way, I think that there should be a follow-up PR (in case this one gets accepted) that offers some public API that directly or indirectly exposes the functionality of |
Yeah, exactly my concerns with this current PR logic. Not sure how to best proceed on that front. |
Looks good to me, I spent a significant amount of time looking around. I haven't worked on that part of the code myself and cannot claim to understand every aspect of this PR, but the new tests are clearly convincing. Reporting two details:
- void operator()(const char *message) { detail::raise_err(m_ptr, message); }
+ void operator()(const char *message) { PyErr_SetString(m_ptr, message); } then ran the tests:
Very nice! The change is necessary and covered.
One wish: could you please undo the move of the I think @Skylion007 asked about global testing: I need to update the smart_holder branch first, which could be relatively tricky this time around because of #4285. I'll try to do that asap and then run the global testing for this PR. |
@rwgk Mind cycling back now that smart holder should be fixed? |
@cbalioglu could you please rebase this branch? Currently it has conflicts. And ideally, could you please also undo the move of the |
@rwgk Hey folks, sorry for the delay. I will have rebase and update the PR later today. |
@rwgk I pushed the revised version of the PR. As you mentioned, the macOS and pip Ubuntu tests failed as expected. However I also see that pypy-3.7 test on Windows 2022-x64 failed. Looking at its log it does not seem to be related to the changes included in this PR, but please let me know if you think otherwise. I don't have a Windows machine, but I can try to fix the issue if it is caused by this PR. |
Yes, that's a known flake and unrelated for sure. (I still need to look at the code changes.) |
include/pybind11/detail/internals.h
Outdated
// 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, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The args to these can actually be const ref, can't they?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, that was an oversight on my end. Just pushed another revision.
I just want to say sorry that I didn't get back here today. This issue keeps derailing me all the time: https://gitlab.kitware.com/cmake/cmake/-/issues/24398 I've been unsuccessful getting a clean smart_holder update (the branch is behind again). I'll keep trying ... |
Interactive testing with all sanitizers in the Google-internal toolchain passed. Global testing initiated. I'm really swamped btw, not sure if/when I'll have time for a full review. I hope others will jump in (with approvals). |
This PR passed global testing (internal test ID OCL:508444881:BASE:508562211:1676007777198:e805d741). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more obvious to me now why you wanted to move the code. I still believe that move is better done in a separate PR, for clarity when introducing the new feature here, but either before this PR, or as a follow-on to this one. This PR looks good to me as-is. Up to you if you prefer move first, move later, or simply this PR without moving the code.
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 |
There was a problem hiding this comment.
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?
|
||
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(); |
There was a problem hiding this comment.
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).
Description
This PR extends #3608 for custom exception types registered via
register_exception
andregister_local_exception
.Suggested changelog entry: