Add error_scope to py::class_::dealloc() to protect destructor calls #2342
Conversation
|
I'll do a separate PR in a few days with improvements to documentation when using destructors. |
There was a problem hiding this comment.
Besides the extra small test, looks good to me!
Thanks for taking this up, @jbarlow83! :-)
| # https://github.com/pybind/pybind11/issues/1878 | ||
| def test_exception_rvalue_abort(): | ||
| with pytest.raises(RuntimeError): | ||
| m.PyPrintDestructor().throw_something() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Yes, the "before" does fail as described in #1878
There was a problem hiding this comment.
OK, if it's fine with you two, why not :-)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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++11and-std=c++2a(i.e. 4 builds). - All tests PASS in all 4 builds.
- I then commented out the 1-line change in
pybind11.hand rebuilt the 4 combinations. - Without the 1-line change,
test_class.pyFAILS in all 4 builds. The error messages differ between Python 2 and 3, but NOT between-std=c++11and-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
| # https://github.com/pybind/pybind11/issues/1878 | ||
| def test_exception_rvalue_abort(): | ||
| with pytest.raises(RuntimeError): | ||
| m.PyPrintDestructor().throw_something() |
There was a problem hiding this comment.
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.
| # https://github.com/pybind/pybind11/issues/1878 | ||
| def test_exception_rvalue_abort(): | ||
| with pytest.raises(RuntimeError): | ||
| m.PyPrintDestructor().throw_something() |
There was a problem hiding this comment.
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++11and-std=c++2a(i.e. 4 builds). - All tests PASS in all 4 builds.
- I then commented out the 1-line change in
pybind11.hand rebuilt the 4 combinations. - Without the 1-line change,
test_class.pyFAILS in all 4 builds. The error messages differ between Python 2 and 3, but NOT between-std=c++11and-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
|
Thanks, @jbarlow83! |
Fixes issue #1878
@YannickJadoul as discussed in gitter