Add support for nested exceptions in custom exceptions #4366

cbalioglu · 2022-11-28T16:28:28Z

Description

This PR extends #3608 for custom exception types registered via register_exception and register_local_exception.

Suggested changelog entry:

* Adds support for nested exceptions in custom exceptions registered via ``register_exception`` and ``register_local_exception``.

Skylion007 · 2022-11-28T17:00:15Z

include/pybind11/pybind11.h

@@ -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);


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?

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.

Skylion007 · 2022-11-28T17:02:36Z

include/pybind11/pybind11.h

@@ -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); }


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,

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); }

cbalioglu · 2022-11-28T18:34:50Z

include/pybind11/detail/internals.h

+// 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 {


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.

cbalioglu · 2022-11-28T18:35:48Z

include/pybind11/detail/internals.h

+    return false;
+}
+
+inline bool apply_exception_translators(std::forward_list<ExceptionTranslator> &translators) {


Moved from pybind11.h and slightly refactored so that it can be used by both the public API and by handle_nested_exception().

cbalioglu · 2022-11-28T18:36:59Z

include/pybind11/detail/internals.h


 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;


Handling of the translator chain is identical to the lines 991-999 of pybind11.h.

Skylion007 · 2022-11-28T20:26:02Z

Thoughts on this @rwgk ?

cbalioglu · 2022-11-28T20:36:26Z

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 handle_nested_exception() for custom exception translators. This PR is sufficient for register_<local>_exception() since pybind11 "owns" the internal translator implementation for such exception types, but for advanced use cases where a custom translator is needed, there is no API to leverage the nested-exception functionality.

Skylion007 · 2022-11-28T20:37:20Z

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 handle_nested_exception() for custom exception translators. This PR is sufficient for register_<local>_exception() since pybind11 "owns" the internal translator implementation for such exception types, but for advanced use cases where a custom translator is needed, there is no API to leverage the nested-exception functionality.

Yeah, exactly my concerns with this current PR logic. Not sure how to best proceed on that front.

rwgk · 2022-12-01T07:09:55Z

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:

I locally undid this change in pybind11.h

-    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:

test_exceptions.py ............F.F............                                                                                       [ 43%]

Very nice! The change is necessary and covered.

I ran the pytest-based tests with ASAN, MSAN, UBSAN, TSAN using the Google toolchain: everything passed. (I didn't really expect issues, but it's good to be sure.)

One wish: could you please undo the move of the struct local_internals code, and move the two apply_exception_translators() overloads after the get_local_internals() definition (if that works)? I agree it would be nicer to move the code, but such strictly-speaking unnecessary moves often cause a lot more trouble than anticipated. While the move is still pretty obvious in the context of this PR, later on it will likely be confusing (e.g. lead people to incorrectly assume it's important to the functional changes under this PR). In my experience it is much better to make only essential changes, and do clean-up kind of work separately as a dedicated project. — Full disclosure and/or to add to the argument: the move will probably give me grief when updating my PRs #4329 & #4349 after this PR is merged (those two are already a pretty unwieldy pair of changes).

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.

Skylion007 · 2023-01-30T16:33:06Z

@rwgk Mind cycling back now that smart holder should be fixed?

rwgk · 2023-02-01T00:54:03Z

@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 struct local_internals code as suggested above?

cbalioglu · 2023-02-07T16:18:36Z

@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 struct local_internals code as suggested above?

@rwgk Hey folks, sorry for the delay. I will have rebase and update the PR later today.

rwgk · 2023-02-07T16:55:03Z

Note that we currently we 5 + 1 expected GHA failures: 5 x macos-latest (#4496), 1 x Pip ubuntu-latest (#4497)

Please ignore those for now.

cbalioglu · 2023-02-07T18:48:16Z

@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.

rwgk · 2023-02-07T18:57:32Z

Looking at its log it does not seem to be related to the changes included in this PR

Yes, that's a known flake and unrelated for sure.

(I still need to look at the code changes.)

Skylion007 · 2023-02-07T19:00:46Z

include/pybind11/detail/internals.h

+// 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,


The args to these can actually be const ref, can't they?

Right, that was an oversight on my end. Just pushed another revision.

rwgk · 2023-02-08T09:19:54Z

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 ...

rwgk · 2023-02-09T20:39:49Z

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).

rwgk · 2023-02-10T06:03:15Z

This PR passed global testing (internal test ID OCL:508444881:BASE:508562211:1676007777198:e805d741).

rwgk

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.

rwgk · 2023-02-10T06:13:37Z

include/pybind11/detail/internals.h

+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


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

rwgk · 2023-02-10T06:15:46Z

include/pybind11/detail/internals.h


 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();


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

Skylion007 reviewed Nov 28, 2022

View reviewed changes

cbalioglu force-pushed the nested branch from d3b54b6 to f7a90cf Compare November 28, 2022 18:22

cbalioglu commented Nov 28, 2022

View reviewed changes

Skylion007 requested review from rwgk and henryiii November 28, 2022 20:37

cbalioglu force-pushed the nested branch from 23af8e9 to 211f888 Compare February 7, 2023 16:50

Skylion007 reviewed Feb 7, 2023

View reviewed changes

cbalioglu and others added 5 commits February 9, 2023 09:32

Add support for nested exceptions in custom exceptions

e9eb4ad

Ensure that all translators are considered for nested exceptions

d068cc6

Re-organize internals.h

f51c638

style: pre-commit fixes

beaaa1c

Add constness to the function signatures

b2b6744

cbalioglu force-pushed the nested branch from d14af6c to b2b6744 Compare February 9, 2023 14:33

style: pre-commit fixes

8984743

rwgk approved these changes Feb 10, 2023

View reviewed changes

rwgk mentioned this pull request Feb 11, 2023

FWD pybind11 google/pybind11k#4366

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for nested exceptions in custom exceptions #4366

Add support for nested exceptions in custom exceptions #4366

cbalioglu commented Nov 28, 2022

Skylion007 Nov 28, 2022

cbalioglu Nov 28, 2022

Skylion007 Nov 28, 2022

cbalioglu Nov 28, 2022

cbalioglu Nov 28, 2022 •

edited

cbalioglu Nov 28, 2022

cbalioglu Nov 28, 2022

Skylion007 commented Nov 28, 2022

cbalioglu commented Nov 28, 2022

Skylion007 commented Nov 28, 2022

rwgk commented Dec 1, 2022

Skylion007 commented Jan 30, 2023

rwgk commented Feb 1, 2023

cbalioglu commented Feb 7, 2023

rwgk commented Feb 7, 2023

cbalioglu commented Feb 7, 2023

rwgk commented Feb 7, 2023

Skylion007 Feb 7, 2023

cbalioglu Feb 7, 2023

rwgk commented Feb 8, 2023

rwgk commented Feb 9, 2023

rwgk commented Feb 10, 2023

rwgk left a comment

rwgk Feb 10, 2023

rwgk Feb 10, 2023

Add support for nested exceptions in custom exceptions #4366

Are you sure you want to change the base?

Add support for nested exceptions in custom exceptions #4366

Conversation

cbalioglu commented Nov 28, 2022

Description

Suggested changelog entry:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbalioglu Nov 28, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skylion007 commented Nov 28, 2022

cbalioglu commented Nov 28, 2022

Skylion007 commented Nov 28, 2022

rwgk commented Dec 1, 2022

Skylion007 commented Jan 30, 2023

rwgk commented Feb 1, 2023

cbalioglu commented Feb 7, 2023

rwgk commented Feb 7, 2023

cbalioglu commented Feb 7, 2023

rwgk commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rwgk commented Feb 8, 2023

rwgk commented Feb 9, 2023

rwgk commented Feb 10, 2023

rwgk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbalioglu Nov 28, 2022 •

edited