-
Notifications
You must be signed in to change notification settings - Fork 12
refactor: centralize async FFI completion handling #68
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,214 @@ | ||
| // SPDX-License-Identifier: Apache-2.0 | ||
| // SPDX-FileCopyrightText: Copyright The Lance Authors | ||
|
|
||
| //! Reusable bridge from Lance futures to C callback completions. | ||
|
|
||
| use std::ffi::c_void; | ||
| use std::future::Future; | ||
|
|
||
| use futures::FutureExt; | ||
|
|
||
| use crate::async_dispatcher::Completion; | ||
| use crate::error::{LanceErrorCode, error_code_from_lance, panic_payload_message, swallow_unwind}; | ||
| use crate::runtime::RT; | ||
|
|
||
| /// Spawn a Lance future and translate its terminal state into exactly one C | ||
| /// completion. | ||
| /// | ||
| /// `into_result` converts the successful Rust value into an operation-specific | ||
| /// C result pointer. `on_panic` lets stateful callers poison or invalidate | ||
| /// state before the panic completion is delivered. | ||
| pub(crate) fn spawn_lance_future<F, T, S, P>( | ||
| completion: Completion, | ||
| future: F, | ||
| into_result: S, | ||
| on_panic: P, | ||
| ) where | ||
| F: Future<Output = lance_core::Result<T>> + Send + 'static, | ||
| T: Send + 'static, | ||
| S: FnOnce(T) -> *mut c_void + Send + 'static, | ||
| P: FnOnce() + Send + 'static, | ||
| { | ||
| RT.spawn(async move { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. catch_unwind starts inside the spawned task, so runtime initialization or RT.spawn panics are not caught by this helper. |
||
| let completion_on_panic = completion; | ||
| let outcome = std::panic::AssertUnwindSafe(async move { | ||
| match future.await { | ||
| Ok(value) => completion.succeed(into_result(value)), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The four bridge tests cover success, Lance error, future panic, and a panicking A fifth case mirroring |
||
| Err(err) => completion.fail(error_code_from_lance(&err), err.to_string()), | ||
| } | ||
| }) | ||
| .catch_unwind() | ||
| .await; | ||
|
|
||
| if let Err(payload) = outcome { | ||
| swallow_unwind("async FFI panic hook", on_panic); | ||
| completion_on_panic.fail( | ||
| LanceErrorCode::Panic, | ||
| format!("panic in FFI call: {}", panic_payload_message(&*payload)), | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The |
||
| ); | ||
| } | ||
| }); | ||
| } | ||
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
| use crate::async_dispatcher::LanceCallback; | ||
| use crate::error::{lance_free_string, lance_last_error_code, lance_last_error_message}; | ||
| use std::ffi::CStr; | ||
| use std::sync::atomic::{AtomicBool, Ordering}; | ||
| use std::sync::{Arc, mpsc}; | ||
| use std::time::Duration; | ||
|
|
||
| #[derive(Debug)] | ||
| struct Observation { | ||
| status: i32, | ||
| result: *mut c_void, | ||
| code: LanceErrorCode, | ||
| message: Option<String>, | ||
| } | ||
|
|
||
| unsafe impl Send for Observation {} | ||
|
|
||
| unsafe extern "C" fn observe(ctx: *mut c_void, status: i32, result: *mut c_void) { | ||
| let tx = unsafe { &*(ctx as *const mpsc::Sender<Observation>) }; | ||
| let code = lance_last_error_code(); | ||
| let message_ptr = lance_last_error_message(); | ||
| let message = if message_ptr.is_null() { | ||
| None | ||
| } else { | ||
| let message = unsafe { CStr::from_ptr(message_ptr) } | ||
| .to_string_lossy() | ||
| .into_owned(); | ||
| unsafe { lance_free_string(message_ptr) }; | ||
| Some(message) | ||
| }; | ||
| tx.send(Observation { | ||
| status, | ||
| result, | ||
| code, | ||
| message, | ||
| }) | ||
| .unwrap(); | ||
| } | ||
|
|
||
| fn completion() -> ( | ||
| Completion, | ||
| mpsc::Receiver<Observation>, | ||
| *mut mpsc::Sender<Observation>, | ||
| ) { | ||
| let (tx, rx) = mpsc::channel(); | ||
| let ctx = Box::into_raw(Box::new(tx)); | ||
| let callback: LanceCallback = observe; | ||
| let completion = unsafe { Completion::new(callback, ctx.cast()) }; | ||
| (completion, rx, ctx) | ||
| } | ||
|
|
||
| fn receive(rx: &mpsc::Receiver<Observation>) -> Observation { | ||
| rx.recv_timeout(Duration::from_secs(5)) | ||
| .expect("async bridge must deliver completion") | ||
| } | ||
|
|
||
| #[test] | ||
| fn success_is_converted_and_delivered() { | ||
| let (completion, rx, ctx) = completion(); | ||
| spawn_lance_future( | ||
| completion, | ||
| async { Ok::<_, lance_core::Error>(42_u64) }, | ||
| |value| Box::into_raw(Box::new(value)).cast(), | ||
| || panic!("success must not invoke on_panic"), | ||
| ); | ||
|
|
||
| let observation = receive(&rx); | ||
| assert_eq!(observation.status, 0); | ||
| assert_eq!(observation.code, LanceErrorCode::Ok); | ||
| assert!(observation.message.is_none()); | ||
| assert_eq!( | ||
| unsafe { *Box::from_raw(observation.result.cast::<u64>()) }, | ||
| 42 | ||
| ); | ||
| unsafe { drop(Box::from_raw(ctx)) }; | ||
| } | ||
|
|
||
| #[test] | ||
| fn lance_error_is_delivered_without_running_converter() { | ||
| let (completion, rx, ctx) = completion(); | ||
| spawn_lance_future::<_, (), _, _>( | ||
| completion, | ||
| async { | ||
| Err(lance_core::Error::invalid_input_source( | ||
| "invalid async input".into(), | ||
| )) | ||
| }, | ||
| |_| panic!("error must not run success converter"), | ||
| || panic!("ordinary error must not invoke on_panic"), | ||
| ); | ||
|
|
||
| let observation = receive(&rx); | ||
| assert_eq!(observation.status, -1); | ||
| assert!(observation.result.is_null()); | ||
| assert_eq!(observation.code, LanceErrorCode::InvalidArgument); | ||
| assert!( | ||
| observation | ||
| .message | ||
| .as_deref() | ||
| .is_some_and(|message| message.contains("invalid async input")) | ||
| ); | ||
| unsafe { drop(Box::from_raw(ctx)) }; | ||
| } | ||
|
|
||
| #[test] | ||
| fn panic_runs_hook_and_delivers_panic_error() { | ||
| let (completion, rx, ctx) = completion(); | ||
| let panicked = Arc::new(AtomicBool::new(false)); | ||
| let panicked_in_hook = Arc::clone(&panicked); | ||
| spawn_lance_future::<_, (), _, _>( | ||
| completion, | ||
| async { | ||
| panic!("async bridge panic"); | ||
| #[allow(unreachable_code)] | ||
| Ok(()) | ||
| }, | ||
| |_| std::ptr::null_mut(), | ||
| move || panicked_in_hook.store(true, Ordering::SeqCst), | ||
| ); | ||
|
|
||
| let observation = receive(&rx); | ||
| assert!(panicked.load(Ordering::SeqCst)); | ||
| assert_eq!(observation.status, -1); | ||
| assert_eq!(observation.code, LanceErrorCode::Panic); | ||
| assert!( | ||
| observation | ||
| .message | ||
| .as_deref() | ||
| .is_some_and(|message| message.contains("async bridge panic")) | ||
| ); | ||
| unsafe { drop(Box::from_raw(ctx)) }; | ||
| } | ||
|
|
||
| #[test] | ||
| fn panicking_hook_does_not_suppress_completion() { | ||
| let (completion, rx, ctx) = completion(); | ||
| spawn_lance_future::<_, (), _, _>( | ||
| completion, | ||
| async { | ||
| panic!("original async panic"); | ||
| #[allow(unreachable_code)] | ||
| Ok(()) | ||
| }, | ||
| |_| std::ptr::null_mut(), | ||
| || panic!("panic hook also panicked"), | ||
| ); | ||
|
|
||
| let observation = receive(&rx); | ||
| assert_eq!(observation.status, -1); | ||
| assert_eq!(observation.code, LanceErrorCode::Panic); | ||
| assert!( | ||
| observation | ||
| .message | ||
| .as_deref() | ||
| .is_some_and(|message| message.contains("original async panic")) | ||
| ); | ||
| unsafe { drop(Box::from_raw(ctx)) }; | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,50 @@ use crate::error::{LanceErrorCode, clear_last_error, panic_payload_message, set_ | |
| /// - `result`: operation-specific result pointer (e.g., `*mut ArrowArrayStream`) | ||
| pub type LanceCallback = unsafe extern "C" fn(ctx: *mut c_void, status: i32, result: *mut c_void); | ||
|
|
||
| /// A copyable async completion endpoint whose raw context is owned by the host. | ||
| /// | ||
| /// This centralizes the unsafe `void *` transport and status/error mapping used | ||
| /// by spawned FFI futures. The caller must keep `callback_ctx` valid until the | ||
| /// callback returns. | ||
| #[derive(Clone, Copy)] | ||
| pub(crate) struct Completion { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Dropping |
||
| callback: LanceCallback, | ||
| callback_ctx: *mut c_void, | ||
| } | ||
|
|
||
| // Safety: construction requires the caller to uphold the public callback | ||
| // contract: the callback is thread-safe and callback_ctx remains valid until | ||
| // completion delivery returns. | ||
| unsafe impl Send for Completion {} | ||
|
|
||
| impl Completion { | ||
| /// Construct a completion endpoint from a validated callback and context. | ||
| /// | ||
| /// # Safety | ||
| /// `callback_ctx` must remain valid until `callback` returns, and | ||
| /// `callback` must be safe to invoke from any completion-delivery thread. | ||
| pub(crate) unsafe fn new(callback: LanceCallback, callback_ctx: *mut c_void) -> Self { | ||
| Self { | ||
| callback, | ||
| callback_ctx, | ||
| } | ||
| } | ||
|
|
||
| pub(crate) fn succeed(self, result: *mut c_void) { | ||
| dispatch_callback(self.callback, self.callback_ctx, 0, result, None); | ||
| } | ||
|
|
||
| pub(crate) fn fail(self, code: LanceErrorCode, message: impl Into<String>) { | ||
| dispatch_callback( | ||
| self.callback, | ||
| self.callback_ctx, | ||
| -1, | ||
| std::ptr::null_mut(), | ||
| Some((code, message.into())), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Safety: LanceCallback is a C function pointer (Send by definition for FFI). | ||
| // The ctx pointer is transferred to the dispatcher thread which calls the callback. | ||
| unsafe impl Send for DispatcherMessage {} | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spawn_lance_futurecentralizes the easy template (spawn + catch_unwind + dispatch), but the four things the next async API must get right still rest on caller discipline: the entry panic guard (scan_async_guarded), the dual-channel validation report (caller-threadset_last_error+completion.fail, the issue #61 contract), the NULL-callback preamble, and the per-handle-type panic policy the caller must choose (the scanner rejects via a poison flag; the dataset is deliberately poison-tolerant, so a no-opon_panicthere would be by design — just undocumented). Missing any of these compiles fine and no test fails.Could the guarded entry and the dual-channel reporting fold into the bridge — say a
Completion::fail_validation(set TLS + dispatch) plus a guarded entry helper? At minimum, listing these caller obligations in the doc comment would make the reuse claim cover the half that matters.