Skip to content
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

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

joelwurtz
Copy link
Contributor

This should allow extension to wrap their code under the try catch mechanism of PHP

@joelwurtz
Copy link
Contributor Author

joelwurtz commented Oct 21, 2023

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 try_catch function to correctly support that by doing the following :

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

@ptondereau
Copy link
Collaborator

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.
Anyway thank you very much for the initial binding

@joelwurtz
Copy link
Contributor Author

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

@joelwurtz
Copy link
Contributor Author

I added a test to expose this problem, at least we have a reproducible case

@joelwurtz
Copy link
Contributor Author

joelwurtz commented Oct 21, 2023

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

@ptondereau
Copy link
Collaborator

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

That sounds promising! According to this PR, this looks like to be stabilized shortly

@ptondereau
Copy link
Collaborator

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

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 CatchErr just be a Rust's panic, just to release remaining allocated vars?

@joelwurtz
Copy link
Contributor Author

Could the CatchErr just be a Rust's panic, just to release remaining allocated vars?

Hum this could be the best solution, we could do the following :

  • Wrap functions that can bailout in a try catch and panic if there is one
  • Wrap php function / method exported to bailout if there is a panic (with a catch_unwind)

@ptondereau
Copy link
Collaborator

Could the CatchErr just be a Rust's panic, just to release remaining allocated vars?

Hum this could be the best solution, we could do the following :

* Wrap functions that can bailout in a try catch and panic if there is one

* Wrap php function / method exported to bailout if there is a panic (with a catch_unwind)

Sounds good to me

@ptondereau ptondereau merged commit 5fdd8fa into davidcole1340:master Oct 25, 2023
20 checks passed
@joelwurtz joelwurtz deleted the feat/try-catch branch October 26, 2023 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants