From 235e3703191fee2577ddcdcf31b4453e44e899ac Mon Sep 17 00:00:00 2001 From: Stephen Belanger Date: Tue, 27 May 2025 23:29:42 +0800 Subject: [PATCH] Revert "Fix SAPI shutdown (#19)" This reverts commit 734f1440d63127c74392c9854f2a481c2e48ed0b. --- README.md | 2 - __test__/handler.spec.mjs | 39 ++-------- crates/php/src/embed.rs | 37 +++------ crates/php/src/sapi.rs | 154 +++++++++++++++++--------------------- 4 files changed, 87 insertions(+), 145 deletions(-) diff --git a/README.md b/README.md index ae7773e7..8ba8a098 100644 --- a/README.md +++ b/README.md @@ -58,7 +58,6 @@ console.log(response.body.toString()) ### `new Php(config)` * `config` {Object} Configuration object - * `argv` {String[]} Process arguments. **Default:** [] * `docroot` {String} Document root for PHP. **Default:** process.cwd() * Returns: {Php} @@ -68,7 +67,6 @@ Construct a new PHP instance to which to dispatch requests. import { Php } from '@platformatic/php-node' const php = new Php({ - argv: process.argv, docroot: process.cwd() }) ```` diff --git a/__test__/handler.spec.mjs b/__test__/handler.spec.mjs index b5775115..60af4679 100644 --- a/__test__/handler.spec.mjs +++ b/__test__/handler.spec.mjs @@ -15,11 +15,12 @@ test('Support input/output streams', async (t) => { t.teardown(() => mockroot.clean()) const php = new Php({ + argv: process.argv, docroot: mockroot.path }) const req = new Request({ - method: 'POST', + method: 'GET', url: 'http://example.com/index.php', body: Buffer.from('Hello, from Node.js!') }) @@ -38,10 +39,12 @@ test('Capture logs', async (t) => { t.teardown(() => mockroot.clean()) const php = new Php({ + argv: process.argv, docroot: mockroot.path }) const req = new Request({ + method: 'GET', url: 'http://example.com/index.php' }) @@ -59,10 +62,12 @@ test('Capture exceptions', async (t) => { t.teardown(() => mockroot.clean()) const php = new Php({ + argv: process.argv, docroot: mockroot.path }) const req = new Request({ + method: 'GET', url: 'http://example.com/index.php' }) @@ -84,10 +89,12 @@ test('Support request and response headers', async (t) => { t.teardown(() => mockroot.clean()) const php = new Php({ + argv: process.argv, docroot: mockroot.path }) const req = new Request({ + method: 'GET', url: 'http://example.com/index.php', headers: { 'X-Test': ['Hello, from Node.js!'] @@ -99,33 +106,3 @@ test('Support request and response headers', async (t) => { t.is(res.body.toString(), 'Hello, from Node.js!') t.is(res.headers.get('X-Test'), 'Hello, from PHP!') }) - -test('Has expected args', async (t) => { - const mockroot = await MockRoot.from({ - 'index.php': `` - }) - t.teardown(() => mockroot.clean()) - - const php = new Php({ - argv: process.argv, - docroot: mockroot.path - }) - - const req = new Request({ - url: 'http://example.com/index.php' - }) - - const res = await php.handleRequest(req) - t.is(res.status, 200) - - t.is(res.body.toString('utf8'), JSON.stringify(process.argv)) -}) diff --git a/crates/php/src/embed.rs b/crates/php/src/embed.rs index b63a7f43..7f5d1367 100644 --- a/crates/php/src/embed.rs +++ b/crates/php/src/embed.rs @@ -1,8 +1,7 @@ use std::{ env::Args, - ffi::{c_char, CString}, + ffi::c_char, path::{Path, PathBuf}, - sync::Arc, }; use ext_php_rs::{ @@ -26,14 +25,6 @@ use crate::{ #[derive(Debug)] pub struct Embed { docroot: PathBuf, - - // TODO: Do something with this... - #[allow(dead_code)] - args: Vec, - - // NOTE: This needs to hold the SAPI to keep it alive - #[allow(dead_code)] - sapi: Arc, } // An embed instance may be constructed on the main thread and then shared @@ -99,16 +90,14 @@ impl Embed { C: AsRef, S: AsRef + std::fmt::Debug, { + ensure_sapi(argv)?; + let docroot = docroot .as_ref() .canonicalize() .map_err(|_| EmbedException::DocRootNotFound(docroot.as_ref().display().to_string()))?; - Ok(Embed { - docroot, - args: argv.iter().map(|v| v.as_ref().to_string()).collect(), - sapi: ensure_sapi()?, - }) + Ok(Embed { docroot }) } /// Get the docroot used for this Embed instance @@ -163,8 +152,12 @@ impl Handler for Embed { /// //assert_eq!(response.body(), "Hello, world!"); /// ``` fn handle(&self, request: Request) -> Result { + unsafe { + ext_php_rs::embed::ext_php_rs_sapi_per_thread_init(); + } + // Initialize the SAPI module - self.sapi.startup()?; + Sapi::startup()?; let url = request.url(); @@ -189,14 +182,6 @@ impl Handler for Embed { .unwrap_or(0); let cookie_data = nullable_cstr(headers.get("Cookie"))?; - let argc = self.args.len() as i32; - let mut argv_ptrs = vec![]; - for arg in self.args.iter() { - let string = CString::new(arg.as_bytes()) - .map_err(|_| EmbedException::CStringEncodeFailed(arg.to_owned()))?; - argv_ptrs.push(string.into_raw()); - } - // Prepare memory stream of the code let mut file_handle = unsafe { let mut file_handle = zend_file_handle { @@ -229,8 +214,8 @@ impl Handler for Embed { // Reset state globals.request_info.proto_num = 110; - globals.request_info.argc = argc; - globals.request_info.argv = argv_ptrs.as_mut_ptr(); + globals.request_info.argc = 0; + globals.request_info.argv = std::ptr::null_mut(); globals.request_info.headers_read = false; globals.sapi_headers.http_response_code = 200; diff --git a/crates/php/src/sapi.rs b/crates/php/src/sapi.rs index 764cdf0f..e64f9728 100644 --- a/crates/php/src/sapi.rs +++ b/crates/php/src/sapi.rs @@ -2,7 +2,7 @@ use std::{ collections::HashMap, env::current_exe, ffi::{c_char, c_int, c_void, CStr}, - sync::{Arc, RwLock, Weak}, + sync::RwLock, }; use bytes::Buf; @@ -12,9 +12,9 @@ use ext_php_rs::{ embed::SapiModule, exception::register_error_observer, ffi::{ - ext_php_rs_sapi_per_thread_init, ext_php_rs_sapi_shutdown, ext_php_rs_sapi_startup, - php_module_shutdown, php_module_startup, php_register_variable, sapi_send_headers, - sapi_shutdown, sapi_startup, ZEND_RESULT_CODE_SUCCESS, + ext_php_rs_sapi_shutdown, ext_php_rs_sapi_startup, php_module_shutdown, php_module_startup, + php_register_variable, sapi_send_headers, sapi_shutdown, sapi_startup, + ZEND_RESULT_CODE_SUCCESS, }, prelude::*, zend::{SapiGlobals, SapiHeader}, @@ -23,21 +23,30 @@ use ext_php_rs::{ use once_cell::sync::OnceCell; use crate::{ - strings::{cstr, drop_str, maybe_current_dir}, + strings::{cstr, maybe_current_dir}, EmbedException, RequestContext, }; use lang_handler::Header; // This is a helper to ensure that PHP is initialized and deinitialized at the // appropriate times. -#[derive(Debug)] -pub(crate) struct Sapi(RwLock>); +pub(crate) struct Sapi(Box); impl Sapi { - pub fn new() -> Result { + pub fn new(argv: Vec) -> Result + where + S: AsRef, + { + let argv: Vec<&str> = argv.iter().map(|s| s.as_ref()).collect(); + // let argc = argv.len() as i32; + // let mut argv_ptrs = argv + // .iter() + // .map(|v| v.as_ptr() as *mut c_char) + // .collect::>(); + let mut sapi = SapiBuilder::new("php_lang_handler", "PHP Lang Handler") .startup_function(sapi_module_startup) - .shutdown_function(sapi_module_shutdown) + // .shutdown_function(sapi_module_shutdown) // .activate_function(sapi_module_activate) .deactivate_function(sapi_module_deactivate) .ub_write_function(sapi_module_ub_write) @@ -57,20 +66,20 @@ impl Sapi { sapi.additional_functions = std::ptr::null(); // sapi.phpinfo_as_text = 1; - let exe_loc = current_exe() - .map(|p| p.display().to_string()) - .map_err(|_| EmbedException::FailedToFindExeLocation)?; + let exe_loc = argv + .first() + .map(|s| s.to_string()) + .or_else(|| current_exe().ok().map(|p| p.display().to_string())) + .ok_or(EmbedException::FailedToFindExeLocation)?; sapi.executable_location = cstr(exe_loc)?; let mut boxed = Box::new(sapi); unsafe { ext_php_rs_sapi_startup(); - sapi_startup(boxed.as_mut()); - if let Some(startup) = boxed.startup { - startup(boxed.as_mut()); - } + sapi_startup(boxed.as_mut()); + php_module_startup(boxed.as_mut(), get_module()); } // TODO: Should maybe capture this to store in EmbedException rather than @@ -85,61 +94,54 @@ impl Sapi { ctx.response_builder().exception(*msg); } }) - // TODO: Report this error somehow? .ok(); }); - Ok(Sapi(RwLock::new(boxed))) + Ok(Sapi(boxed)) } - pub fn startup<'a>(&'a self) -> Result<(), EmbedException> { - unsafe { - ext_php_rs_sapi_per_thread_init(); + fn do_startup(&mut self) -> Result<(), EmbedException> { + let sapi = self.0.as_mut(); + let startup = sapi + .startup + .ok_or(EmbedException::SapiMissingStartupFunction)?; + if unsafe { startup(sapi) } != ZEND_RESULT_CODE_SUCCESS { + return Err(EmbedException::SapiNotStarted); } + Ok(()) + } - let rwlock = &self.0; - let sapi = rwlock.read().map_err(|_| EmbedException::SapiLockFailed)?; - - if let Some(startup) = sapi.startup { - if unsafe { startup(sapi.into_raw()) } != ZEND_RESULT_CODE_SUCCESS { - return Err(EmbedException::SapiNotStarted); - } - } + pub fn startup() -> Result<(), EmbedException> { + SAPI_INIT + .get() + .ok_or(EmbedException::SapiNotInitialized) + .and_then(|rwlock| { + let mut sapi = rwlock.write().map_err(|_| EmbedException::SapiLockFailed)?; - Ok(()) + sapi.do_startup()?; + Ok(()) + }) } } impl Drop for Sapi { fn drop(&mut self) { unsafe { + php_module_shutdown(); sapi_shutdown(); + ext_php_rs_sapi_shutdown(); } } } -pub(crate) static SAPI_INIT: OnceCell>> = OnceCell::new(); - -pub fn ensure_sapi() -> Result, EmbedException> { - let weak_sapi = SAPI_INIT.get_or_try_init(|| Ok(RwLock::new(Weak::new())))?; +pub(crate) static SAPI_INIT: OnceCell> = OnceCell::new(); - if let Some(sapi) = weak_sapi - .read() - .map_err(|_| EmbedException::SapiLockFailed)? - .upgrade() - { - return Ok(sapi); - } - - let mut rwlock = weak_sapi - .write() - .map_err(|_| EmbedException::SapiLockFailed)?; - - let sapi = Sapi::new().map(Arc::new)?; - *rwlock = Arc::downgrade(&sapi); - - Ok(sapi) +pub fn ensure_sapi(argv: Vec) -> Result<&'static RwLock, EmbedException> +where + S: AsRef + std::fmt::Debug, +{ + SAPI_INIT.get_or_try_init(|| Sapi::new(argv).map(RwLock::new)) } // @@ -164,17 +166,15 @@ static HARDCODED_INI: &str = " "; #[no_mangle] -pub extern "C" fn sapi_cli_ini_defaults(ht: *mut ext_php_rs::types::ZendHashTable) { - let config = unsafe { &mut *ht }; +pub extern "C" fn sapi_cli_ini_defaults(configuration_hash: *mut ext_php_rs::types::ZendHashTable) { + let hash = unsafe { &mut *configuration_hash }; - let ini_lines = str::trim(HARDCODED_INI).lines().map(str::trim); + let config = str::trim(HARDCODED_INI).lines().map(str::trim); - for line in ini_lines { + for line in config { if let Some((key, value)) = line.split_once('=') { - use ext_php_rs::convert::IntoZval; - let value = value.into_zval(true).unwrap(); // TODO: Capture error somehow? - config.insert(key, value).ok(); + hash.insert(key, value).ok(); } } } @@ -186,45 +186,31 @@ pub extern "C" fn sapi_module_startup( unsafe { php_module_startup(sapi_module, get_module()) } } -#[no_mangle] -pub extern "C" fn sapi_module_shutdown( - _sapi_module: *mut SapiModule, -) -> ext_php_rs::ffi::zend_result { - unsafe { - php_module_shutdown(); - } - ZEND_RESULT_CODE_SUCCESS -} - #[no_mangle] pub extern "C" fn sapi_module_deactivate() -> c_int { { let mut globals = SapiGlobals::get_mut(); - for i in 0..globals.request_info.argc { - drop_str(unsafe { *globals.request_info.argv.offset(i as isize) }); - } - globals.server_context = std::ptr::null_mut(); globals.request_info.argc = 0; globals.request_info.argv = std::ptr::null_mut(); - drop_str(globals.request_info.request_method); - drop_str(globals.request_info.query_string); - drop_str(globals.request_info.request_uri); - drop_str(globals.request_info.path_translated); - drop_str(globals.request_info.content_type); - drop_str(globals.request_info.cookie_data); + // drop_str(globals.request_info.request_method); + // drop_str(globals.request_info.query_string); + // drop_str(globals.request_info.request_uri); + // drop_str(globals.request_info.path_translated); + // drop_str(globals.request_info.content_type); + // drop_str(globals.request_info.cookie_data); // drop_str(globals.request_info.php_self); - drop_str(globals.request_info.auth_user); - drop_str(globals.request_info.auth_password); - drop_str(globals.request_info.auth_digest); + // drop_str(globals.request_info.auth_user); + // drop_str(globals.request_info.auth_password); + // drop_str(globals.request_info.auth_digest); } // TODO: When _is_ it safe to reclaim the request context? - RequestContext::reclaim(); + // RequestContext::reclaim(); - ZEND_RESULT_CODE_SUCCESS + 0 } #[no_mangle] @@ -370,11 +356,7 @@ pub extern "C" fn sapi_module_register_server_variables(vars: *mut ext_php_rs::t sapi .read() .map_err(|_| EmbedException::SapiLockFailed)? - .upgrade() - .ok_or(EmbedException::SapiLockFailed)? .0 - .read() - .map_err(|_| EmbedException::SapiLockFailed)? .name, vars, );