-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Feature/local exception translator #2650
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
Changes from 12 commits
33744b0
a0af4c3
55c1023
f44ce2e
3818f36
7099f26
08e9977
5545b1f
ae9cec5
696ece0
bef0832
2f2bd18
dfdfe66
f9d0f6f
e0b6dcd
d39b6e3
0e6a331
6bf85a5
33ccf64
63ab723
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -75,9 +75,10 @@ Registering custom translators | |
|
||
If the default exception conversion policy described above is insufficient, | ||
pybind11 also provides support for registering custom exception translators. | ||
To register a simple exception conversion that translates a C++ exception into | ||
a new Python exception using the C++ exception's ``what()`` method, a helper | ||
function is available: | ||
Similar to pybind11 classes, exception translators can be local to the module | ||
they are defined in or global to the entire python session. To register a simple | ||
exception conversion that translates a C++ exception into a new Python exception | ||
using the C++ exception's ``what()`` method, a helper function is available: | ||
|
||
.. code-block:: cpp | ||
|
||
|
@@ -87,29 +88,39 @@ This call creates a Python exception class with the name ``PyExp`` in the given | |
module and automatically converts any encountered exceptions of type ``CppExp`` | ||
into Python exceptions of type ``PyExp``. | ||
|
||
A matching function is available for registering a local exception translator: | ||
|
||
.. code-block:: cpp | ||
|
||
py::register_local_exception<CppExp>(module, "PyExp"); | ||
|
||
|
||
It is possible to specify base class for the exception using the third | ||
parameter, a `handle`: | ||
|
||
.. code-block:: cpp | ||
|
||
py::register_exception<CppExp>(module, "PyExp", PyExc_RuntimeError); | ||
py::register_local_exception<CppExp>(module, "PyExp", PyExc_RuntimeError); | ||
YannickJadoul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
Then `PyExp` can be caught both as `PyExp` and `RuntimeError`. | ||
|
||
The class objects of the built-in Python exceptions are listed in the Python | ||
documentation on `Standard Exceptions <https://docs.python.org/3/c-api/exceptions.html#standard-exceptions>`_. | ||
The default base class is `PyExc_Exception`. | ||
|
||
When more advanced exception translation is needed, the function | ||
``py::register_exception_translator(translator)`` can be used to register | ||
When more advanced exception translation is needed, the functions | ||
``py::register_exception_translator(translator)`` and | ||
``py::register_local_exception_translator(translator)`` can be used to register | ||
functions that can translate arbitrary exception types (and which may include | ||
additional logic to do so). The function takes a stateless callable (e.g. a | ||
additional logic to do so). The functions takes a stateless callable (e.g. a | ||
function pointer or a lambda function without captured variables) with the call | ||
signature ``void(std::exception_ptr)``. | ||
|
||
When a C++ exception is thrown, the registered exception translators are tried | ||
in reverse order of registration (i.e. the last registered translator gets the | ||
first shot at handling the exception). | ||
first shot at handling the exception). All local translators will be tried | ||
before a global translator is tried. | ||
|
||
Inside the translator, ``std::rethrow_exception`` should be used within | ||
a try block to re-throw the exception. One or more catch clauses to catch | ||
|
@@ -168,6 +179,56 @@ section. | |
with ``-fvisibility=hidden``. Therefore exceptions that are used across ABI boundaries need to be explicitly exported, as exercised in ``tests/test_exceptions.h``. | ||
See also: "Problems with C++ exceptions" under `GCC Wiki <https://gcc.gnu.org/wiki/Visibility>`_. | ||
|
||
|
||
Local vs Global Exception Translators | ||
===================================== | ||
|
||
Similar to how the ``py::module_local`` flag allows uesrs to limit a bound class | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This first sentence seems misplaced and the grammar incomplete. I think it reads better if you simply remove it here. The section title provides sufficient context. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. removed |
||
to a single compiled module to prevent name collisions. When a global exception | ||
YannickJadoul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
translator is registered, it will be applied across all modules in the reverse | ||
order of registration. This can create behavior where the order of module import | ||
influences how exceptions are translated. | ||
|
||
If module1 has the following translator: | ||
|
||
.. code-block:: cpp | ||
|
||
py::register_exception_translator([](std::exception_ptr p) { | ||
try { | ||
if (p) std::rethrow_exception(p); | ||
} catch (const std::invalid_argument &e) { | ||
PyErr_SetString("module1 handled this") | ||
} | ||
} | ||
|
||
and module2 has the following similar translator: | ||
|
||
.. code-block:: cpp | ||
|
||
py::register_exception_translator([](std::exception_ptr p) { | ||
try { | ||
if (p) std::rethrow_exception(p); | ||
} catch (const std::invalid_argument &e) { | ||
PyErr_SetString("module2 handled this") | ||
} | ||
} | ||
|
||
then which translator handles the invalid_argument will be determined by the | ||
order that module1 and module2 are imported. Since exception translators are | ||
applied in the reverse order of registration, which ever module was imported | ||
last will "win" and that translator will be applied. | ||
|
||
If there are multiple pybind11 modules that share exception types (either | ||
standard built-in or custom) loaded into a single python instance and | ||
consistent error handling behavior is needed, then local translators should be | ||
used. | ||
|
||
Changing the previous example to use register_local_exception_translator would | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. double backquotes (code) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
mean that when invalid_argument is thrown in the module2 code, the module2 | ||
translator will always handle it, while in module1, the module1 translator will | ||
do the same. | ||
rwgk marked this conversation as resolved.
Show resolved
Hide resolved
|
||
>>>>>>> 854aa95a (Update documentation for new local exception feature) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accident resolving merge conflict? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. whoops |
||
|
||
.. _handling_python_exceptions_cpp: | ||
|
||
Handling exceptions from Python in C++ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -313,12 +313,19 @@ PYBIND11_NOINLINE inline internals &get_internals() { | |
return **internals_pp; | ||
} | ||
|
||
/// Works like `internals.registered_types_cpp`, but for module-local registered types: | ||
inline type_map<type_info *> ®istered_local_types_cpp() { | ||
static type_map<type_info *> locals{}; | ||
return locals; | ||
|
||
struct local_internals { | ||
type_map<type_info *> registered_local_types_cpp; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Answering the question you asked in a different comment: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Names have been changed, since everyone seems to prefer them without the |
||
std::forward_list<void (*) (std::exception_ptr)> registered_local_exception_translators; | ||
}; | ||
|
||
/// Works like `get_internals`, but for things which are locally registered. | ||
inline local_internals &get_local_internals() { | ||
static local_internals locals; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did you think through the consequences of mixing extensions built with e.g. pybind11 2.7.0 and 2.7.1 (assuming it includes this change)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thought process on this (and I'm definitely not an expert in all the intricacies of pybind11 ABI compatibility) is that the internals struct in pybind11 is shared between all the modules. Any changes to that definitely requires a lot of careful thought about whether backwards compatibility is being broken. The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make total sense to me, but I don't consider myself an authority on this, too. Adding this to the PR description is to expose it, in case someone is able to poke a hole into the rationale, we want to know asap. |
||
return locals; | ||
YannickJadoul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
|
||
/// 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 | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -840,8 +840,12 @@ class cpp_function : public function { | |
#endif | ||
} catch (...) { | ||
/* When an exception is caught, give each registered exception | ||
translator a chance to translate it to a Python exception | ||
in reverse order of registration. | ||
translator a chance to translate it to a Python exception. First | ||
all module-local translators will be tried in reverse order of | ||
registration. If none of the module-locale translators handle | ||
the exception (or there are no module-locale translators) then | ||
the global translators will be tried, also in reverse order of | ||
registration. | ||
|
||
A translator may choose to do one of the following: | ||
|
||
|
@@ -851,6 +855,18 @@ class cpp_function : public function { | |
- delegate translation to the next translator by throwing a new type of exception. */ | ||
|
||
auto last_exception = std::current_exception(); | ||
|
||
YannickJadoul marked this conversation as resolved.
Show resolved
Hide resolved
|
||
auto & registered_local_exception_translators = get_local_internals().registered_local_exception_translators; | ||
for (auto& translator : registered_local_exception_translators) { | ||
try { | ||
translator(last_exception); | ||
} catch (...) { | ||
last_exception = std::current_exception(); | ||
continue; | ||
} | ||
return nullptr; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is slightly repetitive, but fixing it might be more annoying than keeping the code duplication. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The sort of thing that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is too much duplication, and I assume it's easy to move this to a helper function. Could you please try that? (Even if the number of lines of code is similar in the end, it's worth it IMO. Duplication easily leads to accidents when working on maintenance changes.) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this duplication has been removed and made into a helper. Let me know if you don't like something about my implementation. |
||
|
||
auto ®istered_exception_translators = get_internals().registered_exception_translators; | ||
for (auto& translator : registered_exception_translators) { | ||
try { | ||
|
@@ -1134,7 +1150,7 @@ class generic_type : public object { | |
auto tindex = std::type_index(*rec.type); | ||
tinfo->direct_conversions = &internals.direct_conversions[tindex]; | ||
if (rec.module_local) | ||
registered_local_types_cpp()[tindex] = tinfo; | ||
get_local_internals().registered_local_types_cpp[tindex] = tinfo; | ||
else | ||
internals.registered_types_cpp[tindex] = tinfo; | ||
internals.registered_types_py[(PyTypeObject *) m_ptr] = { tinfo }; | ||
|
@@ -1320,7 +1336,7 @@ class class_ : public detail::generic_type { | |
generic_type::initialize(record); | ||
|
||
if (has_alias) { | ||
auto &instances = record.module_local ? registered_local_types_cpp() : get_internals().registered_types_cpp; | ||
auto &instances = record.module_local ? get_local_internals().registered_local_types_cpp : get_internals().registered_types_cpp; | ||
instances[std::type_index(typeid(type_alias))] = instances[std::type_index(typeid(type))]; | ||
} | ||
} | ||
|
@@ -2026,6 +2042,19 @@ void register_exception_translator(ExceptionTranslator&& translator) { | |
std::forward<ExceptionTranslator>(translator)); | ||
} | ||
|
||
|
||
/** | ||
* Add a new module-local exception translator. Locally registered functions | ||
* will be tried before any globally registered exception translators, which | ||
* will only be invoked if the moduke-local handlers do not deal with | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo (strange that codespell didn't get this one!?) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fixed |
||
* the exception. | ||
*/ | ||
template <typename ExceptionTranslator> | ||
void register_local_exception_translator(ExceptionTranslator&& translator) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe, unfortunately this duplicates something that's not great in the existing code: there is zero wiggle room for what There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay, I did this. I did not change any of the type information, because I'm pretty sure that would be a PYBIND11_INTERNALS_VERSION ABI breaking change.
not
as it did in your PR. But I removed all the templating from the register functions. I also turned There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, perfect, thanks. I did not mean to suggest adding the const. My PR was just to record that I thought about it carefully and then discarded the idea because I think it's too disruptive for too little gain. |
||
detail::get_local_internals().registered_local_exception_translators.push_front( | ||
std::forward<ExceptionTranslator>(translator)); | ||
} | ||
|
||
/** | ||
* Wrapper to generate a new Python exception type. | ||
* | ||
|
@@ -2085,6 +2114,32 @@ exception<CppException> ®ister_exception(handle scope, | |
return ex; | ||
} | ||
|
||
/** | ||
* Registers a Python exception in `m` of the given `name` and installs an exception translator to | ||
* translate the C++ exception to the created Python exception using the exceptions what() method. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. delete There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done. |
||
* This translator will only be used for exceptions that are thrown in this module and will be | ||
* tried before global exception translators, including those registered with register_exception. | ||
* This is intended for simple exception translations; for more complex translation, register the | ||
* exception object and translator directly. | ||
*/ | ||
template <typename CppException> | ||
exception<CppException> ®ister_local_exception(handle scope, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Other reviewers: should this be merged with How do we best do this? An auxiliary helper with an extra runtime or template argument, and keep the current API with 2 functions? Or do we also want to merge these two functions in the API (in a backwards-compatible way, with a default argument at the end of the argument list or template arguments) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, absolutely, this is too much duplication. |
||
const char *name, | ||
handle base = PyExc_Exception) { | ||
auto &ex = detail::get_exception_object<CppException>(); | ||
if (!ex) ex = exception<CppException>(scope, name, base); | ||
|
||
register_local_exception_translator([](std::exception_ptr p) { | ||
if (!p) return; | ||
try { | ||
std::rethrow_exception(p); | ||
} catch (const CppException &e) { | ||
detail::get_exception_object<CppException>()(e.what()); | ||
} | ||
}); | ||
return ex; | ||
} | ||
|
||
PYBIND11_NAMESPACE_BEGIN(detail) | ||
PYBIND11_NOINLINE inline void print(const tuple &args, const dict &kwargs) { | ||
auto strings = tuple(args.size()); | ||
|
Uh oh!
There was an error while loading. Please reload this page.