-
Notifications
You must be signed in to change notification settings - Fork 782
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
Conversation
Awesome! 💯 |
@davidhewitt |
Thanks for all the reviews! I will answer and make a round of changes maybe tomorrow.
If you're ready for 0.9 to be released then please proceed without waiting for this. We can include this in 0.10. (There's technically breaking changes here to a couple of |
I haven't looked thoroughly at the implementation, I rather wanted to ask if we should forcefully exit (e.g. by abort) instead of raising an exception. A panic breaks all normal assumption about control flow in rust and might leave the rust part in a broken state, into which the python can than call later. See https://doc.rust-lang.org/nomicon/unwinding.html and https://doc.rust-lang.org/nomicon/poisoning.html for some related information. |
I can see why aborting is an easier choice. However, I don't think we should do this. I think Python users writing Rust for the first time are likely to write a lot of things which panic (e.g. From the edition guide, I understand that the use of |
I like the way normal rust binaries currently handle this: fn crashy() {
None.unwrap()
}
fn main() {
println!("Hello, world!");
crashy();
println!("Goodbye, world!");
} This prints
when running it with Long backtrace
pyo3 could have some message like "Your rust code panicked. pyo3 catched this for you. Do <...> to get a backtrace. Note: If you return a PyResult, it will be converted to a python error." |
Then let's include this in 0.9.
It seems too difficult for me. I opened a question on the user forum. |
Oh that's a great idea, thank you! Hopefully we'll get some clearer answers there |
Based on the discussion in the forum, now I think we should raise exceptions for panics. |
We could potentially add this as an optional feature |
Cool. I am not sure I see why panic with PyRefMut specifically is a problem. AFAIK drop code still runs during stack unwinding. We should definitely add a test for the PyRefMut case. |
I said Here's an example of such a logical invariant. The problem we have is we assume |
I'm not hugely keen on "implicit I have been thinking that the best solution is probably to "poison" the |
src/class/basic.rs
Outdated
} | ||
Err(e) => PyErr::from(e).restore_and_null(py), | ||
} | ||
let py = crate::Python::assume_gil_acquired(); |
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 might have missed this in the thread, but is assume_gil_acquired
sound without calling GILPool::new
beforehand? If so, could we document the reason somewhere?
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.
In all current code, GILPool::new
is called immediately after, like this:
let py = Python::assume_gil_acquired();
let pool = GILPool::new(py);
...
What I'm changing it to in this PR is now
let py = Python::assume_gil_acquired();
catch_unwind!(py, {
let pool = GILPool::new(py);
...
})
...
However you've rightly noticed I have accidentally lost the GILPool::new
here. I will fix this and also carefully check the rest of the changes I made.
I can't comment on whether this usage of assume_gil_acquired
is sound, but I have other soundness concerns with it. See #800
433de55
to
f1938d4
Compare
I'm not for that.
|
Ok. Fwiw I guess we can always change to have poisoning later if it turns out to be a problem. Sometime in the next few days I'll push some tests and documentation to this branch |
Personally, I only |
What if we made the exception type derive from https://docs.python.org/3/library/exceptions.html#SystemExit This would mean that it would be really rare for Python programs to catch this exception. To do this, I'd just like there to be some way to refer to this exception from Python code, so that in cases where users really do want to catch it, they can. I haven't thought of a good way to do this, so any ideas are welcome. |
Sounds like a good idea.
What about injecting a module containing the We could just publish a python package to PyPI to reserve the name with some simple code like: class PanicException(BaseException):
pass and it would be constructed by calling |
How about recommending users to register our custom exception module to their module?
Can it be confusing? |
Probably not from the perspective of PyO3 users, but it may be confusing for developers of PyO3 itself. The user would see The only edge cases are:
|
Additionally, when PyO3 is translating an exception to |
Couldn't we do this automatically? E.g. add a If that can work, I think I prefer it to a globally installed module. Though the globally installed module is a nice idea too; it's just more work for us :D
Yes I think this is a good idea. |
One other benefit of the global module is it allows multiple independent PyO3 libraries to use the exact same type. |
True, but it also complicates compatibility if we ever needed to change this module for any reason. I'm very torn on which is the best approach 😄 |
My 1st attempt is placed on #805
I don't know how we can do it. Maybe we need one more |
Yeah, I'm not sure what happens too 🙄. Please run the test code yourself. |
👍 Give me a few days to play with the printing also and I'll report back |
@davidhewitt |
First need to fix travis. I can do that this weekend. |
FYI haven't forgotten this, will do tomorrow. |
e4185be
to
2904fec
Compare
I've added a note to that doc. I've also improved the error message that occurs. Now, with this sample code:
This is the output:
|
src/err.rs
Outdated
pub fn fetch(_: Python) -> PyErr { | ||
/// | ||
/// If the current error is a `PanicException` (which would have originated in other pyo3 code) | ||
/// then this function will resume the uncaught panic. |
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.
What does uncaught
mean?
We catch the panic and resume it here, right?
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.
Ah yes, I meant the Python exception specifically was not caught in Python code. Perhaps this can be phrased better?
6f48b8d
to
f37f429
Compare
FYI while finishing this branch off I had an idea for a nice refactoring #901. But I wanted to open that as a separate PR, so please review that PR first and then I will rebase this one and finish up the documentation suggestion requested. |
a0eff02
to
85e043d
Compare
Rebased on #901 and updated the |
Thank you for lots of work! |
Fixes #492
This is a first attempt at updating all the wrapping code to use
catch_unwind
to prevent unsoundness of panics inside Rust callback functions.I implemented this using a
pyo3::catch_unwind
macro, which takes aPython
token and a body of code which should evaluate to aPyResult
.I ran into complications around lifetimes and the callback converters so I ended up simplifying a lot of the callback conversion code. I think a lot of those macros are now in slightly better shape but there's probably more refactoring that could be done.
Please give this a thorough review, I'm prepared to rework this as many times as needed to get it right.
TODO: