Add and document py::error_already_set::discard_as_unraisable() #2372

jbarlow83 · 2020-08-08T10:13:32Z

To deal with exceptions that hit destructors or other noexcept functions.

Supercedes #2358

virtuald

LGTM

jbarlow83 · 2020-08-09T05:49:06Z

Added a missing documentation file I edited and forgot to include, and fixed Python 2.7 support. I believe the current CI errors are due to recent changes in the CMake setup.

YannickJadoul

Nice addition :-) Thanks for taking the time to work on this, @jbarlow83!
I personally don't need this in my project, I believe, so I don't have problems with keeping this a very thin/minor PR (mostly in the docs), but I'm wondering why we're not including the other ideas (e.g., the lambda-wrapping utility function)?

YannickJadoul · 2020-08-10T12:37:29Z

docs/advanced/classes.rst

+of type :class:`error_already_set`. If this error is thrown out of a class destructor,
+``std::terminate()`` will be called, terminating the process. Class destructors
+must catch all exceptions of type :class:`error_already_set` to discard the Python
+exception using :meth:`error_already_set::discard_as_unraisable`. (In addition,


Minor thing: I don't think the parentheses are needed here. This is important enough to realize :-)
Another minor thing: Maybe mention things like py::cast_error, etc, that are also C++ errors and not caught by py::error_already_set?
Major thing: reading this, I wonder "what do you do then, for C++ excecptions? Maybe we should still provide a wrapper function such that users can do this without calling the C API?

Do you mean std::terminate() -> std::terminate?

I think classes.rst needs a bigger overhaul than this but was trying to limit the scope of changes for now. In particular, py::error_already_set serves a different function from the C++ exception wrappers, and needs to be separately explained separately from them and then an example added that brings the two concepts together. In particular, I find it pretty unintuitive that if you have a Python -> C++ -> Python callback situation, and you throw ValueError from Python, you can't catch py::value_error. You can only catch py::value_error when another C++ function throws it.

I think we could add py::builtin_exception::discard_as_unraisable. It's a little trickier to convert a std::exception into a Python exception, because I don't think we should invoke the exception translator as part of dealing with an unraisable exception.

Do you mean std::terminate() -> std::terminate?

No, sorry, I meant the parentheses around "In addition, ...".

I think classes.rst needs a bigger overhaul than this but was trying to limit the scope of changes for now. In particular, py::error_already_set serves a different function from the C++ exception wrappers, and needs to be separately explained separately from them and then an example added that brings the two concepts together.

We could add py::builtin_exception::discard_as_unraisable. It's a little trickier to convert a std::exception into a Python exception, because I don't think we should invoke the exception translator as part of dealing with an unraisable exception.

Alright, yes, I can see. In that case, this is definitely better than what we had, so more changes can come later! :-)

docs/advanced/classes.rst

docs/advanced/exceptions.rst

YannickJadoul

Right. If this is ready, I'll happily press "merge"!

EDIT: Oh, wait. First CI needs to be fixed. But I'm guessing that's linked to #2381?

jbarlow83 · 2020-08-11T19:32:22Z

@YannickJadoul CI is fixed

YannickJadoul · 2020-08-11T20:23:52Z

@jbarlow83 Great! So, ready?

Let's maybe still get a quick review from a few others. @bstaletic, @henryiii, @rwgk, and/or @EricCousineau-TRI, anything to add/change?

jbarlow83 · 2020-08-12T20:14:25Z

Yes, should be ready

henryiii

Just curious, could this be used to help with #2383? The problem there is similar, pybind11 tries to call __repr__ and if that throws, it crashes with an error_already_set.

jbarlow83 · 2020-08-12T20:42:26Z

@henryiii #2383 is different, added a comment there.

rwgk

Thanks, looks great to me. Just a couple minor suggestions.

rwgk · 2020-08-15T07:53:31Z

include/pybind11/pytypes.h

