From 6e45c5fb3a345e800bd970426579bb97008724d8 Mon Sep 17 00:00:00 2001 From: Federico Martinez Date: Wed, 8 Jul 2026 16:37:27 -0300 Subject: [PATCH] fix: worker-safe index lifecycle; drop FFI backend, keep ext-php-rs only MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two production blockers from the code review, plus removal of the FFI backend. Fix #1 — handle leak: nothing tied a registry entry's lifetime to the owning PHP object, so every open() without an explicit close() leaked an IndexState (mmap reader + segment FDs, and up to the writer heap) for the life of a PHP-FPM worker. Add `impl Drop for Index` (RAII release on GC) and an ExtClient::__destruct() as a deterministic, idempotent release point. Fix #2 — mutex poisoning: a panic inside a tantivy op under the global table lock poisoned the Mutex, and every later `.lock().unwrap()` then panicked, bricking all indexes in the worker. Route all lock sites through a lock_table() helper that recovers via PoisonError::into_inner() (the guarded HashMap can't be left half-updated by a panic in the user closure, so recovery is safe). Both fixes covered by new regression tests (registry poison recovery; ext Drop releases the handle), written test-first. Drop the FFI backend entirely (tantivy-ffi crate, C header, FfiClient) so only the ext-php-rs extension remains. The Client facade now delegates straight to ExtClient and errors with an actionable message when the extension isn't loaded (this also closes the FFI-only packaging finding). Updated CI, docs, composer. Also require IndexBusyException in the PHPUnit bootstrap (latent fatal). Verified: cargo build --release, cargo test (11 passed), clippy -D warnings, and the PHP smoke test against the built extension all green. --- .github/workflows/build.yml | 44 +++---- Cargo.lock | 8 -- Cargo.toml | 8 +- README.md | 46 ++------ crates/tantivy-core/src/registry.rs | 41 ++++++- crates/tantivy-core/src/writer.rs | 2 +- crates/tantivy-ext/src/lib.rs | 51 +++++++- crates/tantivy-ffi/Cargo.toml | 12 -- crates/tantivy-ffi/src/error.rs | 59 --------- crates/tantivy-ffi/src/ffi.rs | 138 ---------------------- crates/tantivy-ffi/src/lib.rs | 9 -- crates/tantivy-ffi/tests/ffi_roundtrip.rs | 41 ------- docs/BUILD.md | 44 +++---- include/tantivyphp.h | 18 --- php/composer.json | 5 +- php/src/Client.php | 26 ++-- php/src/ExtClient.php | 20 +++- php/src/FfiClient.php | 137 --------------------- php/tests/bootstrap.php | 2 +- php/tests/smoke.php | 10 +- 20 files changed, 175 insertions(+), 546 deletions(-) delete mode 100644 crates/tantivy-ffi/Cargo.toml delete mode 100644 crates/tantivy-ffi/src/error.rs delete mode 100644 crates/tantivy-ffi/src/ffi.rs delete mode 100644 crates/tantivy-ffi/src/lib.rs delete mode 100644 crates/tantivy-ffi/tests/ffi_roundtrip.rs delete mode 100644 include/tantivyphp.h delete mode 100644 php/src/FfiClient.php diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 96a9a27..e9dcf44 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -1,8 +1,8 @@ name: build -# Builds and tests both PHP bindings (FFI cdylib + ext-php-rs extension) for the tantivy-php -# workspace, per spec §6 (docs/superpowers/specs/2026-07-08-tantivy-php-ext-php-rs-binding-design.md) -# and plan Task 9 (docs/superpowers/plans/2026-07-08-tantivy-php-ext-php-rs-binding.md). +# Builds and tests the ext-php-rs PHP extension for the tantivy-php workspace, per spec §6 +# (docs/superpowers/specs/2026-07-08-tantivy-php-ext-php-rs-binding-design.md) and plan Task 9 +# (docs/superpowers/plans/2026-07-08-tantivy-php-ext-php-rs-binding.md). # # Scope is pinned to PHP 8.4 NTS, x86_64, per the project's stated constraint (see spec §10 # no-objetivos: no ZTS, no other PHP versions, no macOS). @@ -36,10 +36,9 @@ jobs: # against whichever ABI php-config reports, so we assert NTS explicitly below rather # than trust the default silently. tools: none - # ffi must be loaded for the FFI-backend smoke; ffi.enable=1 alone does nothing if the - # extension isn't present. The ext-backend smoke needs no PHP extension of its own. - extensions: ffi - ini-values: ffi.enable=1 + # The ext backend IS the extension under test (loaded via -d extension= in the smoke + # step), so no PHP extension needs to be preinstalled here. + extensions: none - name: Assert PHP build is NTS (project is pinned to 8.4 NTS only) run: php -v | grep -q 'NTS' || { echo "::error::expected an NTS PHP build, got:"; php -v; exit 1; } @@ -58,33 +57,27 @@ jobs: php -v php-config --version - # Builds crates/tantivy-ffi (libtantivyphp.so) and crates/tantivy-ext (libtantivyphp_ext.so) - # from the workspace root in one pass. On this Linux setup (stable Rust + clang present), - # no special env vars (LIBCLANG_PATH / BINDGEN_EXTRA_CLANG_ARGS) are needed — see progress - # notes in docs/superpowers/sdd/progress.md for the historical libclang-only fallback. - - name: cargo build --release (builds both cdylibs) + # Builds crates/tantivy-ext (libtantivyphp_ext.so) from the workspace root. On this Linux + # setup (stable Rust + clang present), no special env vars (LIBCLANG_PATH / + # BINDGEN_EXTRA_CLANG_ARGS) are needed — see progress notes in + # docs/superpowers/sdd/progress.md for the historical libclang-only fallback. + - name: cargo build --release run: cargo build --release - - name: cargo test (tantivy-core + tantivy-ffi roundtrip) + - name: cargo test (tantivy-core + tantivy-ext) run: cargo test - name: cargo clippy -D warnings run: cargo clippy --release --all-targets -- -D warnings - - name: Smoke test — FFI backend (ffi.enable, TANTIVYPHP_LIB) - env: - TANTIVYPHP_LIB: ${{ github.workspace }}/target/release/libtantivyphp.so - run: php -d ffi.enable=1 php/tests/smoke.php - - name: Smoke test — ext backend (extension=, no ffi.enable needed) run: php -d extension="${{ github.workspace }}/target/release/libtantivyphp_ext.so" php/tests/smoke.php - # Package the Linux half of the 4-artifact matrix (spec §6): FFI cdylib as-is, ext cdylib - # renamed to tantivyphp.so so it drops straight into extension_dir per the install guide. + # Package the Linux ext artifact: the ext cdylib renamed to tantivyphp.so so it drops + # straight into extension_dir per the install guide. - name: Stage Linux artifacts run: | mkdir -p dist/linux - cp target/release/libtantivyphp.so dist/linux/libtantivyphp.so cp target/release/libtantivyphp_ext.so dist/linux/tantivyphp.so - name: Upload Linux artifacts @@ -99,8 +92,7 @@ jobs: runs-on: windows-latest # Recipe mirrors ext-php-rs's own upstream Windows CI (setup-php + nightly, no dev-pack step, # default MSVC linker). Kept `continue-on-error: true` only until this job has gone green here - # at least once — a Windows-ext failure then never blocks the pipeline, and the FFI cdylib - # already covers Windows customers who can enable ffi.enable=1. Flip to false once it's green. + # at least once — a Windows-ext failure then never blocks the pipeline. Flip to false once green. continue-on-error: true steps: - uses: actions/checkout@v4 @@ -153,11 +145,7 @@ jobs: php -d extension="${{ github.workspace }}\target\release\tantivyphp_ext.dll" -m php -d extension="${{ github.workspace }}\target\release\tantivyphp_ext.dll" -r "var_dump(class_exists('Tantivy\Native\Index'));" - # Windows half of the 4-artifact matrix (spec §6). NOTE: this only stages the ext .dll. - # The FFI Windows artifact (tantivyphp.dll, "existing path" per spec §6) is not built by - # this workflow yet — it was previously flagged (docs/superpowers/sdd/progress.md, "Carried - # items") as not verified in any environment and is tracked separately, not part of this - # ext-focused spike/CI task. + # Stages the Windows ext .dll artifact. - name: Stage Windows ext artifact shell: pwsh run: | diff --git a/Cargo.lock b/Cargo.lock index 605b25d..c454c5b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1620,14 +1620,6 @@ dependencies = [ "tantivy-core", ] -[[package]] -name = "tantivy-ffi" -version = "0.1.0" -dependencies = [ - "serde_json", - "tantivy-core", -] - [[package]] name = "tantivy-fst" version = "0.5.0" diff --git a/Cargo.toml b/Cargo.toml index 81f8183..622fc41 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -1,18 +1,18 @@ [workspace] resolver = "2" -members = ["crates/tantivy-core", "crates/tantivy-ffi", "crates/tantivy-ext"] +members = ["crates/tantivy-core", "crates/tantivy-ext"] [workspace.package] version = "0.1.0" edition = "2021" -description = "Embed the tantivy search engine in PHP, via a Rust cdylib (FFI) or a native ext-php-rs extension." +description = "Embed the tantivy search engine in PHP, via a native ext-php-rs extension." repository = "https://github.com/InvGate/php-tantivy" license = "MIT OR Apache-2.0" [profile.release] # Strip the local symbol table from distributed artifacts (no DWARF is emitted anyway). -# Exported ABI symbols live in .dynsym and are preserved, so FFI::cdef and the PHP extension -# entry point still resolve. +# Exported ABI symbols live in .dynsym and are preserved, so the PHP extension entry point +# still resolves. strip = true [workspace.dependencies] diff --git a/README.md b/README.md index d2f61c9..ebd3428 100644 --- a/README.md +++ b/README.md @@ -2,52 +2,33 @@ Embed the [tantivy](https://github.com/quickwit-oss/tantivy) search engine (Rust) in PHP. -The Rust engine is exposed to PHP through **two interchangeable backends** that share the -same core, so you can pick whichever your environment allows: - -| Backend | Loaded via | Needs `ffi.enable`? | Artifact | -|---|---|---|---| -| **FFI** | `FFI::cdef` | yes | `libtantivyphp.so` / `tantivyphp.dll` (plain cdylib, PHP-version-independent) | -| **Native extension** | `extension=` in `php.ini` | **no** | `libtantivyphp_ext.so` / `tantivyphp_ext.dll` (built with [ext-php-rs](https://github.com/davidcole1340/ext-php-rs)) | - -Use the FFI backend when FFI is available; use the native extension when a hardened -environment forbids `ffi.enable`. Both are behaviorally identical — same engine, same -JSON boundary, same results. +The Rust engine is exposed to PHP as a **native PHP extension** built with +[ext-php-rs](https://github.com/davidcole1340/ext-php-rs) and loaded via `extension=` in +`php.ini` — no `ffi.enable` required. Artifact: `libtantivyphp_ext.so` / `tantivyphp_ext.dll`. ## Requirements - PHP **8.4**, NTS, x86_64 (Linux or Windows) -- For the FFI backend: `ffi.enable=1` and the cdylib on disk -- For the native extension: the extension loaded via `php.ini` (no FFI) +- The extension loaded via `php.ini` ## Install / build See [docs/BUILD.md](docs/BUILD.md). In short: ```bash -cargo build --release # builds both libtantivyphp.so and libtantivyphp_ext.so -``` - -Then either: - -```ini -; FFI backend -ffi.enable=1 -; and point TANTIVYPHP_LIB / TANTIVYPHP_HEADER at the cdylib + include/tantivyphp.h +cargo build --release # builds target/release/libtantivyphp_ext.so ``` -or: +Then load it: ```ini -; native extension backend extension=tantivyphp_ext.so ``` ## Usage -The PHP-facing API is a single facade, `Tantivy\Client`, which auto-selects the backend -(native extension if loaded, else FFI). Consumers depend only on `Tantivy\Client` and -`Tantivy\ClientInterface`. +The PHP-facing API is a single facade, `Tantivy\Client`, backed by the native extension. +Consumers depend only on `Tantivy\Client` and `Tantivy\ClientInterface`. ```php use Tantivy\Client; @@ -85,18 +66,15 @@ from refresh; batch your writes and commit periodically for throughput. ``` crates/tantivy-core Rust engine (schema, index registry, writer, query) — binding-agnostic -crates/tantivy-ffi C-ABI cdylib (the FFI backend) -crates/tantivy-ext ext-php-rs native extension (the extension backend) -include/tantivyphp.h C header for the FFI backend -php/src PHP client: Client (facade), ClientInterface, FfiClient, ExtClient -php/tests/smoke.php backend-agnostic smoke test +crates/tantivy-ext ext-php-rs native extension +php/src PHP client: Client (facade), ClientInterface, ExtClient +php/tests/smoke.php smoke test (requires the extension loaded) ``` ## Testing ```bash -cargo test # Rust core + FFI roundtrip -TANTIVYPHP_LIB=target/release/libtantivyphp.so php -d ffi.enable=1 php/tests/smoke.php +cargo test # Rust core + ext php -d extension=target/release/libtantivyphp_ext.so php/tests/smoke.php ``` diff --git a/crates/tantivy-core/src/registry.rs b/crates/tantivy-core/src/registry.rs index 355aa15..d8c6d97 100644 --- a/crates/tantivy-core/src/registry.rs +++ b/crates/tantivy-core/src/registry.rs @@ -29,6 +29,14 @@ fn table() -> &'static Mutex> { TABLE.get_or_init(|| Mutex::new(HashMap::new())) } +/// Toma el lock de la tabla recuperándose de un envenenamiento. Si un panic desenrolló mientras +/// otro llamador tenía el lock, `lock()` devuelve `PoisonError`; hacer `.unwrap()` propagaría ese +/// panic y dejaría la tabla inutilizable para TODO el proceso (crítico en un worker PHP-FPM de larga +/// vida). Un panic aislado no corrompe el HashMap en sí, así que recuperamos el guard y seguimos. +fn lock_table() -> std::sync::MutexGuard<'static, HashMap> { + table().lock().unwrap_or_else(|poisoned| poisoned.into_inner()) +} + /// Abre un índice existente o lo crea en `cfg.path`. Devuelve un handle opaco. pub fn open_or_create(cfg: IndexConfig) -> Result { std::fs::create_dir_all(&cfg.path).map_err(|e| format!("no se pudo crear el dir: {e}"))?; @@ -78,7 +86,7 @@ fn register_state(index: Index, id_field: &str, writer_heap_bytes: usize) -> Res }; let handle = NEXT_HANDLE.fetch_add(1, Ordering::SeqCst); - table().lock().unwrap().insert(handle, state); + lock_table().insert(handle, state); Ok(handle) } @@ -87,7 +95,7 @@ pub fn with_state( handle: u64, f: impl FnOnce(&mut IndexState) -> Result, ) -> Result { - let mut guard = table().lock().unwrap(); + let mut guard = lock_table(); let state = guard .get_mut(&handle) .ok_or_else(|| format!("handle inválido: {handle}"))?; @@ -96,7 +104,7 @@ pub fn with_state( /// Cierra y descarta el índice. Devuelve true si existía. pub fn close(handle: u64) -> bool { - table().lock().unwrap().remove(&handle).is_some() + lock_table().remove(&handle).is_some() } #[cfg(test)] @@ -170,6 +178,33 @@ mod tests { let _ = std::fs::remove_dir_all(&dir); } + #[test] + fn recovers_after_a_poisoned_lock() { + let dir = std::env::temp_dir().join(format!("tv_test_{}_poison", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + let h = open_or_create(cfg(dir.to_str().unwrap())).unwrap(); + + // Envenenamos el mutex global: forzamos un panic mientras with_state tiene el lock tomado. + // (silenciamos el hook de panic para no ensuciar la salida del test con el backtrace esperado.) + let prev_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(|_| {})); + let panicked = std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| { + let _ = with_state(h, |_s| -> Result<(), String> { + panic!("boom con el lock tomado") + }); + })); + std::panic::set_hook(prev_hook); + assert!(panicked.is_err(), "la clausura debía paniquear"); + + // Con .lock().unwrap() esto panicaría (mutex envenenado) y dejaría la tabla inutilizable + // para todo el proceso. Al recuperar el lock envenenado, el registro sigue operativo. + let n = with_state(h, |s| s.doc_count()).unwrap(); + assert_eq!(n, 0); + assert!(close(h)); + + let _ = std::fs::remove_dir_all(&dir); + } + #[test] fn open_read_only_fails_on_missing_index_and_opens_existing() { let dir = std::env::temp_dir().join(format!("tv_test_{}_readonly", std::process::id())); diff --git a/crates/tantivy-core/src/writer.rs b/crates/tantivy-core/src/writer.rs index 54d6767..a4dae01 100644 --- a/crates/tantivy-core/src/writer.rs +++ b/crates/tantivy-core/src/writer.rs @@ -5,7 +5,7 @@ use crate::registry::IndexState; /// Prefijo estable y neutro (no depende del idioma del mensaje) que marca el caso "el writer lock /// exclusivo del índice está tomado por otro proceso" (p. ej. un rebuild en curso). Los bindings PHP /// lo mapean a `Tantivy\IndexBusyException` para que los consumidores chequeen el TIPO, no el texto. -/// Es un contrato: si se cambia acá, actualizar los clientes (FfiClient/ExtClient). +/// Es un contrato: si se cambia acá, actualizar el cliente (ExtClient / TantivyException::forOperation). pub const WRITER_LOCKED_PREFIX: &str = "index_locked:"; /// Obtiene (o crea) el IndexWriter cacheado del estado. tantivy permite un solo writer por diff --git a/crates/tantivy-ext/src/lib.rs b/crates/tantivy-ext/src/lib.rs index 775ee4e..e01b3f5 100644 --- a/crates/tantivy-ext/src/lib.rs +++ b/crates/tantivy-ext/src/lib.rs @@ -27,6 +27,17 @@ pub struct Index { handle: u64, } +/// RAII: cuando PHP libera el objeto `Tantivy\Native\Index` (refcount a 0 / GC), ext-php-rs dropea +/// este struct y liberamos el estado del registro. Sin esto, cada open sin un close() explícito fuga +/// un IndexState (reader mmap + FDs de segmentos, y hasta el heap del writer) por toda la vida del +/// proceso — crítico en un worker PHP-FPM de larga vida que abre un índice por request. close() es +/// idempotente, así que un close() manual previo más este Drop no se pisan. +impl Drop for Index { + fn drop(&mut self) { + registry::close(self.handle); + } +} + #[php_impl] impl Index { #[php(name = "openOrCreate")] @@ -69,7 +80,7 @@ impl Index { #[php(name = "optimize")] pub fn optimize(&self) -> PhpResult<()> { - // v1: no-op, igual que el path FFI (tv_optimize). + // v1: no-op (el merge se agenda en el plan de rebuild). Nunca falla. Ok(()) } @@ -95,3 +106,41 @@ impl Index { pub fn get_module(module: ModuleBuilder) -> ModuleBuilder { module.class::() } + +#[cfg(test)] +mod tests { + use super::*; + use tantivy_core::schema::{FieldsDescriptor, IndexConfig}; + + fn cfg(path: &str) -> IndexConfig { + IndexConfig { + path: path.to_string(), + id_field: "id_key".into(), + fields: FieldsDescriptor { + text: vec!["title".into()], + keys: vec!["id_key".into()], + attributes: vec![], + }, + writer_heap_bytes: 15_000_000, + } + } + + #[test] + fn dropping_index_releases_the_registry_handle() { + let dir = std::env::temp_dir().join(format!("tv_ext_drop_{}", std::process::id())); + let _ = std::fs::remove_dir_all(&dir); + + let handle = registry::open_or_create(cfg(dir.to_str().unwrap())).unwrap(); + let index = Index { handle }; + drop(index); + + // Si Drop cerró el handle, close() acá devuelve false (ya no está). Sin Drop, el estado + // seguiría en el registro (fuga) y close() devolvería true. + assert!( + !registry::close(handle), + "Drop debía liberar el handle del registro" + ); + + let _ = std::fs::remove_dir_all(&dir); + } +} diff --git a/crates/tantivy-ffi/Cargo.toml b/crates/tantivy-ffi/Cargo.toml deleted file mode 100644 index e0a8411..0000000 --- a/crates/tantivy-ffi/Cargo.toml +++ /dev/null @@ -1,12 +0,0 @@ -[package] -name = "tantivy-ffi" -version.workspace = true -edition.workspace = true - -[lib] -name = "tantivyphp" -crate-type = ["cdylib", "rlib"] - -[dependencies] -tantivy-core.workspace = true -serde_json.workspace = true diff --git a/crates/tantivy-ffi/src/error.rs b/crates/tantivy-ffi/src/error.rs deleted file mode 100644 index 1fb569c..0000000 --- a/crates/tantivy-ffi/src/error.rs +++ /dev/null @@ -1,59 +0,0 @@ -use std::cell::RefCell; -use std::ffi::CString; -use std::os::raw::c_char; - -thread_local! { - static LAST_ERROR: RefCell> = const { RefCell::new(None) }; -} - -/// Guarda el último error del thread actual. -pub fn set_last_error(msg: &str) { - LAST_ERROR.with(|e| *e.borrow_mut() = Some(msg.to_owned())); -} - -/// Consume y devuelve el último error del thread actual. -pub fn take_last_error() -> Option { - LAST_ERROR.with(|e| e.borrow_mut().take()) -} - -/// Ejecuta `f`, atrapa panics y errores, guarda el mensaje y devuelve `default` ante fallo. -pub fn ffi_guard(default: T, f: impl FnOnce() -> Result + std::panic::UnwindSafe) -> T { - match std::panic::catch_unwind(f) { - Ok(Ok(v)) => v, - Ok(Err(msg)) => { - set_last_error(&msg); - default - } - Err(_) => { - set_last_error("panic en el borde FFI"); - default - } - } -} - -#[no_mangle] -pub extern "C" fn tv_last_error() -> *mut c_char { - match take_last_error() { - Some(msg) => CString::new(msg).unwrap_or_default().into_raw(), - None => std::ptr::null_mut(), - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn guard_stores_error_and_returns_default() { - let out = ffi_guard(-1_i64, || Err("boom".to_string())); - assert_eq!(out, -1); - assert_eq!(take_last_error(), Some("boom".to_string())); - } - - #[test] - fn guard_catches_panic() { - let out = ffi_guard(0_u64, || panic!("kaboom")); - assert_eq!(out, 0); - assert!(take_last_error().unwrap().contains("panic")); - } -} diff --git a/crates/tantivy-ffi/src/ffi.rs b/crates/tantivy-ffi/src/ffi.rs deleted file mode 100644 index 93303f4..0000000 --- a/crates/tantivy-ffi/src/ffi.rs +++ /dev/null @@ -1,138 +0,0 @@ -use std::ffi::{CStr, CString}; -use std::os::raw::c_char; - -use crate::error::ffi_guard; -use tantivy_core::{query, registry, writer}; - -/// Convierte un puntero C a &str; error si es null o no-UTF8. -fn cstr<'a>(p: *const c_char) -> Result<&'a str, String> { - if p.is_null() { - return Err("puntero nulo".into()); - } - unsafe { CStr::from_ptr(p) } - .to_str() - .map_err(|_| "cadena no UTF-8".into()) -} - -fn out(s: String) -> *mut c_char { - CString::new(s).unwrap_or_default().into_raw() -} - -#[no_mangle] -pub extern "C" fn tv_version() -> *mut c_char { - out(env!("CARGO_PKG_VERSION").to_string()) -} - -#[no_mangle] -pub extern "C" fn tv_string_free(s: *mut c_char) { - if !s.is_null() { - unsafe { drop(CString::from_raw(s)) }; - } -} - -#[no_mangle] -pub extern "C" fn tv_index_open_or_create(config_json: *const c_char) -> u64 { - ffi_guard(0, || { - let cfg = serde_json::from_str(cstr(config_json)?) - .map_err(|e| format!("config JSON inválido: {e}"))?; - registry::open_or_create(cfg) - }) -} - -#[no_mangle] -pub extern "C" fn tv_index_open_readonly(config_json: *const c_char) -> u64 { - ffi_guard(0, || { - let cfg = serde_json::from_str(cstr(config_json)?) - .map_err(|e| format!("config JSON inválido: {e}"))?; - registry::open_read_only(cfg) - }) -} - -#[no_mangle] -pub extern "C" fn tv_index_close(handle: u64) -> i32 { - ffi_guard(-1, || Ok(if registry::close(handle) { 0 } else { -1 })) -} - -#[no_mangle] -pub extern "C" fn tv_add_document(handle: u64, doc_json: *const c_char) -> i32 { - ffi_guard(-1, || { - let doc = cstr(doc_json)?.to_owned(); - registry::with_state(handle, |s| writer::add_document(s, &doc))?; - Ok(0) - }) -} - -#[no_mangle] -pub extern "C" fn tv_update_document( - handle: u64, - key_field: *const c_char, - key_value: *const c_char, - doc_json: *const c_char, -) -> i32 { - ffi_guard(-1, || { - let kf = cstr(key_field)?.to_owned(); - let kv = cstr(key_value)?.to_owned(); - let doc = cstr(doc_json)?.to_owned(); - registry::with_state(handle, |s| writer::update_document(s, &kf, &kv, &doc))?; - Ok(0) - }) -} - -#[no_mangle] -pub extern "C" fn tv_delete_document( - handle: u64, - key_field: *const c_char, - key_value: *const c_char, -) -> i32 { - ffi_guard(-1, || { - let kf = cstr(key_field)?.to_owned(); - let kv = cstr(key_value)?.to_owned(); - registry::with_state(handle, |s| writer::delete_by_id(s, &kf, &kv))?; - Ok(0) - }) -} - -#[no_mangle] -pub extern "C" fn tv_commit(handle: u64) -> i32 { - ffi_guard(-1, || { - registry::with_state(handle, writer::commit)?; - Ok(0) - }) -} - -#[no_mangle] -pub extern "C" fn tv_optimize(_handle: u64) -> i32 { - // v1: no-op (merge se agrega en el plan de rebuild). Nunca falla. - 0 -} - -#[no_mangle] -pub extern "C" fn tv_doc_count(handle: u64) -> i64 { - ffi_guard(-1, || { - let n = registry::with_state(handle, |s| s.doc_count())?; - Ok(n as i64) - }) -} - -#[no_mangle] -pub extern "C" fn tv_search(handle: u64, query_json: *const c_char) -> *mut c_char { - ffi_guard(std::ptr::null_mut(), || { - let q = cstr(query_json)?.to_owned(); - let json = registry::with_state(handle, |s| query::search(s, &q))?; - Ok(out(json)) - }) -} - -#[cfg(test)] -mod tests { - use super::*; - use std::ffi::CStr; - - #[test] - fn version_roundtrips_through_c_string() { - let ptr = tv_version(); - let got = unsafe { CStr::from_ptr(ptr) }.to_str().unwrap().to_owned(); - tv_string_free(ptr); - assert_eq!(got, env!("CARGO_PKG_VERSION")); - } -} diff --git a/crates/tantivy-ffi/src/lib.rs b/crates/tantivy-ffi/src/lib.rs deleted file mode 100644 index 78c1103..0000000 --- a/crates/tantivy-ffi/src/lib.rs +++ /dev/null @@ -1,9 +0,0 @@ -// las funciones del borde C-ABI reciben punteros crudos por diseño (contrato con el caller C/PHP); -// el lint not_unsafe_ptr_arg_deref no aplica a funciones extern "C" pensadas para llamarse desde C. -#![allow(clippy::not_unsafe_ptr_arg_deref)] - -mod error; -mod ffi; - -pub use error::tv_last_error; -pub use ffi::*; diff --git a/crates/tantivy-ffi/tests/ffi_roundtrip.rs b/crates/tantivy-ffi/tests/ffi_roundtrip.rs deleted file mode 100644 index a3867bd..0000000 --- a/crates/tantivy-ffi/tests/ffi_roundtrip.rs +++ /dev/null @@ -1,41 +0,0 @@ -use std::ffi::{CStr, CString}; - -use tantivyphp::*; - -fn c(s: &str) -> CString { - CString::new(s).unwrap() -} - -#[test] -fn full_roundtrip_through_c_abi() { - let dir = std::env::temp_dir().join(format!("tv_ffi_{}", std::process::id())); - let _ = std::fs::remove_dir_all(&dir); - let cfg = format!( - r#"{{"path":"{}","id_field":"id_key", - "fields":{{"text":["title"],"keys":["id_key"],"attributes":[]}}, - "writer_heap_bytes":15000000}}"#, - dir.to_str().unwrap() - ); - - let h = tv_index_open_or_create(c(&cfg).as_ptr()); - assert!(h != 0); - - assert_eq!(tv_add_document(h, c(r#"{"id_key":"1","title":"reset password"}"#).as_ptr()), 0); - // NRT: el add no commitea, así que todavía no es visible. Recién el commit explícito lo publica. - assert_eq!(tv_doc_count(h), 0); - assert_eq!(tv_commit(h), 0); - assert_eq!(tv_doc_count(h), 1); - - let res_ptr = tv_search(h, c(r#"{"text":"reset","text_fields":["title"],"limit":5}"#).as_ptr()); - assert!(!res_ptr.is_null()); - let res = unsafe { CStr::from_ptr(res_ptr) }.to_str().unwrap().to_owned(); - tv_string_free(res_ptr); - assert!(res.contains("\"id_key\":\"1\"")); - - assert_eq!(tv_delete_document(h, c("id_key").as_ptr(), c("1").as_ptr()), 0); - assert_eq!(tv_commit(h), 0); - assert_eq!(tv_doc_count(h), 0); - - assert_eq!(tv_index_close(h), 0); - let _ = std::fs::remove_dir_all(&dir); -} diff --git a/docs/BUILD.md b/docs/BUILD.md index 8fdf680..18c5820 100644 --- a/docs/BUILD.md +++ b/docs/BUILD.md @@ -1,46 +1,30 @@ # Build -The workspace produces two independent artifacts. Build both with `cargo build --release`. - -## FFI backend — `libtantivyphp.so` / `tantivyphp.dll` - -A plain cdylib loaded from PHP via `FFI::cdef`. PHP-version-independent. - -### Linux (NTS) -``` -cargo build --release # -> target/release/libtantivyphp.so -``` - -### Windows (NTS) -``` -rustup target add x86_64-pc-windows-msvc -cargo build --release --target x86_64-pc-windows-msvc -# -> target/x86_64-pc-windows-msvc/release/tantivyphp.dll -``` -Plain cdylib: no PHP SDK, no ext-php-rs, no nightly required. - -### Verify -``` -TANTIVYPHP_LIB=target/release/libtantivyphp.so php -d ffi.enable=1 php/tests/smoke.php -``` +The workspace produces one artifact: the native PHP extension. Build it with `cargo build --release`. ## Native extension — `libtantivyphp_ext.so` / `tantivyphp_ext.dll` -A native PHP extension built with `ext-php-rs`, loaded via `extension=`. No `ffi.enable` -needed. Links against the PHP ABI, so it is built per PHP minor (currently 8.4) and per -thread-safety mode (NTS). +A native PHP extension built with `ext-php-rs`, loaded via `extension=`. Links against the PHP +ABI, so it is built per PHP minor (currently 8.4) and per thread-safety mode (NTS). ### Linux (NTS) Requires PHP 8.4 dev headers (`php-config` on PATH) and Clang. ``` -cargo build --release -p tantivy-ext # -> target/release/libtantivyphp_ext.so +cargo build --release # -> target/release/libtantivyphp_ext.so ``` ### Windows (NTS) Requires Rust **nightly** (vectorcall calling convention), MSVC (`cl.exe`) + `rust-lld`, and a PHP 8.4 NTS SDK from windows.php.net. ``` -cargo +nightly build --release -p tantivy-ext # -> target/release/tantivyphp_ext.dll +cargo +nightly build --release # -> target/release/tantivyphp_ext.dll +``` + +### Install +Drop the built library into your PHP `extension_dir` (rename to `tantivyphp.so` if you like) +and load it: +```ini +extension=tantivyphp_ext.so ``` ### Verify @@ -48,5 +32,5 @@ cargo +nightly build --release -p tantivy-ext # -> target/release/tantivyphp_e php -d extension=target/release/libtantivyphp_ext.so php/tests/smoke.php ``` -`php/tests/smoke.php` is backend-agnostic: it uses the `Tantivy\Client` facade, which selects -the native extension when it is loaded and otherwise falls back to FFI. +`php/tests/smoke.php` uses the `Tantivy\Client` facade, which requires the native extension +to be loaded (it errors with an actionable message otherwise). diff --git a/include/tantivyphp.h b/include/tantivyphp.h deleted file mode 100644 index 92c8045..0000000 --- a/include/tantivyphp.h +++ /dev/null @@ -1,18 +0,0 @@ -#define FFI_SCOPE "TANTIVYPHP" -#define FFI_LIB "libtantivyphp.so" - -char* tv_version(void); -void tv_string_free(char* s); -char* tv_last_error(void); - -unsigned long long tv_index_open_or_create(const char* config_json); -unsigned long long tv_index_open_readonly(const char* config_json); -int tv_index_close(unsigned long long handle); - -int tv_add_document(unsigned long long handle, const char* doc_json); -int tv_update_document(unsigned long long handle, const char* key_field, const char* key_value, const char* doc_json); -int tv_delete_document(unsigned long long handle, const char* key_field, const char* key_value); -int tv_commit(unsigned long long handle); -int tv_optimize(unsigned long long handle); -long long tv_doc_count(unsigned long long handle); -char* tv_search(unsigned long long handle, const char* query_json); diff --git a/php/composer.json b/php/composer.json index b6386bc..ee88b63 100644 --- a/php/composer.json +++ b/php/composer.json @@ -1,12 +1,9 @@ { "name": "invgate/php-tantivy", - "description": "PHP client for the tantivy search engine, via a Rust cdylib (FFI) or a native ext-php-rs extension.", + "description": "PHP client for the tantivy search engine, via a native ext-php-rs extension.", "type": "library", "license": "MIT OR Apache-2.0", "require": { "php": ">=8.4" }, - "suggest": { - "ext-ffi": "Required only for the FFI backend (FfiClient). Not needed when the native tantivyphp extension is loaded." - }, "autoload": { "psr-4": { "Tantivy\\": "src/" } }, "autoload-dev": { "psr-4": { "Tantivy\\Tests\\": "tests/" } } } diff --git a/php/src/Client.php b/php/src/Client.php index 5d9076c..8ee38e0 100644 --- a/php/src/Client.php +++ b/php/src/Client.php @@ -5,26 +5,34 @@ namespace Tantivy; /** - * Fachada que selecciona el backend disponible. Es el ÚNICO lugar que sabe que existen dos - * implementaciones; los consumidores sólo dependen de esta clase y de ClientInterface. - * Cuando un backend gane a mediano plazo, se borra su clase perdedora y esta rama del if. + * Punto de entrada público. Los consumidores dependen sólo de esta clase y de ClientInterface; + * la implementación concreta (ExtClient, sobre la extensión nativa ext-php-rs) queda encapsulada. */ final class Client { public static function openOrCreate(array $config): ClientInterface { - return (self::backend())::openOrCreate($config); + self::ensureExtensionLoaded(); + return ExtClient::openOrCreate($config); } public static function openReadOnly(array $config): ClientInterface { - return (self::backend())::openReadOnly($config); + self::ensureExtensionLoaded(); + return ExtClient::openReadOnly($config); } - /** @return class-string */ - private static function backend(): string + /** + * La extensión nativa registra la clase Tantivy\Native\Index al cargarse. Si no está, fallamos + * con un mensaje accionable en vez de dejar reventar un "class not found" opaco más abajo. + */ + private static function ensureExtensionLoaded(): void { - // detecta la extensión nativa por la clase que registra (más robusto que el nombre del módulo). - return \class_exists('\\Tantivy\\Native\\Index') ? ExtClient::class : FfiClient::class; + if (!\class_exists('\\Tantivy\\Native\\Index')) { + throw new TantivyException( + 'La extensión nativa tantivyphp no está cargada (falta Tantivy\\Native\\Index). ' + . 'Cargala con extension=tantivyphp.so (o tantivyphp_ext.so) en tu php.ini.' + ); + } } } diff --git a/php/src/ExtClient.php b/php/src/ExtClient.php index 49c9701..52d4111 100644 --- a/php/src/ExtClient.php +++ b/php/src/ExtClient.php @@ -6,8 +6,8 @@ /** * Backend nativo (ext-php-rs). Wrapper delgado sobre la clase nativa Tantivy\Native\Index que - * registra la extensión. Traduce cualquier error nativo a TantivyException para que el tipo de - * excepción sea idéntico al del backend FFI. Mismo contrato JSON que FfiClient. + * registra la extensión. Traduce cualquier error nativo a TantivyException. Contrato JSON estable + * con el núcleo Rust (mismos campos de config/doc/query). */ final class ExtClient implements ClientInterface { @@ -15,6 +15,22 @@ private function __construct(private readonly \Tantivy\Native\Index $index) { } + /** + * Red de seguridad determinística: libera el índice al destruirse el cliente, sin depender de que + * el consumidor llame close() a mano. El estado del índice vive en un registro global del proceso + * (en Rust), así que no cerrarlo lo fuga por toda la vida del worker (reader mmap + FDs de + * segmentos). El Drop nativo de Tantivy\Native\Index también lo libera al liberarse el objeto; + * close() es idempotente, así que ambos caminos coexisten sin problema. + */ + public function __destruct() + { + try { + $this->index->close(); + } catch (\Throwable) { + // cerrar nunca debe tirar desde un destructor. + } + } + public static function openOrCreate(array $config): ClientInterface { try { diff --git a/php/src/FfiClient.php b/php/src/FfiClient.php deleted file mode 100644 index 833e1c0..0000000 --- a/php/src/FfiClient.php +++ /dev/null @@ -1,137 +0,0 @@ -tv_last_error(); - if ($ptr === null) { - return 'error desconocido'; - } - $msg = FFI::string($ptr); - self::ffi()->tv_string_free($ptr); - return $msg; - } - - public static function openOrCreate(array $config): ClientInterface - { - $handle = self::ffi()->tv_index_open_or_create(self::json($config)); - if ($handle === 0) { - throw new TantivyException('open_or_create falló: ' . self::lastError()); - } - return new self($handle); - } - - /** - * Abre un índice EXISTENTE en modo solo-lectura (para búsquedas). Falla si no existe: - * no crea ni muta el índice, así una búsqueda no requiere permisos de escritura y un - * índice no construido falla explícito en vez de crear uno vacío ("0 resultados"). - */ - public static function openReadOnly(array $config): ClientInterface - { - $handle = self::ffi()->tv_index_open_readonly(self::json($config)); - if ($handle === 0) { - throw new TantivyException('open_read_only falló: ' . self::lastError()); - } - return new self($handle); - } - - public function addDocument(array $doc): void - { - if (self::ffi()->tv_add_document($this->handle, self::json($doc)) !== 0) { - throw TantivyException::forOperation('add_document', self::lastError()); - } - } - - public function updateDocument(string $keyField, string $keyValue, array $doc): void - { - if (self::ffi()->tv_update_document($this->handle, $keyField, $keyValue, self::json($doc)) !== 0) { - throw TantivyException::forOperation('update_document', self::lastError()); - } - } - - public function deleteDocument(string $keyField, string $keyValue): void - { - if (self::ffi()->tv_delete_document($this->handle, $keyField, $keyValue) !== 0) { - throw TantivyException::forOperation('delete_document', self::lastError()); - } - } - - public function commit(): void - { - if (self::ffi()->tv_commit($this->handle) !== 0) { - throw new TantivyException('commit falló: ' . self::lastError()); - } - } - - public function optimize(): void - { - self::ffi()->tv_optimize($this->handle); - } - - public function documentCount(): int - { - $n = self::ffi()->tv_doc_count($this->handle); - if ($n < 0) { - throw new TantivyException('doc_count falló: ' . self::lastError()); - } - return (int) $n; - } - - /** - * @return list}> - */ - public function search(array $query): array - { - $ptr = self::ffi()->tv_search($this->handle, self::json($query)); - if ($ptr === null) { - throw new TantivyException('search falló: ' . self::lastError()); - } - $json = FFI::string($ptr); - self::ffi()->tv_string_free($ptr); - try { - $decoded = json_decode($json, true, 512, JSON_THROW_ON_ERROR); - } catch (\JsonException $e) { - // no tragar una respuesta malformada como "0 resultados": es un error real del backend. - throw TantivyException::forOperation('search', 'respuesta JSON inválida', $e); - } - return $decoded['hits'] ?? []; - } - - public function close(): void - { - self::ffi()->tv_index_close($this->handle); - } - - private static function json(array $data): string - { - return json_encode($data, JSON_UNESCAPED_UNICODE | JSON_THROW_ON_ERROR); - } -} diff --git a/php/tests/bootstrap.php b/php/tests/bootstrap.php index 7e3753c..4d98609 100644 --- a/php/tests/bootstrap.php +++ b/php/tests/bootstrap.php @@ -6,7 +6,7 @@ // standalone, así que cargamos las clases directamente en lugar de depender // del autoload PSR-4 declarado en composer.json. require_once __DIR__ . '/../src/TantivyException.php'; +require_once __DIR__ . '/../src/IndexBusyException.php'; require_once __DIR__ . '/../src/ClientInterface.php'; -require_once __DIR__ . '/../src/FfiClient.php'; require_once __DIR__ . '/../src/ExtClient.php'; require_once __DIR__ . '/../src/Client.php'; diff --git a/php/tests/smoke.php b/php/tests/smoke.php index 48a827d..e192206 100644 --- a/php/tests/smoke.php +++ b/php/tests/smoke.php @@ -2,13 +2,12 @@ declare(strict_types=1); -// Smoke runner independiente del backend: usa la fachada Tantivy\Client, que elige FFI o ext -// según qué haya cargado. Sale con código != 0 y un mensaje si algo falla. +// Smoke runner: usa la fachada Tantivy\Client sobre la extensión nativa. +// Sale con código != 0 y un mensaje si algo falla. require __DIR__ . '/../src/TantivyException.php'; require __DIR__ . '/../src/IndexBusyException.php'; require __DIR__ . '/../src/ClientInterface.php'; -require __DIR__ . '/../src/FfiClient.php'; require __DIR__ . '/../src/ExtClient.php'; require __DIR__ . '/../src/Client.php'; @@ -37,9 +36,6 @@ function assert_that(bool $cond, string $msg): void 'writer_heap_bytes' => 15_000_000, ]; -$backend = \class_exists('\\Tantivy\\Native\\Index') ? 'ext' : 'ffi'; -fwrite(STDOUT, "smoke backend: $backend\n"); - $c = Client::openOrCreate($config); $c->addDocument(['id_key' => '1', 'title' => 'reset password']); $c->commit(); @@ -54,5 +50,5 @@ function assert_that(bool $cond, string $msg): void assert_that($c->documentCount() === 0, 'documentCount debe ser 0 tras delete+commit'); $c->close(); -fwrite(STDOUT, "SMOKE OK ($backend)\n"); +fwrite(STDOUT, "SMOKE OK (ext)\n"); exit(0);