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.

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

Exception handling gets confused when Python code is called from destructor #1877

Open
odahoda opened this Issue Sep 21, 2017 · 3 comments

Comments

Projects
None yet
2 participants
@odahoda

odahoda commented Sep 21, 2017

Given this .pyx code:

cdef void cb(int a) with gil:
   printf("C++ prints: %d\n", a);
   print("Cython prints: %d" % a)

def main(crash):
    cdef unique_ptr[Foo] ptr
    ptr.reset(new Foo(cb))
    raise RuntimeError("Kaboom")

The Foo class calls the cb callback from its destructor, i.e. when the unique_ptr goes out of scope.
Cython seems to think that the exception is caused by the print call in cb() (or any "potentially exception raising" code) and prints an "Exception ignored" message.
The exception is then cleared, so the calling code complains that main "returned NULL without setting an error".

Complete example in the attached file cython-bug.zip. Execute with python setup.py build_ext --inplace && python pymain.py.

The output that I get from running the example is:

[...snip build messages...]
--------------------------------------------------------------------------------
Foo::Foo()
C++ prints: 1
Cython prints: 1
Foo::~Foo()
C++ prints: 2
Cython prints: 2
--------------------------------------------------------------------------------
Foo::Foo()
C++ prints: 1
Cython prints: 1
Foo::~Foo()
C++ prints: 2
RuntimeError: Kaboom

During handling of the above exception, another exception occurred:

SystemError: <built-in method write of _io.TextIOWrapper object at 0x7f2726712630> returned a result with an error set
Exception ignored in: 'pyxstuff.cb'
SystemError: <built-in method write of _io.TextIOWrapper object at 0x7f2726712630> returned a result with an error set
Cython prints: 2Got exception: <built-in function main> returned NULL without setting an error

This is using Cython version 0.26.1 on Linux.

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Sep 21, 2017

Contributor

Thanks for the report. My guess is that Cython does not wrap the C++ cleanup in a Python "finally" construct, i.e. it keeps the live exception instead of storing it away.

Contributor

scoder commented Sep 21, 2017

Thanks for the report. My guess is that Cython does not wrap the C++ cleanup in a Python "finally" construct, i.e. it keeps the live exception instead of storing it away.

@odahoda

This comment has been minimized.

Show comment
Hide comment
@odahoda

odahoda Sep 24, 2017

Yeah, it looks like I can work around the issue by wrapping the body of the callback function in PyErr_Fetch/PyErr_Restore.

odahoda commented Sep 24, 2017

Yeah, it looks like I can work around the issue by wrapping the body of the callback function in PyErr_Fetch/PyErr_Restore.

odahoda added a commit to odahoda/noisicaa that referenced this issue Sep 24, 2017

@scoder

This comment has been minimized.

Show comment
Hide comment
@scoder

scoder Sep 26, 2017

Contributor

Honestly, this seems like the best fix already. Most C++ cleanup code does not do any Python interaction (in fact, most C++ cleanup code is probably trivial), so always storing away and (conditionally) restoring the current exception at the end of a function with stack allocated C++ objects would be a bit wasteful.

I'll keep this ticket open, because I agree that the current state is unsatisfactory, but it's not clear to me if we should do something about it.

Contributor

scoder commented Sep 26, 2017

Honestly, this seems like the best fix already. Most C++ cleanup code does not do any Python interaction (in fact, most C++ cleanup code is probably trivial), so always storing away and (conditionally) restoring the current exception at the end of a function with stack allocated C++ objects would be a bit wasteful.

I'll keep this ticket open, because I agree that the current state is unsatisfactory, but it's not clear to me if we should do something about it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment