Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add catch_unwind! macro to prevent panics crossing ffi boundaries #797
Add catch_unwind! macro to prevent panics crossing ffi boundaries #797
Changes from all commits
9380bfd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really don't understand why we need this.
Why is it problematic to fetch
PanicException
?cc: @programmerjake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea is that unless caught a
panic!
should cause a stack unwind all the way until program exit (like it does without this PR). The changes in this PR are intended to make it so that we unwind safely through the FFI layer, because what we do at the moment is technically UB.This means we need two main things:
PanicException
.PanicException
is not caught, we should usestd::panic::resume_unwind()
when we get back into Rust code so that we continue the unwind.I need the original panic payload to call
resume_unwind()
, but at the moment it's difficult for me to store that onPanicException
. I expect that will create a lot of work which will extend this already large PR, so I opened #853 instead.Without the original payload, for now I just call
panic!
instead ofstd::panic::resume_unwind()
, which is close enough. It has the downside that the panic handler runs again, so at the moment thethread '<unnamed>' panicked at xyz
message will be printed multiple times as the program unwinds. (Once for the originalpanic!
and once each time we continue unwinding when we fetchPanicException
)Perhaps I should add a comment to the code documenting this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not true.
As I explained it can cause just a logic error.
So I'm not still convinced that PyO3 itself should cause panic for that.
Let's consider a scenario where multiple PyO3 crates, say, c1 and c2 work together.
c1 can fetch c2's PanicException. Do we really want to resume panic in such a situation?
How about adding an API (e.g.,
resume_panic
) to PanicException?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By this I mean the current pyo3 behaviour of not using
catch_unwind
is UB - Rust panic through the FFI stack frames isn't well defined. This is why we need to addcatch_unwind
.I agree with you that catching
PanicException
can cause a logic error. This could occur either when Python catches the exception, or if pyO3 does not continue to unwind after fetching aPanicException
.This is why I think the default behaviour should be to keep unwinding all the way to program exit - even if c1 fetches c2's
PanicException
.I'll put some diagrams together this evening to try to explain better.