From 1682b67326dcf44c2b221f664c219871f22d166a Mon Sep 17 00:00:00 2001 From: Jason Rhinelander Date: Thu, 20 Jul 2017 23:14:33 -0400 Subject: [PATCH] Simplify error_already_set `error_already_set` is more complicated than it needs to be, partly because it manages reference counts itself rather than using `py::object`, and partly because it tries to do more exception clearing than is needed. This commit greatly simplifies it, and fixes #927. Using `py::object` instead of `PyObject *` means we can rely on implicit copy/move constructors. The current logic did both a `PyErr_Clear` on deletion *and* a `PyErr_Fetch` on creation. I can't see how the `PyErr_Clear` on deletion is ever useful: the `Fetch` on creation itself clears the error, so the only way doing a `PyErr_Clear` on deletion could do anything if is some *other* exception was raised while the `error_already_set` object was alive--but in that case, clearing some other exception seems wrong. (Code that is worried about an exception handler raising another exception would already catch a second `error_already_set` from exception code). The destructor itself called `clear()`, but `clear()` was a little bit more paranoid that needed: it called `restore()` to restore the currently captured error, but then immediately cleared it, using the `PyErr_Restore` to release the references. That's unnecessary: it's valid for us to release the references manually. This updates the code to simply release the references on the three objects (preserving the gil acquire). `clear()`, however, also had the side effect of clearing the current error, even if the current `error_already_set` didn't have a current error (e.g. because of a previous `restore()` or `clear()` call). I don't really see how clearing the error here can ever actually be useful: the only way the current error could be set is if you called `restore()` (in which case the current stored error-related members have already been released), or if some *other* code raised the error, in which case `clear()` on *this* object is clearing an error for which it shouldn't be responsible. Neither of those seem like intentional or desirable features, and manually requesting deletion of the stored references similarly seems pointless, so I've just made `clear()` an empty method and marked it deprecated. This also fixes a minor potential issue with the destruction: it is technically possible for `value` to be null (though this seems likely to be rare in practice); this updates the check to look at `type` which will always be non-null for a `Fetch`ed exception. This also adds error_already_set round-trip throw tests to the test suite. --- include/pybind11/common.h | 34 ---------------------------- include/pybind11/embed.h | 1 - include/pybind11/pybind11.h | 6 +++-- include/pybind11/pytypes.h | 36 ++++++++++++++++++++++++++++++ tests/test_exceptions.cpp | 17 ++++++++++---- tests/test_exceptions.py | 44 +++++++++++++++++++++++++++++++++++++ 6 files changed, 97 insertions(+), 41 deletions(-) diff --git a/include/pybind11/common.h b/include/pybind11/common.h index b205bb9d8..d0cc2d6e0 100644 --- a/include/pybind11/common.h +++ b/include/pybind11/common.h @@ -231,7 +231,6 @@ extern "C" { try { \ return pybind11_init(); \ } catch (pybind11::error_already_set &e) { \ - e.clear(); \ PyErr_SetString(PyExc_ImportError, e.what()); \ return nullptr; \ } catch (const std::exception &e) { \ @@ -278,7 +277,6 @@ extern "C" { pybind11_init_##name(m); \ return m.ptr(); \ } catch (pybind11::error_already_set &e) { \ - e.clear(); \ PyErr_SetString(PyExc_ImportError, e.what()); \ return nullptr; \ } catch (const std::exception &e) { \ @@ -353,8 +351,6 @@ inline static constexpr int log2(size_t n, int k = 0) { return (n <= 1) ? k : lo // Returns the size as a multiple of sizeof(void *), rounded up. inline static constexpr size_t size_in_ptrs(size_t s) { return 1 + ((s - 1) >> log2(sizeof(void *))); } -inline std::string error_string(); - /** * The space to allocate for simple layout instance holders (see below) in multiple of the size of * a pointer (e.g. 2 means 16 bytes on 64-bit architectures). The default is the minimum required @@ -703,36 +699,6 @@ template T& get_or_create_shared_data(const std::string& name) { return *ptr; } -/// Fetch and hold an error which was already set in Python -class error_already_set : public std::runtime_error { -public: - error_already_set() : std::runtime_error(detail::error_string()) { - PyErr_Fetch(&type, &value, &trace); - } - - error_already_set(const error_already_set &) = delete; - - error_already_set(error_already_set &&e) - : std::runtime_error(e.what()), type(e.type), value(e.value), - trace(e.trace) { e.type = e.value = e.trace = nullptr; } - - inline ~error_already_set(); // implementation in pybind11.h - - error_already_set& operator=(const error_already_set &) = delete; - - /// Give the error back to Python - void restore() { PyErr_Restore(type, value, trace); type = value = trace = nullptr; } - - /// Clear the held Python error state (the C++ `what()` message remains intact) - void clear() { restore(); PyErr_Clear(); } - - /// Check if the trapped exception matches a given Python exception class - bool matches(PyObject *ex) const { return PyErr_GivenExceptionMatches(ex, type); } - -private: - PyObject *type, *value, *trace; -}; - /// C++ bindings of builtin Python exceptions class builtin_exception : public std::runtime_error { public: diff --git a/include/pybind11/embed.h b/include/pybind11/embed.h index a3f6a4e67..0eb656b0c 100644 --- a/include/pybind11/embed.h +++ b/include/pybind11/embed.h @@ -51,7 +51,6 @@ pybind11_init_##name(m); \ return m.ptr(); \ } catch (pybind11::error_already_set &e) { \ - e.clear(); \ PyErr_SetString(PyExc_ImportError, e.what()); \ return nullptr; \ } catch (const std::exception &e) { \ diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 079d35457..21153be0b 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -1758,9 +1758,11 @@ class gil_scoped_release { }; #endif error_already_set::~error_already_set() { - if (value) { + if (type) { gil_scoped_acquire gil; - clear(); + type.release().dec_ref(); + value.release().dec_ref(); + trace.release().dec_ref(); } } diff --git a/include/pybind11/pytypes.h b/include/pybind11/pytypes.h index cc48bbbff..095d40f1d 100644 --- a/include/pybind11/pytypes.h +++ b/include/pybind11/pytypes.h @@ -279,6 +279,42 @@ template T reinterpret_borrow(handle h) { return {h, object::borrow \endrst */ template T reinterpret_steal(handle h) { return {h, object::stolen_t{}}; } +NAMESPACE_BEGIN(detail) +inline std::string error_string(); +NAMESPACE_END(detail) + +/// Fetch and hold an error which was already set in Python. An instance of this is typically +/// thrown to propagate python-side errors back through C++ which can either be caught manually or +/// else falls back to the function dispatcher (which then raises the captured error back to +/// python). +class error_already_set : public std::runtime_error { +public: + /// Constructs a new exception from the current Python error indicator, if any. The current + /// Python error indicator will be cleared. + error_already_set() : std::runtime_error(detail::error_string()) { + PyErr_Fetch(&type.ptr(), &value.ptr(), &trace.ptr()); + } + + inline ~error_already_set(); + + /// Give the currently-held error back to Python, if any. If there is currently a Python error + /// already set it is cleared first. After this call, the current object no longer stores the + /// error variables (but the `.what()` string is still available). + void restore() { PyErr_Restore(type.release().ptr(), value.release().ptr(), trace.release().ptr()); } + + // Does nothing; provided for backwards compatibility. + PYBIND11_DEPRECATED("Use of error_already_set.clear() is deprecated") + void clear() {} + + /// Check if the currently trapped error type matches the given Python exception class (or a + /// subclass thereof). May also be passed a tuple to search for any exception class matches in + /// the given tuple. + bool matches(handle ex) const { return PyErr_GivenExceptionMatches(ex.ptr(), type.ptr()); } + +private: + object type, value, trace; +}; + /** \defgroup python_builtins _ Unless stated otherwise, the following C++ functions behave the same as their Python counterparts. diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 7cfc5762b..ae28abb48 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -120,10 +120,7 @@ TEST_SUBMODULE(exceptions, m) { py::dict foo; try { foo["bar"]; } catch (py::error_already_set& ex) { - if (ex.matches(PyExc_KeyError)) - ex.clear(); - else - throw; + if (!ex.matches(PyExc_KeyError)) throw; } }); @@ -156,4 +153,16 @@ TEST_SUBMODULE(exceptions, m) { } return false; }); + + // test_nested_throws + m.def("try_catch", [m](py::object exc_type, py::function f, py::args args) { + try { f(*args); } + catch (py::error_already_set &ex) { + if (ex.matches(exc_type)) + py::print(ex.what()); + else + throw; + } + }); + } diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index cc4baaa09..06d442e67 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -76,3 +76,47 @@ def test_custom(msg): except m.MyException5_1: raise RuntimeError("Exception error: caught child from parent") assert msg(excinfo.value) == "this is a helper-defined translated exception" + + +def test_nested_throws(capture): + """Tests nested (e.g. C++ -> Python -> C++) exception handling""" + + def throw_myex(): + raise m.MyException("nested error") + + def throw_myex5(): + raise m.MyException5("nested error 5") + + # In the comments below, the exception is caught in the first step, thrown in the last step + + # C++ -> Python + with capture: + m.try_catch(m.MyException5, throw_myex5) + assert str(capture).startswith("MyException5: nested error 5") + + # Python -> C++ -> Python + with pytest.raises(m.MyException) as excinfo: + m.try_catch(m.MyException5, throw_myex) + assert str(excinfo.value) == "nested error" + + def pycatch(exctype, f, *args): + try: + f(*args) + except m.MyException as e: + print(e) + + # C++ -> Python -> C++ -> Python + with capture: + m.try_catch( + m.MyException5, pycatch, m.MyException, m.try_catch, m.MyException, throw_myex5) + assert str(capture).startswith("MyException5: nested error 5") + + # C++ -> Python -> C++ + with capture: + m.try_catch(m.MyException, pycatch, m.MyException5, m.throws4) + assert capture == "this error is rethrown" + + # Python -> C++ -> Python -> C++ + with pytest.raises(m.MyException5) as excinfo: + m.try_catch(m.MyException, pycatch, m.MyException, m.throws5) + assert str(excinfo.value) == "this is a helper-defined translated exception"