Skip to content

Use PyErr_WriteUnraisable when an error occurs in a destructor #2358

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Use PyErr_WriteUnraisable when an error occurs in a destructor #2358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1406,6 +1406,9 @@ class class_ : public detail::generic_type {
);
}
v_h.value_ptr() = nullptr;
if (PyErr_Occurred()) {
PyErr_WriteUnraisable(v_h.type ? (PyObject*)v_h.type->type : nullptr);
Copy link
Contributor

@jbarlow83 jbarlow83 Aug 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reason for passing the type rather than the value here? According to the docs:

The function is called with a single argument obj that identifies the context in which the unraisable exception occurred. If possible, the repr of obj will be printed in the warning message.

As written it would print Exception ignored in: ValueError (since repr(v_h.type->type) is ValueError). It would be better if we could find a way to get better information to the user, e.g. the name of the destructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v_h.type->type is the class, not the exception.

I assumed that if I hit an error here, then the value no longer existed so a __repr__ wouldn't be sensible? I'm not sure if the name is still around, though I guess it must be?

Though, you're right, it would be good to do something more sensible. Python does this:

$ cat t.py
class X:
    def __del__(self):
        raise ValueError("something")
X()
$ python t.py
Exception ignored in: <function X.__del__ at 0x7fd070258f28>
Traceback (most recent call last):
  File "t.py", line 3, in __del__
    raise ValueError("something")
ValueError: something

As mentioned above, here's what I currently have:

Exception ignored in: <class 'pybind11_tests.class_.PyRaiseDestructor'>
Traceback (most recent call last):
  File "<snip>/pybind11/tests/test_class.py", line 389, in test_pyexception_destructor
    m.PyRaiseDestructor()
ValueError: unraisable error

I suppose we could add a name and a pointer value?

Copy link
Contributor

@jbarlow83 jbarlow83 Aug 3, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually it's probably good as is, since printing the name of the offending class gives a good idea as to where the occurred.

We definitely don't want to call any extra functions in this type of code, using what is available is best.

}
}

static detail::function_record *get_function_record(handle h) {
Expand Down
9 changes: 9 additions & 0 deletions tests/test_class.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,15 @@ TEST_SUBMODULE(class_, m) {
py::class_<PyPrintDestructor>(m, "PyPrintDestructor")
.def(py::init<>())
.def("throw_something", &PyPrintDestructor::throw_something);

struct PyRaiseDestructor {
PyRaiseDestructor() {}
~PyRaiseDestructor() {
PyErr_SetString(PyExc_ValueError, "unraisable error");
}
};
py::class_<PyRaiseDestructor>(m, "PyRaiseDestructor")
.def(py::init<>());
}

template <int N> class BreaksBase { public:
Expand Down
18 changes: 18 additions & 0 deletions tests/test_class.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -329,3 +329,21 @@ class PyNonFinalFinalChild(m.IsNonFinalFinal):
def test_exception_rvalue_abort():
with pytest.raises(RuntimeError):
m.PyPrintDestructor().throw_something()


def test_pyexception_destructor():
import sys

if hasattr(sys, "unraisablehook"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only Python 3.8+ have unraisablehook. I think for earlier versions we could call a subprocess and check its stderr.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's why I made it conditional. If the logic works on one version of python since this is so esoteric I didn't think it was worth testing on the older versions too.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pybind11 has some way of capturing the stderr. It would definitely be good to test on all Python versions, if possible (especially Python 2, which tends to act up/do things differently!).

old_hook = sys.unraisablehook
try:
ctxt = [None]

def new_hook(*args):
ctxt[0] = True

sys.unraisablehook = new_hook
m.PyRaiseDestructor()
assert ctxt[0] is not None
finally:
sys.unraisablehook = old_hook