Use PyErr_WriteUnraisable when an error occurs in a destructor

[virtuald]

Minor improvement to #2342

With the default hook, you end up with an error message looking something like so:

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

[YannickJadoul]

I'm not sure about this. Calling PyErr_SetString without throwing an error_already_set exception is normally invalid in pybind11, no? If I understand correctly, this is why #2342 was needed; since pybind11 assumes the error indicator is not set without an exception being thrown and Python complains if the the error indicator is not set when a nullptr PyObject* is returned from a C function.
The other side of the coin is that any pybind11 binding or utility like py::print that sets the error indicator but doesn't throw py::error_already_set is incorrect (I think). So in these cases, the destructor will (by default, if it is noexcept) still crash.

I believe @jbarlow83 was still developing an extra utility that would catch error and call PyErr_WriteUnraisable to protect noexcept destructors. @jbarlow83, any thoughts/input?

In principle, a noexcept(false) destructor that throws an exception would still be a problem, though, yes. But then, it should throw a error_already_set exception, so I don't think this PR would help, then?

[virtuald]

The python docs say that PyErr_WriteUnraisable is exactly for this type of situation:

It is used, for example, when an exception occurs in an __del__() method.

I imagine if you needed to have a destructor that called python code, you would need to do something like this?

try {
  // py code
} catch (py::error_already_set &e) {
  e.restore();
}

[jbarlow83]

Yes, I've spent a while on this. It's all pretty complicated and nothing is obvious without some study.

What I do like about this PR is that @virtuald and and I have kind of independently converged on a similar solution for dealloc(). It makes sense for that function to call PyErr_WriteUnraisable if an error is set. In addition to that, it makes sense for the policy to be that we exit destructors by setting a Python error (because we can't throw anyway).

That said I do have a few concerns with this PR as written which I will tag.

One thing that is obvious - whatever we do - is we need to expand the documentation on destructors to discuss the issues. I've already ready drafted some material for that, because I think there should be a thorough explanation. (Quite busy with other work right now.)

Python complains if an error is not set when a nullptr PyObject* is returned from a C function.

Python complains if:

1. we return non null, with error set
2. we return null, with error not set
3. we call into Python C-API again while an error is still set - this is what Add error_scope to py::class_::dealloc() to protect destructor calls #2342 fixes

There could be some py:: functions that should be throwing error_already_set and don't. py::print, however, I think is correct, because object_api::operator() throws error_already_set in every relevant case.

One case that could be missing a throw error_already_set() here:

• class.h:127 PyObject_CallMethod

Anyway, the template function I was considering now looks like this:

// C++ runtime errors are the responsibility of the user, this is only for Python errors
template <typename Func>
inline void destructor_exception_trap(Func f) {
    try {
        f();
    } catch (error_already_set &e) { // trap any thrown py::error_already_set
        e.restore(); // sets error
    } catch (builtin_exception &e) { // trap py::value_error, etc.
        e.set_error(); // sets error
    }
}

// Usage
struct PythonRaiseInDestructor {
    PythonRaiseInDestructor(const py::str &s) : s(s) {}
    ~PythonRaiseInDestructor() {
        py::destructor_exception_trap([&]() {
            throw py::value_error(s);
        });
    }

    py::str s;
};

What I like about this is the necessary steps are sufficient complex that they ought to be encapsulated, and encapsulating them makes it easier to apply fixes universally or improve the solution. What I don't like is hiding a try-catch.

[jbarlow83]

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

[virtuald]

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.

[YannickJadoul]

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!).

[jbarlow83]

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.

[virtuald]

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?

[jbarlow83]

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.

[virtuald]

I was contemplating something similar to destructor_exception_trap... but I agree it's a bit ugly that it hides a try-catch. Better than a macro though.

