-
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 all 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 |
---|---|---|
|
@@ -12,7 +12,11 @@ | |
#include "../pytypes.h" | ||
|
||
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) | ||
|
||
using ExceptionTranslator = void (*)(std::exception_ptr); | ||
|
||
PYBIND11_NAMESPACE_BEGIN(detail) | ||
|
||
// Forward declarations | ||
inline PyTypeObject *make_static_property_type(); | ||
inline PyTypeObject *make_default_metaclass(); | ||
|
@@ -100,7 +104,7 @@ struct internals { | |
std::unordered_set<std::pair<const PyObject *, const char *>, override_hash> inactive_override_cache; | ||
type_map<std::vector<bool (*)(PyObject *, void *&)>> direct_conversions; | ||
std::unordered_map<const PyObject *, std::vector<PyObject *>> patients; | ||
std::forward_list<void (*) (std::exception_ptr)> registered_exception_translators; | ||
std::forward_list<ExceptionTranslator> registered_exception_translators; | ||
std::unordered_map<std::string, void *> shared_data; // Custom data to be shared across extensions | ||
std::vector<PyObject *> loader_patient_stack; // Used by `loader_life_support` | ||
std::forward_list<std::string> static_strings; // Stores the std::strings backing detail::c_str() | ||
|
@@ -313,12 +317,25 @@ 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; | ||
|
||
// 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, | ||
// 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 { | ||
type_map<type_info *> registered_types_cpp; | ||
std::forward_list<ExceptionTranslator> registered_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 |
---|---|---|
|
@@ -66,6 +66,29 @@ | |
|
||
PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE) | ||
|
||
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; | ||
} | ||
|
||
PYBIND11_NAMESPACE_END(detail) | ||
|
||
|
||
/// Wraps an arbitrary C++ function/method/lambda function/.. into a callable Python object | ||
class cpp_function : public function { | ||
public: | ||
|
@@ -560,6 +583,7 @@ class cpp_function : public function { | |
} | ||
} | ||
|
||
|
||
/// Main dispatch logic for calls to functions bound using pybind11 | ||
static PyObject *dispatcher(PyObject *self, PyObject *args_in, PyObject *kwargs_in) { | ||
using namespace detail; | ||
|
@@ -840,8 +864,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: | ||
|
||
|
@@ -850,17 +878,15 @@ class cpp_function : public function { | |
- do nothing and let the exception fall through to the next translator, or | ||
- delegate translation to the next translator by throwing a new type of exception. */ | ||
|
||
auto last_exception = std::current_exception(); | ||
auto ®istered_exception_translators = get_internals().registered_exception_translators; | ||
for (auto& translator : registered_exception_translators) { | ||
try { | ||
translator(last_exception); | ||
} catch (...) { | ||
last_exception = std::current_exception(); | ||
continue; | ||
} | ||
auto &local_exception_translators = get_local_internals().registered_exception_translators; | ||
if (detail::apply_exception_translators(local_exception_translators)) { | ||
return nullptr; | ||
} | ||
auto &exception_translators = get_internals().registered_exception_translators; | ||
if (detail::apply_exception_translators(exception_translators)) { | ||
return nullptr; | ||
} | ||
|
||
PyErr_SetString(PyExc_SystemError, "Exception escaped from default exception translator!"); | ||
return nullptr; | ||
} | ||
|
@@ -961,6 +987,7 @@ class cpp_function : public function { | |
} | ||
}; | ||
|
||
|
||
/// Wrapper for Python extension modules | ||
class module_ : public object { | ||
public: | ||
|
@@ -1134,7 +1161,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_types_cpp[tindex] = tinfo; | ||
else | ||
internals.registered_types_cpp[tindex] = tinfo; | ||
internals.registered_types_py[(PyTypeObject *) m_ptr] = { tinfo }; | ||
|
@@ -1320,7 +1347,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_types_cpp : get_internals().registered_types_cpp; | ||
instances[std::type_index(typeid(type_alias))] = instances[std::type_index(typeid(type))]; | ||
} | ||
} | ||
|
@@ -2020,12 +2047,24 @@ template <typename InputType, typename OutputType> void implicitly_convertible() | |
pybind11_fail("implicitly_convertible: Unable to find type " + type_id<OutputType>()); | ||
} | ||
|
||
template <typename ExceptionTranslator> | ||
void register_exception_translator(ExceptionTranslator&& translator) { | ||
|
||
inline void register_exception_translator(ExceptionTranslator &&translator) { | ||
detail::get_internals().registered_exception_translators.push_front( | ||
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 module-local handlers do not deal with | ||
* the exception. | ||
*/ | ||
inline void register_local_exception_translator(ExceptionTranslator &&translator) { | ||
detail::get_local_internals().registered_exception_translators.push_front( | ||
std::forward<ExceptionTranslator>(translator)); | ||
} | ||
|
||
/** | ||
* Wrapper to generate a new Python exception type. | ||
* | ||
|
@@ -2059,22 +2098,20 @@ PYBIND11_NAMESPACE_BEGIN(detail) | |
// directly in register_exception, but that makes clang <3.5 segfault - issue #1349). | ||
template <typename CppException> | ||
exception<CppException> &get_exception_object() { static exception<CppException> ex; return ex; } | ||
PYBIND11_NAMESPACE_END(detail) | ||
|
||
/** | ||
* 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. | ||
* This is intended for simple exception translations; for more complex translation, register the | ||
* exception object and translator directly. | ||
*/ | ||
// Helper function for register_exception and register_local_exception | ||
template <typename CppException> | ||
exception<CppException> ®ister_exception(handle scope, | ||
const char *name, | ||
handle base = PyExc_Exception) { | ||
exception<CppException> ®ister_exception_impl(handle scope, | ||
const char *name, | ||
handle base, | ||
bool isLocal) { | ||
auto &ex = detail::get_exception_object<CppException>(); | ||
if (!ex) ex = exception<CppException>(scope, name, base); | ||
|
||
register_exception_translator([](std::exception_ptr p) { | ||
auto register_func = isLocal ? ®ister_local_exception_translator | ||
: ®ister_exception_translator; | ||
|
||
register_func([](std::exception_ptr p) { | ||
if (!p) return; | ||
try { | ||
std::rethrow_exception(p); | ||
|
@@ -2085,6 +2122,36 @@ exception<CppException> ®ister_exception(handle scope, | |
return ex; | ||
} | ||
|
||
PYBIND11_NAMESPACE_END(detail) | ||
|
||
/** | ||
* Registers a Python exception in `m` of the given `name` and installs a translator to | ||
* translate the C++ exception to the created Python exception using the what() method. | ||
* This is intended for simple exception translations; for more complex translation, register the | ||
* exception object and translator directly. | ||
*/ | ||
template <typename CppException> | ||
exception<CppException> ®ister_exception(handle scope, | ||
const char *name, | ||
handle base = PyExc_Exception) { | ||
return detail::register_exception_impl<CppException>(scope, name, base, false /* isLocal */); | ||
} | ||
|
||
/** | ||
* Registers a Python exception in `m` of the given `name` and installs a translator to | ||
* translate the C++ exception to the created Python exception using the what() method. | ||
* 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) { | ||
return detail::register_exception_impl<CppException>(scope, name, base, true /* isLocal */); | ||
} | ||
|
||
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.