-
Notifications
You must be signed in to change notification settings - Fork 67
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
feat(zend): add helper for try catch and bailout in PHP #275
Conversation
6d5434a
to
25cfc18
Compare
The main purpose of this try_catch is also to correctly drop value from Rust and avoid memory leak, as an example the following code will leak : pub fn my_php_method() {
let string = "foo".to_string();
something_that_may_bailout();
} When a bailout occurs it will jump to the last try catch and so the drop mechanism of rust will never be called As a best practice extension developer should now use the pub fn my_php_method() {
let string = "foo".to_string();
let catch = try_catch(|| {
something_that_may_bailout();
});
// bailout occurs if err
if catch.is_err() {
// dropping manually the value so it does not leak
std::mem::drop(string);
// then call bailout to go to the correct last try catch
bailout();
}
} However this would force all chain of execution to have this practice, there may be a better api to achieve that in a nicer and transparent way for extension developer |
I've tried to fix the problem in the past but I've never found a good solution. This also occurs in the other way from php with a rust extension loaded + the script crash with a memory limit error or timeout. My initial idea was to wrap the whole module init into a zend try catch. |
There may be a solution by using "extern unwind", see https://blog.rust-lang.org/inside-rust/2021/01/26/ffi-unwind-longjmp.html I'm trying to write test case to have a good reproducer and see if using this would solve it |
I added a test to expose this problem, at least we have a reproducible case |
The best solution for me would be to wrap function that may bailout in the try_catch mechanism and return the CatchErr Then on the the macro system if the function or method can return a CatchErr we bailout if this is the case So extension developer would be aware of the CatchErr and just need to pass down the chain until it reach the function. In this case it should correctly drop all values used by rust and bailout on the correct try catch |
That sounds promising! According to this PR, this looks like to be stabilized shortly |
Ok, does it mean that the extension developer should also implement the way of what to do when an unwind error comes from PHP? Could the |
Hum this could be the best solution, we could do the following :
|
Sounds good to me |
4a7f381
to
e28c57e
Compare
This should allow extension to wrap their code under the try catch mechanism of PHP