Skip to content

Add error_scope to py::class_::dealloc() to protect destructor calls #2342

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

Merged
merged 1 commit into from
Aug 1, 2020
Merged

Add error_scope to py::class_::dealloc() to protect destructor calls #2342

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
7 changes: 7 additions & 0 deletions include/pybind11/pybind11.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1388,6 +1388,13 @@ class class_ : public detail::generic_type {

/// Deallocates an instance; via holder, if constructed; otherwise via operator delete.
static void dealloc(detail::value_and_holder &v_h) {
// We could be deallocating because we are cleaning up after a Python exception.
// If so, the Python error indicator will be set. We need to clear that before
// running the destructor, in case the destructor code calls more Python.
// If we don't, the Python API will exit with an exception, and pybind11 will
// throw error_already_set from the C++ destructor which is forbidden and triggers
// std::terminate().
error_scope scope;
if (v_h.holder_constructed()) {
v_h.holder<holder_type>().~holder_type();
v_h.set_holder_constructed(false);
Expand Down
11 changes: 11 additions & 0 deletions tests/test_class.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,17 @@ TEST_SUBMODULE(class_, m) {
// test_non_final_final
struct IsNonFinalFinal {};
py::class_<IsNonFinalFinal>(m, "IsNonFinalFinal", py::is_final());

struct PyPrintDestructor {
PyPrintDestructor() {}
~PyPrintDestructor() {
py::print("Print from destructor");
}
void throw_something() { throw std::runtime_error("error"); }
};
py::class_<PyPrintDestructor>(m, "PyPrintDestructor")
.def(py::init<>())
.def("throw_something", &PyPrintDestructor::throw_something);
}

template <int N> class BreaksBase { public:
Expand Down
6 changes: 6 additions & 0 deletions tests/test_class.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -323,3 +323,9 @@ def test_non_final_final():
class PyNonFinalFinalChild(m.IsNonFinalFinal):
pass
assert str(exc_info.value).endswith("is not an acceptable base type")


# https://github.com/pybind/pybind11/issues/1878
def test_exception_rvalue_abort():
with pytest.raises(RuntimeError):
m.PyPrintDestructor().throw_something()
Copy link
Collaborator

@YannickJadoul YannickJadoul Jul 30, 2020

Choose a reason for hiding this comment

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

Can we assert that the PyPrintDestructor() actually gets called before the error gets handled? We need to make sure we're testing the situation we think we're testing.

Copy link
Collaborator

@rwgk rwgk Jul 30, 2020

Choose a reason for hiding this comment

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

Sounds tricky, because you'd have to somehow know what is "before" and insert something there.
My opinion: if the test fails without the change to pybind11.h we're good.

Copy link
Collaborator

@bstaletic bstaletic Jul 31, 2020

Choose a reason for hiding this comment

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

Yes, the "before" does fail as described in #1878

Copy link
Collaborator

@YannickJadoul YannickJadoul Jul 31, 2020

Choose a reason for hiding this comment

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

OK, if it's fine with you two, why not :-)

Copy link
Collaborator

@YannickJadoul YannickJadoul Jul 31, 2020

Choose a reason for hiding this comment

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

Actually, it's probably OK here, but in an assert, pytest rewrites the AST (as we've seen before), so it is slightly tricky: https://github.com/pybind/pybind11/pull/2291/files#r453234693

Copy link
Contributor Author

@jbarlow83 jbarlow83 Jul 31, 2020

Choose a reason for hiding this comment

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

Since throw_something() is actually PyPrintDestructor.throw_something(self) under the hood, it's impossible for the PyPrintDestructor() to be not constructed before the exception is thrown. Python needs a reference to self before it can make the method call.

Given the subtlety of #1878 I think any additional assertions etc. may mean we create another reference to PyPrintDestructor which would change the timing of the destructor and not trigger the error.

Copy link
Collaborator

@rwgk rwgk Aug 1, 2020

Choose a reason for hiding this comment

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

I just spent about one hour to convince myself that this PR is safe to merge beyond a reasonable doubt. See approach below. (In the unlikely case that there are issues that escaped the automatic and my manual testing, it's a super easy rollback.)

I will merge this PR now. @jbarlow83 Thank you very much for the contribution!

Approach:

  • I checked out current pybind11 master.
  • I put the changes in this PR on top.
  • I built with Python 2.7.18rc1 and 3.8.4rc1, using clang++ with -std=c++11 and -std=c++2a (i.e. 4 builds).
  • All tests PASS in all 4 builds.
  • I then commented out the 1-line change in pybind11.h and rebuilt the 4 combinations.
  • Without the 1-line change, test_class.py FAILS in all 4 builds. The error messages differ between Python 2 and 3, but NOT between -std=c++11 and -std=c++2a. The two kinds of error messages are pasted below.

PYTHON 2 ERROR

===================================================================================== FAILURES ======================================================================================
____________________________________________________________________________ test_exception_rvalue_abort ____________________________________________________________________________

    def test_exception_rvalue_abort():
        with pytest.raises(RuntimeError):
>           m.PyPrintDestructor().throw_something()
E           SystemError: error return without exception set

test_class.py:331: SystemError
------------------------------------------------------------------------------- Captured stdout call --------------------------------------------------------------------------------
Print from destructor
============================================================================== short test summary info ==============================================================================

PYTHON3 ERROR

test_class.py ....................terminate called after throwing an instance of 'pybind11::error_already_set'
  what():  SystemError: <built-in method join of str object at 0x7f23d0a84cf0> returned a result with an error set
Fatal Python error: Aborted

Current thread 0x00007f23d0f9d740 (most recent call first):
  File "/usr/local/google/home/rwgk/clone/pybind11/tests/test_class.py", line 331 in test_exception_rvalue_abort
  File "/usr/local/lib/python3.8/dist-packages/_pytest/python.py", line 182 in pytest_pyfunc_call
<long traceback truncated>

For completeness:

$ clang++ --version
clang version 9.0.1-11 
Target: x86_64-pc-linux-gnu
Thread model: posix
InstalledDir: /usr/bin