davidcole1340 / ext-php-rs

Bindings for the Zend API to build PHP extensions natively in Rust.
Apache License 2.0
598 stars 64 forks source link

feat(zend): add helper for try catch and bailout in PHP #275

Closed joelwurtz closed 1 year ago

joelwurtz commented 1 year ago

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

joelwurtz commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

joelwurtz commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 :

ptondereau commented 1 year ago

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