+        PyErr_WriteUnraisable(err_context.ptr());
+    }
+    void discard_as_unraisable(const char *err_context) {
+        auto obj = reinterpret_steal<object>(PYBIND11_FROM_STRING(err_context));


discard_as_unraisable(reinterpret_steal<object>(PYBIND11_FROM_STRING(err_context)));
?
to avoid code duplication.

Sounds good. BTW reason for not calling str here is that it is defined before str is.

Hmmm, guess there's no way to forward declare that?
I suppose it's not worth the effort to try rearranging things :-/

rwgk · 2020-08-15T08:00:31Z

tests/test_exceptions.py

+        assert triggered[0] is True
+
+    captured = capsys.readouterr()
+    captured_err = captured[1]  # Python 3.5 doesn't like .err the namedtuple


# captured.err does not work with Python 3.5
?

The captured object is a namedtuple or similar object with a namedtuple-like interface.

We'd want to say captured.err here, but that failed in CI on Python 3.5 on Debian and nowhere else. Perhaps it's specific to a particular minor revision. I'll fix the typo.

rwgk · 2020-08-15T18:05:56Z

tests/test_exceptions.py

+    if hooked:
+        assert triggered[0] is True
+
+    captured_err = capsys.readouterr()[1]  # Returns (stdout, syserr)


typo? Should this be stderr?
If you fix that I'll merge this PR, we have 4 approvals already.

FWIW If capsys.readouterr() always returns that tuple, then consider doing tuple unpacking:

_, captures_err = capsys.readouterr()

@virtuald

To deal with exceptions that hit destructors or other noexcept functions. Includes fixes to support Python 2.7 and extends documentation on error handling. @virtuald and @YannickJadoul both contributed to this PR.

rwgk · 2020-08-16T17:04:53Z

Hi @jbarlow83, thanks for the contribution! I'll merge this now.

virtuald approved these changes Aug 8, 2020

View reviewed changes

jbarlow83 force-pushed the pr branch from 8d1e164 to dad5920 Compare August 9, 2020 05:35

YannickJadoul reviewed Aug 10, 2020

View reviewed changes

jbarlow83 force-pushed the pr branch 4 times, most recently from ac4a891 to 9ca68ca Compare August 11, 2020 09:00

YannickJadoul approved these changes Aug 11, 2020

View reviewed changes

jbarlow83 force-pushed the pr branch 3 times, most recently from 05ca399 to 5437330 Compare August 11, 2020 18:54

YannickJadoul requested review from rwgk, henryiii, bstaletic and EricCousineau-TRI August 11, 2020 20:24

henryiii approved these changes Aug 12, 2020

View reviewed changes

rwgk approved these changes Aug 15, 2020

View reviewed changes

jbarlow83 force-pushed the pr branch from 5437330 to ec16c2f Compare August 15, 2020 08:33

rwgk approved these changes Aug 15, 2020

View reviewed changes

Add and document py::error_already_set::discard_as_unraisable()

7fd5736

To deal with exceptions that hit destructors or other noexcept functions. Includes fixes to support Python 2.7 and extends documentation on error handling. @virtuald and @YannickJadoul both contributed to this PR.

jbarlow83 force-pushed the pr branch from ec16c2f to 7fd5736 Compare August 16, 2020 16:35

rwgk merged commit 3618bea into pybind:master Aug 16, 2020

jbarlow83 deleted the pr branch August 16, 2020 21:59

YannickJadoul mentioned this pull request Aug 19, 2020

Use PyErr_WriteUnraisable when an error occurs in a destructor #2358

Closed

YannickJadoul mentioned this pull request Aug 21, 2020

Improve documentation of Python and C++ exceptions #2408

Merged

mkuron mentioned this pull request Dec 21, 2020

Intel ICC C++17 compatibility #2729

Merged

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#2372

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add and document py::error_already_set::discard_as_unraisable() #2372

Add and document py::error_already_set::discard_as_unraisable() #2372

jbarlow83 commented Aug 8, 2020

virtuald left a comment

jbarlow83 commented Aug 9, 2020

YannickJadoul left a comment

YannickJadoul Aug 10, 2020

jbarlow83 Aug 10, 2020 •

edited

Loading

YannickJadoul Aug 10, 2020

YannickJadoul left a comment •

edited

Loading

jbarlow83 commented Aug 11, 2020

YannickJadoul commented Aug 11, 2020

jbarlow83 commented Aug 12, 2020

henryiii left a comment

jbarlow83 commented Aug 12, 2020

rwgk left a comment

rwgk Aug 15, 2020

jbarlow83 Aug 15, 2020

YannickJadoul Aug 15, 2020

rwgk Aug 15, 2020

jbarlow83 Aug 15, 2020

rwgk Aug 15, 2020

EricCousineau-TRI Aug 15, 2020

rwgk commented Aug 16, 2020

Add and document py::error_already_set::discard_as_unraisable() #2372

Add and document py::error_already_set::discard_as_unraisable() #2372

Conversation

jbarlow83 commented Aug 8, 2020

virtuald left a comment

Choose a reason for hiding this comment

jbarlow83 commented Aug 9, 2020

YannickJadoul left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbarlow83 Aug 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

YannickJadoul left a comment • edited Loading

Choose a reason for hiding this comment

jbarlow83 commented Aug 11, 2020

YannickJadoul commented Aug 11, 2020

jbarlow83 commented Aug 12, 2020

henryiii left a comment

Choose a reason for hiding this comment

jbarlow83 commented Aug 12, 2020

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

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

Choose a reason for hiding this comment

rwgk commented Aug 16, 2020

jbarlow83 Aug 10, 2020 •

edited

Loading

YannickJadoul left a comment •

edited

Loading