From 135ba8deafb8bf64a15b24d1513899eb600e2011 Mon Sep 17 00:00:00 2001 From: Dean Moldovan Date: Sat, 10 Sep 2016 11:58:02 +0200 Subject: [PATCH] Make error_already_set fetch and hold the Python error This clears the Python error at the error_already_set throw site, thus allowing Python calls to be made in destructors which are triggered by the exception. This is preferable to the alternative, which would be guarding every Python API call with an error_scope. This effectively flips the behavior of error_already_set. Previously, it was assumed that the error stays in Python, so handling the exception in C++ would require explicitly calling PyErr_Clear(), but nothing was needed to propagate the error to Python. With this change, handling the error in C++ does not require a PyErr_Clear() call, but propagating the error to Python requires an explicit error_already_set::restore(). The change does not break old code which explicitly calls PyErr_Clear() for cleanup, which should be the majority of user code. The need for an explicit restore() call does break old code, but this should be mostly confined to the library and not user code. --- include/pybind11/cast.h | 2 +- include/pybind11/common.h | 16 +++++++++++++++- include/pybind11/eigen.h | 1 - include/pybind11/pybind11.h | 4 ++-- tests/test_eval.cpp | 1 - tests/test_exceptions.cpp | 18 ++++++++++++++++++ tests/test_exceptions.py | 8 ++++++++ 7 files changed, 44 insertions(+), 6 deletions(-) diff --git a/include/pybind11/cast.h b/include/pybind11/cast.h index 638e4245f..8741498d5 100644 --- a/include/pybind11/cast.h +++ b/include/pybind11/cast.h @@ -53,7 +53,7 @@ PYBIND11_NOINLINE inline internals &get_internals() { [](std::exception_ptr p) -> void { try { if (p) std::rethrow_exception(p); - } catch (const error_already_set &) { return; + } catch (error_already_set &e) { e.restore(); return; } catch (const index_error &e) { PyErr_SetString(PyExc_IndexError, e.what()); return; } catch (const key_error &e) { PyErr_SetString(PyExc_KeyError, e.what()); return; } catch (const value_error &e) { PyErr_SetString(PyExc_ValueError, e.what()); return; diff --git a/include/pybind11/common.h b/include/pybind11/common.h index ef92daff7..cd0742980 100644 --- a/include/pybind11/common.h +++ b/include/pybind11/common.h @@ -384,6 +384,21 @@ inline void ignore_unused(const int *) { } NAMESPACE_END(detail) +/// 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() { Py_XDECREF(type); Py_XDECREF(value); Py_XDECREF(trace); } + + /// Give the error back to Python + void restore() { PyErr_Restore(type, value, trace); type = value = trace = nullptr; } + +private: + PyObject *type, *value, *trace; +}; + #define PYBIND11_RUNTIME_EXCEPTION(name) \ class name : public std::runtime_error { public: \ name(const std::string &w) : std::runtime_error(w) { }; \ @@ -392,7 +407,6 @@ NAMESPACE_END(detail) }; // C++ bindings of core Python exceptions -class error_already_set : public std::runtime_error { public: error_already_set() : std::runtime_error(detail::error_string()) {} }; PYBIND11_RUNTIME_EXCEPTION(stop_iteration) PYBIND11_RUNTIME_EXCEPTION(index_error) PYBIND11_RUNTIME_EXCEPTION(key_error) diff --git a/include/pybind11/eigen.h b/include/pybind11/eigen.h index 63a48088a..c0925a7f3 100644 --- a/include/pybind11/eigen.h +++ b/include/pybind11/eigen.h @@ -217,7 +217,6 @@ struct type_caster::value>:: try { obj = matrix_type(obj); } catch (const error_already_set &) { - PyErr_Clear(); return false; } } diff --git a/include/pybind11/pybind11.h b/include/pybind11/pybind11.h index 4a50b3353..24f10e379 100644 --- a/include/pybind11/pybind11.h +++ b/include/pybind11/pybind11.h @@ -426,7 +426,8 @@ protected: if (result.ptr() != PYBIND11_TRY_NEXT_OVERLOAD) break; } - } catch (const error_already_set &) { + } catch (error_already_set &e) { + e.restore(); return nullptr; } catch (...) { /* When an exception is caught, give each registered exception @@ -1309,7 +1310,6 @@ NAMESPACE_END(detail) template void print(Args &&...args) { - error_scope scope; // Preserve error state auto c = detail::collect_arguments(std::forward(args)...); detail::print(c.args(), c.kwargs()); } diff --git a/tests/test_eval.cpp b/tests/test_eval.cpp index 45d811d74..ed4c226fe 100644 --- a/tests/test_eval.cpp +++ b/tests/test_eval.cpp @@ -63,7 +63,6 @@ test_initializer eval([](py::module &m) { try { py::eval("nonsense code ..."); } catch (py::error_already_set &) { - PyErr_Clear(); return true; } return false; diff --git a/tests/test_exceptions.cpp b/tests/test_exceptions.cpp index 67133a637..613b1355f 100644 --- a/tests/test_exceptions.cpp +++ b/tests/test_exceptions.cpp @@ -66,6 +66,13 @@ void throws_logic_error() { throw std::logic_error("this error should fall through to the standard handler"); } +struct PythonCallInDestructor { + PythonCallInDestructor(const py::dict &d) : d(d) {} + ~PythonCallInDestructor() { d["good"] = py::cast(true); } + + py::dict d; +}; + test_initializer custom_exceptions([](py::module &m) { // make a new custom exception and use it as a translation target static py::exception ex(m, "MyException"); @@ -123,4 +130,15 @@ test_initializer custom_exceptions([](py::module &m) { PyErr_SetString(PyExc_ValueError, "foo"); throw py::error_already_set(); }); + + m.def("python_call_in_destructor", [](py::dict d) { + try { + PythonCallInDestructor set_dict_in_destructor(d); + PyErr_SetString(PyExc_ValueError, "foo"); + throw py::error_already_set(); + } catch (const py::error_already_set&) { + return true; + } + return false; + }); }); diff --git a/tests/test_exceptions.py b/tests/test_exceptions.py index a92300c32..4048d43f5 100644 --- a/tests/test_exceptions.py +++ b/tests/test_exceptions.py @@ -13,6 +13,14 @@ def test_error_already_set(msg): assert msg(excinfo.value) == "foo" +def test_python_call_in_catch(): + from pybind11_tests import python_call_in_destructor + + d = {} + assert python_call_in_destructor(d) is True + assert d["good"] is True + + def test_custom(msg): from pybind11_tests import (MyException, throws1, throws2, throws3, throws4, throws_logic_error)