It makes sense for that function to call PyErr_WriteUnraisable if an error is set. In addition to that, it makes sense for the policy to be that we exit destructors by setting a Python error (because we can't throw anyway).

I agree. I think if we update the documentation with a version of what's in these comments, then that would be sufficient.

One case that I was a bit worried about was if I had an object that had a py::object attribute that was destructed as my object was being destructed... but I think if we encapsulate it using the rules here then that will never be an issue because the inner error will also go to sys.unraisablehook

[YannickJadoul]

The python docs say that PyErr_WriteUnraisable is exactly for this type of situation:

It is used, for example, when an exception occurs in an __del__() method.

I'm not disputing this is the correct functionality to call. I'm saying that PyErr_Occurred() should never return true in pybind11, because a py::error_already_set should be thrown.

I imagine if you needed to have a destructor that called python code, you would need to do something like this?

try {
  // py code
} catch (py::error_already_set &e) {
  e.restore();
}

Exactly. Your PR won't fix this, because the thrown exception will abort your noexcept destructor before even reaching if (PyErr_Occurred()).

[YannickJadoul]

It makes sense for that function to call PyErr_WriteUnraisable if an error is set.

Sure, yes.

In addition to that, it makes sense for the policy to be that we exit destructors by setting a Python error (because we can't throw anyway).

I'm not sure about this, though. Again, we never have pybind11 set any error without py::error_already_set being thrown. So we still have to add an extra construct. I think I preferred the solution @jbarlow83 suggested in Gitter, then, rather than breaking the pybind11 contract on setting the error indicator in Python.

Python complains if:

1. we return `non null, with error set`

2. we return `null, with error not set`

Exactly. But these can only occur by accident. I think these concerns are orthogonal to this current problem/PR?

There could be some py:: functions that should be throwing error_already_set and don't. ``

That's a bug. If you find those, we should fix them (rather than implementing a workaround here)! :-)

[jbarlow83]

Ahh, now I see. The idea in the PR amounts to saying that in the case of a destructor, pybind11 would possibly exit with the destructor with an error, and the value_holder thing would tidy up. But I tend to think it is not worth making an exception to the contract for this case. (Also, the the exit-with-error-set approach may fail in the case of subclassed destructors, as we would enter the superclass destructor in an invalid state.) Scratch that.

The increasing complexity of handling this justifies adding a helper function to support it.

// C++ runtime errors are the responsibility of the user, this is only for Python errors
template <typename Func>
inline void destructor_exception_trap(const char *caller, Func f) {
    bool unraisable = false;
    try {
        f();
    } catch (error_already_set &e) { // trap any thrown py::error_already_set
        e.restore();
        unraisable = true;
    } catch (builtin_exception &e) { // trap py::value_error, etc.
        e.set_error(); // sets error
        unraisable = true;
    }
    if (unraisable) {
        PyErr_WriteUnraisable(py::str(caller).ptr());
    }          
}

// Usage
destroyme::~destroyme { destructor_exception_trap(__func__, []() { ... } }

I'm not sure if there's a reasonable way to obtain the associated PyTypeObject* from inside that template function (if one exists). Automating the passing of __func__ is a dead end except in C++20.

[YannickJadoul]

Right, using the Python C API error indicator (basically a global variable) to bypass a noexcept seems wrong to me.

But I tend to think it is not worth making an exception to the contract for this case.

I'm not sure it's an explicit "contract", but that's what I got from working with pybind11 anyway. Feel free to correct me if necessary :-)

(Also, the the exit-with-error-set approach may fail in the case of subclassed destructors, as we would enter the superclass destructor in an invalid state.)

Good point! Hadn't even thought of that.

I have a suggestion, to resolve this:

• Managing the noexcept-compliance of a destructor is the user's job (and there's not a lot we can do about it anyway, since it's user's code; we'd need something like reflection to maybe be able to solve something). Best we can do here is provide a warning in the docs that calling Python functions out of a default noexcept destructor is dangerous.
• I'm up for providing the utility to the user, to make it easier for the user. Whether that's the current solution, or even something with macros, I don't really care. But this solution seems OK, and lambda's make it really straightforward to use, so why not.
• Would it make sense to add a method discard_as_unraisable to py::error_already_set/py::builtin_exception? It somehow feels like it could be part of these classes.
• We can (should?) wrap the destructor in dealloc with a try-catch if an error is set ánd the destructor is not noexcept, btw, to follow Python more closely.

[jbarlow83]

This thing is a quagmire. I realized, for consistency, we ought to also run any exception translators that the user registered, i.e. running the equivalent of the catch(...) block in cpp_function::dispatcher. This works in a quick test, but it feels like a bad idea to be running exception translators in a loop when we're already unwinding the stack because of some exception that very code produced with different state. There are surely infinite exception throwing loops here. Trying to get all this right seems just too complicated and probably unnecessary.

As such I am proposing that we add error_already_set::discard_as_unraisable() (done on my end), and document its use. The only really surprising case is that an innocent py::whatever call in a destructor could throw and terminate, because there's nothing about it that telegraphs the possibility of throwing. An explicit throw py::builtin_exception() in a destructor is less likely to happen and easier to spot. This is simple enough to justify not having a template lambda.

A few days ago I tried a noexcept(false) destructor and it seemed to crash anyway, so I don't know if there's anything we can do in dealloc.

[YannickJadoul]

@virtuald This issue has been addressed by #2372. Is there something in this PR you want to keep or rework, or can this be closed?