diff --git a/docs/adr/009-ffi-bridge-reliability.md b/docs/adr/009-ffi-bridge-reliability.md index d188a0de..982fdcef 100644 --- a/docs/adr/009-ffi-bridge-reliability.md +++ b/docs/adr/009-ffi-bridge-reliability.md @@ -66,7 +66,7 @@ different dispatch mechanism: | Adapter | Dispatch model | Respond path | |---------|----------------|--------------| | Node | Callback via `ThreadsafeFunction` (push) | Synchronous NAPI call | -| Deno | mpsc queue + `nextRequest()` polling (pull) | Synchronous FFI symbol | +| Deno | Bounded mpsc queue + `UnsafeCallback` wake (push notification, synchronous drain) | Synchronous FFI symbol | | Tauri | Tauri `Channel` event stream (push) | Typed invoke command | The Node adapter has proven reliable under high concurrency (32-stream bursts, @@ -74,10 +74,12 @@ The Node adapter has proven reliable under high concurrency (32-stream bursts, conditions that produce `unknown handle` and `sendChunk failed` errors under the same load that Node handles cleanly. -The root causes are architectural, not incidental: the Deno adapter's -JSON-over-FFI + mpsc + polling design introduces latency between handle -allocation in Rust and handle consumption in JS. That latency window is where -every observed race occurs. +The original root causes were architectural, not incidental: Deno's former +JSON-over-FFI + async `nextRequest()` path introduced latency between handle +allocation in Rust and handle consumption in JS. Issue #122 replaced that path +with synchronous queue draining to avoid `spawn_blocking` starvation. Issue +#397 retained that safe drain path but replaced its hot idle polling with a +thread-safe callback that only notifies JS when work is ready. This exploration asks what architectural changes would bring Deno (and future adapters) to the same reliability level as Node and Tauri. @@ -85,8 +87,7 @@ adapters) to the same reliability level as Node and Tauri. ## Questions 1. Should the Deno adapter move from a polling (`nextRequest`) model to a - push model (e.g. a callback-based or channel-based mechanism), and what - would that look like given Deno's FFI constraints? + push-notified model while retaining synchronous queue draining? 2. Should `on_request` in the core avoid spawning detached tokio tasks, and instead use a synchronous (non-spawn) sender so the adapter controls the async boundary? @@ -121,11 +122,12 @@ libuv event loop. The NAPI `rawRespond` call is synchronous from the JS thread into the Rust slab. There is no intermediate queue, no polling latency, and no detached task. -**Why Deno struggles:** The `on_request` callback spawns a detached tokio task -to `try_send` into an mpsc channel. JS polls with `nextRequest()`. Between -enqueue and dequeue, the Rust request task continues running — it may time out, -the drain may fire, or `stopServe` may remove the registry entry. By the time -JS calls `respond()`, the handle may already be gone. +**Why Deno struggled before #122:** The `on_request` callback used an async +`nextRequest()` dispatch through Deno's fixed `spawn_blocking` pool. Under +concurrent self-fetches, request delivery competed with fetch work and could +deadlock. Deno now uses a bounded queue, a request-ready callback carrying only +an opaque generation token, and synchronous `try_next_request` draining on the +JS thread. This preserves #122's deadlock avoidance without #397's idle poll. **Why Tauri is safe:** Tauri's `Channel` is owned by the command lifetime. Frontend and backend are separate processes, so there is no shared-runtime @@ -133,8 +135,10 @@ deadlock risk. The channel is push-based with fail-closed semantics. ### Constraints -- Deno's FFI (`dlopen`) does not support passing closures or callbacks from - Rust to JS — hence the polling model. +- Deno's FFI supports `UnsafeCallback.threadSafe()` for calls originating on + foreign Tokio threads. The callback pointer must outlive every possible + native invocation and should be unrefed when it must not keep the process + alive. - Respond must be synchronous in Deno to avoid deadlock when client and server share the same single-threaded event loop (documented in #122). - The core's `on_request` callback must not block the QUIC accept loop. @@ -150,6 +154,7 @@ deadlock risk. The channel is push-based with fail-closed semantics. | **C. InsertGuard ownership tokens** — Rust returns an owning guard with the handle; handle cannot be freed until guard is dropped | Explicit lifetime control | Adds RAII complexity across the FFI boundary; guards must be passed back | | **D. Move Deno to napi-rs** — use `napi-rs` for Deno (via `deno_napi` compat) instead of raw FFI | Same proven dispatch as Node | Adds napi dependency; Deno FFI is the official path and napi compat is not guaranteed | | **E. Tolerate stale handles** — make `respond`/`sendChunk`/`finishBody` return Ok for missing handles | No architectural change | Silently drops responses; masks real bugs | +| **F. Callback wake + synchronous drain** — retain the bounded queue and `try_next_request`, but invoke a thread-safe Deno callback with an opaque generation token after enqueue | Preserves #122 behavior, avoids borrowed payload pointers, sleeps when idle | Requires callback pointer lifetime and restart routing to be explicit | ## Implications @@ -164,13 +169,11 @@ deadlock risk. The channel is push-based with fail-closed semantics. ## Next steps -- [ ] Prototype Option B (remove `tokio::spawn` in Deno dispatch, use direct - `try_send`) — this is the smallest change and may be sufficient. -- [ ] If Option B is insufficient, prototype Option A (oneshot acknowledgment) - on a branch and benchmark the per-request overhead. -- [ ] Run the regression test from #122 (32-stream burst × 5 iterations) on - each prototype to confirm zero stale-handle errors. +- [x] Keep direct bounded `try_send` delivery and synchronous queue draining. +- [x] Add Option F's callback wake without passing request payload pointers. +- [x] Run the regression test from #122 (32-stream burst × 5 iterations) to + confirm zero stale-handle errors. - [ ] Audit whether any other adapter has a similar detached-task pattern that could surface under future load. -- [ ] Document the chosen dispatch contract in [architecture.md](../architecture.md) +- [x] Document the chosen dispatch contract in [architecture.md](../architecture.md) so future adapters (e.g. Python) follow the same pattern. diff --git a/docs/architecture.md b/docs/architecture.md index 12a09276..934bc0d8 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -100,7 +100,7 @@ Core's `serve()` accepts an `on_request: Arc>` and the fail-closed undeliverable fallback — are owned by the adapter: diff --git a/packages/iroh-http-deno/src/adapter.ts b/packages/iroh-http-deno/src/adapter.ts index d5ab32a6..d164874b 100644 --- a/packages/iroh-http-deno/src/adapter.ts +++ b/packages/iroh-http-deno/src/adapter.ts @@ -256,6 +256,11 @@ const lib = Deno.dlopen( result: "i32", nonblocking: false, }, + iroh_http_set_request_ready_callback: { + parameters: ["u64", "u64", "pointer"], + result: "i32", + nonblocking: false, + }, // #126: Split-fetch — sync start + sync poll, bypasses spawn_blocking. iroh_http_start_fetch: { parameters: ["buffer", "usize"], @@ -276,54 +281,64 @@ const lib = Deno.dlopen( } as const, ); -// ── Fast event-loop yield ───────────────────────────────────────────────────── -// -// #126: MessageChannel.postMessage gives ~0.017ms yields vs setTimeout(0)'s -// ~2.5ms in Deno. This is critical for same-process client+server latency -// where the serve loop and fetch completion both need the event loop. -// -// Creates a fresh channel per call to avoid keeping the event loop alive -// when all serve loops have stopped. - -function createYieldFn(): { - yield: () => Promise; - cancel: () => void; - close: () => void; -} { - const ch = new MessageChannel(); - let pendingResolve: (() => void) | null = null; +// ── Event-driven serve wakeups ──────────────────────────────────────────────── + +interface ServeWakeSignal { + wait(): Promise; + wake(): void; +} + +function createServeWakeSignal(): ServeWakeSignal { + let pending = false; + let resolve: (() => void) | undefined; return { - yield: () => - new Promise((resolve) => { - pendingResolve = resolve; - ch.port2.onmessage = () => { - pendingResolve = null; - resolve(); - }; - ch.port1.postMessage(undefined); - }), - // #115: Immediately resolve any in-flight yield so the polling loop - // can re-poll try_next_request and see the -1 shutdown sentinel - // without waiting for the next MessageChannel tick. - cancel: () => { - pendingResolve?.(); - pendingResolve = null; + wait(): Promise { + if (pending) { + pending = false; + return Promise.resolve(); + } + return new Promise((r) => { + resolve = r; + }); }, - close: () => { - ch.port1.close(); - ch.port2.close(); + wake(): void { + if (resolve) { + const current = resolve; + resolve = undefined; + current(); + } else { + pending = true; + } }, }; } // ── Per-endpoint serve cancellation ─────────────────────────────────────────── // -// #115: stopServe fires an async FFI call. The polling loop may be suspended -// in `await yielder.yield()` when the Rust side processes the stop. This map -// lets stopServe immediately cancel the yield so the loop re-polls and sees -// the -1 shutdown sentinel without a timing-dependent delay. +// #115: stopServe fires an async FFI call. Wake its request loop immediately; +// the native shutdown signal wakes it again once the -1 sentinel is visible. const serveCancellers = new Map void>(); +let servePollObserverForTesting: (() => void) | undefined; + +/** @internal Test-only observer for synchronous serve queue polls. */ +export function _observeServePollsForTesting( + observer: (() => void) | undefined, +): void { + servePollObserverForTesting = observer; +} + +let nextServeWakeToken = 1n; +const serveWakeSignals = new Map(); +const serveWakeCallback = Deno.UnsafeCallback.threadSafe( + { parameters: ["u64"], result: "void" } as const, + (token: bigint | number) => { + serveWakeSignals.get(BigInt(token))?.wake(); + }, +); +// Keep the module-lifetime pointer valid while allowing an idle process to exit. +serveWakeCallback.unref(); + // `serveStart` crosses the async JSON dispatch bridge. Keep its registration // promise so an immediate self-fetch or stop cannot overtake native setup. const serveStartOps = new Map>>(); @@ -785,7 +800,7 @@ function syncRespond( } /** - * Sync-poll serve loop (#122). + * Event-driven synchronous serve queue drain (#122, #397). * * Root cause of #122: `nextRequest` and `rawFetch` both go through * `iroh_http_call` with `nonblocking: true`, sharing Deno's fixed-size @@ -793,11 +808,11 @@ function syncRespond( * threads, `nextRequest` can't get a thread → circular deadlock → 60s * timeout fires. * - * Fix: `iroh_http_try_next_request` is a sync FFI symbol (`nonblocking: false`) - * that does `try_recv()` on the serve queue — O(1), runs on the JS thread, - * NEVER enters the thread pool. JS polls with `setTimeout(0)` yield when - * the queue is empty. Under load the queue always has items so there's no - * latency penalty. + * `iroh_http_try_next_request` remains a sync FFI symbol (`nonblocking: false`) + * that does `try_recv()` on the serve queue — O(1), runs on the JS thread, and + * never enters the thread pool. Rust invokes a module-lifetime, thread-safe + * `UnsafeCallback` with an opaque generation token when work arrives. JS then + * drains the queue synchronously until empty and sleeps until the next wake. * * The serve lifecycle still uses `serveStart` / `stopServe` through the * dispatch path (they're one-shot calls, no concurrency concern). @@ -812,6 +827,11 @@ export const rawServe: RawServeFn = ( ): Promise => { // FFI handle as BigInt — endpoint handles are u64 on the Rust side. const eh = BigInt(endpointHandle); + const wakeToken = nextServeWakeToken++; + const wakeSignal = createServeWakeSignal(); + serveWakeSignals.set(wakeToken, wakeSignal); + serveCancellers.set(endpointHandle, () => wakeSignal.wake()); + const serveStart = call>("serveStart", { endpointHandle, ...options.serveOptions, @@ -831,7 +851,7 @@ export const rawServe: RawServeFn = ( ); return serveStart.then( () => { - // Start connection event polling loop if a callback was supplied. + // Start the separate connection-event polling loop if requested. if (options.onConnectionEvent) { const onEv = options.onConnectionEvent; (async () => { @@ -864,21 +884,32 @@ export const rawServe: RawServeFn = ( // Per-call output buffer for try_next_request (reused across polls). let pollBuf = new Uint8Array(4096) as Uint8Array; + const registerResult = lib.symbols.iroh_http_set_request_ready_callback( + eh, + wakeToken, + serveWakeCallback.pointer, + ) as number; + if (registerResult < 0) { + serveWakeSignals.delete(wakeToken); + serveCancellers.delete(endpointHandle); + throw classifyError( + JSON.stringify({ + code: "INVALID_HANDLE", + message: "serve queue closed before callback registration", + }), + ); + } return (async () => { - const yielder = createYieldFn(); - // #115: Register cancel callback so stopServe can break the yield. - serveCancellers.set(endpointHandle, () => yielder.cancel()); - let pollCount = 0; try { while (true) { - // Sync poll — runs on JS thread, never enters spawn_blocking pool. + // Sync drain — runs on JS thread, never enters spawn_blocking pool. let n = lib.symbols.iroh_http_try_next_request( eh, pollBuf, BigInt(pollBuf.byteLength), ) as number; - pollCount++; + servePollObserverForTesting?.(); if (n < -1) { // Buffer too small — grow and retry immediately. @@ -896,11 +927,7 @@ export const rawServe: RawServeFn = ( } if (n === 0) { - // Queue empty — yield to the event loop so rawFetch results - // and I/O can be processed, then poll again. - // #126: MessageChannel gives ~0.017ms yields vs setTimeout(0)'s - // ~2.5ms — critical for same-process client+server latency. - await yielder.yield(); + await wakeSignal.wait(); continue; } @@ -939,7 +966,7 @@ export const rawServe: RawServeFn = ( await Promise.allSettled([...pending]); } finally { serveCancellers.delete(endpointHandle); - yielder.close(); + serveWakeSignals.delete(wakeToken); // #245: Drain the in-flight stopServe op (if any) before resolving // so no non-blocking FFI op survives past `handle.finished`. const stopOp = serveStopOps.get(endpointHandle); @@ -948,8 +975,10 @@ export const rawServe: RawServeFn = ( })(); }, ).catch(async (err) => { + serveCancellers.delete(endpointHandle); + serveWakeSignals.delete(wakeToken); // A stop requested while registration was failing still owns an async FFI - // op. Drain it even though the polling loop (and its finally block) never + // op. Drain it even though the request loop (and its finally block) never // existed, so `finished` cannot outlive a Deno op-sanitizer boundary. await waitForServeStop(endpointHandle); // close_all / closeEndpoint can win the race before serveStart returns. @@ -971,10 +1000,7 @@ export function makeAllocBodyWriter(endpointHandle: number): AllocBodyWriterFn { // Sends QUIC CONNECTION_CLOSE frames so peers observe a clean disconnect. function _closeAllSync(): void { lib.symbols.iroh_http_close_all(); - // #245: close_all removes the request queues, but a serve loop parked in - // `await yielder.yield()` only re-polls when its MessageChannel turn lands. - // Wake every loop explicitly so it sees the -1 sentinel promptly instead of - // relying on MessageChannel timing under load. + // Ensure every JS loop observes the native -1 shutdown sentinel promptly. for (const cancel of serveCancellers.values()) cancel(); } Deno.addSignalListener("SIGTERM", _closeAllSync); @@ -987,7 +1013,7 @@ globalThis.addEventListener("unload", _closeAllSync); /** * @internal Test-only hook that triggers the same path as the SIGINT/SIGTERM * signal handlers. Used by the regression test for issue #155 to verify that - * `iroh_http_close_all` wakes pending serve polling loops. + * `iroh_http_close_all` wakes pending serve request loops. */ export function _closeAllForTesting(): void { _closeAllSync(); @@ -1056,15 +1082,14 @@ export async function closeEndpoint( } export function stopServe(handle: number): void { - // #115: Cancel the yield immediately so the polling loop re-polls and - // sees the -1 shutdown sentinel without waiting for the MessageChannel. + // #115: Wake the drain loop while the native stop operation is starting. serveCancellers.get(handle)?.(); // #245: Register the in-flight op so the serve loop can await it before // resolving `finished`; otherwise this non-blocking FFI op can outlive the // test boundary and trip Deno's `sanitizeOps` leak detector under load. const op = (async () => { // Do not let stop overtake an in-flight serve registration. This also - // guarantees the polling loop has installed its cancellation hook first. + // guarantees the request loop has installed its cancellation hook first. await waitForServeStart(handle); await call>("stopServe", { endpointHandle: handle }); })().catch(() => {}); @@ -1428,7 +1453,7 @@ export class DenoAdapter extends IrohAdapter { }, callback: (payload: RequestPayload) => Promise, ): Promise { - // Delegate to the module-level rawServe which owns the polling loop. + // Delegate to the module-level rawServe which owns the request loop. return rawServe(endpointHandle, options, callback); } diff --git a/packages/iroh-http-deno/src/dispatch.rs b/packages/iroh-http-deno/src/dispatch.rs index b15f0f88..bba6414f 100644 --- a/packages/iroh-http-deno/src/dispatch.rs +++ b/packages/iroh-http-deno/src/dispatch.rs @@ -111,10 +111,11 @@ fn err_adapter(e: AdapterInputError) -> Value { } /// Deno request transport: enqueues each request onto the serve-registry mpsc -/// queue polled by the Deno event loop. Registry-miss, serve-shutdown, and -/// queue-full are Deno-local transport-reachability failures mapped to -/// [`Undeliverable`]; [`deliver_request`] then sends the fail-closed 503 and -/// finishes the response body (the latter fixes the pre-#315 hang). +/// queue, then wakes the Deno event loop to synchronously drain it. +/// Registry-miss, serve-shutdown, and queue-full are Deno-local +/// transport-reachability failures mapped to [`Undeliverable`]; +/// [`deliver_request`] then sends the fail-closed 503 and finishes the response +/// body (the latter fixes the pre-#315 hang). struct DenoTransport { handle: u64, } @@ -154,6 +155,7 @@ impl RequestTransport for DenoTransport { tracing::warn!("iroh-http-deno: serve queue full — dropping request with 503"); return Err(Undeliverable::new("deno serve queue full")); } + q.wake_request_loop(); Ok(()) } } @@ -842,7 +844,7 @@ async fn stop_serve(p: Value) -> Value { } }; ep.stop_serve(); - // Signal shutdown immediately so the JS polling loop stops dequeuing. + // Signal shutdown immediately so the JS drain loop stops dequeuing. // The watch channel persists its value — any in-flight or future // `try_next_request` call sees shutdown and returns -1. serve_registry::signal_shutdown(handle); @@ -868,8 +870,8 @@ async fn stop_serve(p: Value) -> Value { // callback reject promptly without a registry miss. The entry is // replaced by the next `serve_start` or removed by `close_endpoint`. // - // (Supersedes DENO-002 — the JS polling loop now stops via the - // shutdown watch channel, not channel disconnection.) + // (Supersedes DENO-002 — the JS drain loop now stops via the shutdown + // watch channel and request-ready wake callback, not channel disconnection.) // // #122: wait for the previous serve loop to fully terminate before returning. // Without this, a subsequent `serve_start` on the same endpoint would diff --git a/packages/iroh-http-deno/src/lib.rs b/packages/iroh-http-deno/src/lib.rs index 499b2648..34470273 100644 --- a/packages/iroh-http-deno/src/lib.rs +++ b/packages/iroh-http-deno/src/lib.rs @@ -11,6 +11,8 @@ //! - `iroh_http_respond` — send response head for a pending request //! - `iroh_http_finish_body` — signal body-complete (drop writer) //! - `iroh_http_cancel_reader`— cancel a body reader +//! - `iroh_http_try_next_request` — drain a ready serve request +//! - `iroh_http_set_request_ready_callback` — install serve wake notification //! //! All async symbols are `nonblocking: true` in the Deno `dlopen` call. //! Sync symbols are `nonblocking: false`. @@ -563,17 +565,40 @@ pub extern "C" fn iroh_http_cancel_reader(endpoint_handle: u64, handle: u64) -> 0 } -// ── Non-blocking serve request polling ──────────────────────────────────────── +// ── Event-driven serve request draining ─────────────────────────────────────── // // #122: The dispatch-based `nextRequest` (via `iroh_http_call` nonblocking:true) // deadlocks under concurrent load because it competes for Deno's // `spawn_blocking` thread pool with `rawFetch`. // -// Fix: `iroh_http_try_next_request` is a sync FFI symbol (`nonblocking: false`) -// that calls `try_recv()` on the serve queue — O(1), never blocks. -// JS polls it in a tight loop with `setTimeout(0)` yield when empty. -// This keeps request delivery on the JS thread, completely bypassing the -// thread pool that `rawFetch` uses. +// `iroh_http_try_next_request` is a sync FFI symbol (`nonblocking: false`) that +// calls `try_recv()` on the serve queue — O(1), never blocks. A thread-safe +// Deno callback wakes JS when the queue transitions to ready, then JS drains +// synchronously until empty. This bypasses the thread pool without idle polling. + +/// Register the request-ready callback for the current serve generation. +/// +/// The callback receives only the opaque `token`; request payloads remain owned +/// by the bounded Rust queue and are retrieved through +/// [`iroh_http_try_next_request`]. +/// +/// Returns `0` on success or `-1` if no serve queue exists for the endpoint. +/// +/// # Safety +/// `callback` must remain a valid function pointer for as long as the endpoint +/// can serve. The Deno adapter satisfies this with one module-lifetime callback. +#[unsafe(no_mangle)] +pub unsafe extern "C" fn iroh_http_set_request_ready_callback( + endpoint_handle: u64, + token: u64, + callback: extern "C" fn(u64), +) -> i32 { + let Some(queue) = serve_registry::get(endpoint_handle) else { + return -1; + }; + queue.set_request_ready_waker(serve_registry::RequestReadyWaker::new(callback, token)); + 0 +} /// Try to receive the next queued request without blocking. /// @@ -581,7 +606,7 @@ pub extern "C" fn iroh_http_cancel_reader(endpoint_handle: u64, handle: u64) -> /// /// Return value: /// - `n > 0` — bytes written; a request is available. -/// - `0` — queue empty; call again after yielding. +/// - `0` — queue empty; wait for the request-ready callback. /// - `-1` — serve stopped or queue removed; exit the loop. /// - `n < -1`— buffer too small; `|n|` bytes required. /// @@ -643,7 +668,7 @@ pub unsafe extern "C" fn iroh_http_try_next_request( /// frame before the process exits. #[unsafe(no_mangle)] pub extern "C" fn iroh_http_close_all() { - // Wake all serve polling loops first so JS handlers can settle before the + // Wake all serve drain loops first so JS handlers can settle before the // endpoints they hold are torn out from under them (regression: issue #155). serve_registry::shutdown_all(); registry::close_all_endpoints(); diff --git a/packages/iroh-http-deno/src/serve_registry.rs b/packages/iroh-http-deno/src/serve_registry.rs index 41dbe1ae..a821a19d 100644 --- a/packages/iroh-http-deno/src/serve_registry.rs +++ b/packages/iroh-http-deno/src/serve_registry.rs @@ -1,8 +1,9 @@ -//! Per-endpoint request queues for the serve polling model. +//! Per-endpoint request queues for the Deno serve bridge. //! -//! Because Deno FFI cannot receive Rust callbacks, the serve loop pushes each -//! incoming [`RequestPayload`] into an `mpsc` channel. The TypeScript adapter -//! polls by calling `nextRequest` repeatedly (each call awaits one item). +//! Incoming [`RequestPayload`] values stay in a bounded `mpsc` channel. A +//! lifetime-stable Deno `UnsafeCallback` only notifies the TypeScript adapter +//! that work is ready; TypeScript then synchronously drains the queue. No +//! borrowed payload pointer crosses the callback boundary. //! //! Connection events (peer connect/disconnect) are similarly queued — the //! TypeScript adapter polls them via `nextConnectionEvent`. @@ -16,13 +17,30 @@ use tokio::sync::mpsc; const QUEUE_CAPACITY: usize = 256; -/// A queued request ready to be delivered to the TypeScript polling loop. +/// A queued request ready to be drained by the TypeScript request loop. pub type QueuedRequest = serde_json::Value; /// A queued connection event (peer connect / disconnect). pub type QueuedConnectionEvent = serde_json::Value; -/// Receiver half — held in the registry, polled by `nextRequest` / `nextConnectionEvent`. +/// Callback that wakes the Deno event loop with an opaque serve-generation token. +#[derive(Clone, Copy)] +pub(crate) struct RequestReadyWaker { + callback: extern "C" fn(u64), + token: u64, +} + +impl RequestReadyWaker { + pub(crate) fn new(callback: extern "C" fn(u64), token: u64) -> Self { + Self { callback, token } + } + + fn wake(self) { + (self.callback)(self.token); + } +} + +/// Receiver half — held in the registry and drained by the TypeScript adapter. pub struct ServeQueue { pub tx: mpsc::Sender, pub rx: tokio::sync::Mutex>, @@ -35,6 +53,33 @@ pub struct ServeQueue { /// after `shutdown()` is triggered still see the closed state immediately. shutdown_tx: tokio::sync::watch::Sender, pub shutdown_rx: tokio::sync::watch::Receiver, + request_ready_waker: Mutex>, +} + +impl ServeQueue { + /// Install the request-ready callback and wake it if work arrived first. + pub fn set_request_ready_waker(&self, waker: RequestReadyWaker) { + *self + .request_ready_waker + .lock() + .unwrap_or_else(|e| e.into_inner()) = Some(waker); + + let request_waiting = self.rx.try_lock().map(|rx| !rx.is_empty()).unwrap_or(true); + if *self.shutdown_rx.borrow() || request_waiting { + waker.wake(); + } + } + + /// Notify the TypeScript adapter that it should drain the request queue. + pub fn wake_request_loop(&self) { + let waker = *self + .request_ready_waker + .lock() + .unwrap_or_else(|e| e.into_inner()); + if let Some(waker) = waker { + waker.wake(); + } + } } fn registry() -> &'static Mutex>> { @@ -55,6 +100,7 @@ pub fn register(endpoint_handle: u64) -> std::sync::Arc { conn_rx: tokio::sync::Mutex::new(conn_rx), shutdown_tx, shutdown_rx, + request_ready_waker: Mutex::new(None), }); registry() .lock() @@ -78,32 +124,34 @@ pub fn get(endpoint_handle: u64) -> Option> { /// blocked `recv()` in `nextRequest`, and any future callers will also observe /// the shutdown state immediately (watch persists its last value). pub fn remove(endpoint_handle: u64) { - if let Some(queue) = registry() + let queue = registry() .lock() .unwrap_or_else(|e| e.into_inner()) - .remove(&endpoint_handle) - { - // Trigger shutdown — this unblocks all pending nextRequest recv() calls. + .remove(&endpoint_handle); + if let Some(queue) = queue { + // Trigger shutdown and wake the event-driven synchronous drain loop. let _ = queue.shutdown_tx.send(true); + queue.wake_request_loop(); } } /// Signal shutdown without removing the queue from the registry. /// -/// This allows the JS polling loop to observe shutdown immediately via the -/// watch channel, while the caller can still drain queued items before the -/// queue is removed. +/// This allows the JS request loop to observe shutdown immediately via the +/// watch channel and wake callback, while the caller can still drain queued +/// items before the queue is removed. pub fn signal_shutdown(endpoint_handle: u64) { if let Some(queue) = get(endpoint_handle) { let _ = queue.shutdown_tx.send(true); + queue.wake_request_loop(); } } /// Signal shutdown to *every* registered serve queue and drain the registry. /// /// Called from `iroh_http_close_all` so that a SIGINT path which bypasses -/// `closeEndpoint` still wakes JS polling loops; otherwise `nextRequest` -/// would block forever and the Deno process would never exit (issue #155). +/// `closeEndpoint` still wakes JS request loops; otherwise the Deno process +/// would never exit (issue #155). pub fn shutdown_all() { let drained: Vec> = { let mut map = registry().lock().unwrap_or_else(|e| e.into_inner()); @@ -111,5 +159,6 @@ pub fn shutdown_all() { }; for queue in drained { let _ = queue.shutdown_tx.send(true); + queue.wake_request_loop(); } } diff --git a/packages/iroh-http-deno/test/adapter.test.ts b/packages/iroh-http-deno/test/adapter.test.ts index cdb92ef4..a963a68c 100644 --- a/packages/iroh-http-deno/test/adapter.test.ts +++ b/packages/iroh-http-deno/test/adapter.test.ts @@ -20,7 +20,10 @@ import { } from "jsr:@std/assert@^1"; import { createNode, PublicKey } from "../mod.ts"; import { generateSecretKey, publicKeyVerify, secretKeySign } from "../mod.ts"; -import { bigintToSafeNumber } from "../src/adapter.ts"; +import { + _observeServePollsForTesting, + bigintToSafeNumber, +} from "../src/adapter.ts"; async function waitFor( predicate: () => Promise, @@ -141,8 +144,8 @@ Deno.test("publicKeyVerify — valid signature passes", async () => { // Regression #115: serve loop must not hold pending ops after shutdown. // This test uses sanitizeOps: true (the Deno default) intentionally — -// if stopServe() doesn't drain the pending nextRequest() call, Deno's -// sanitizeOps check will fail. +// if stopServe() doesn't settle the request-ready wake loop, Deno's sanitizeOps +// check will fail. Deno.test({ name: "serve — no pending ops remain after signal abort (regression #115)", sanitizeOps: true, @@ -160,6 +163,44 @@ Deno.test({ await handle.finished; }); +// Regression: #397 — an idle server must sleep until Rust reports work. +// +// The MessageChannel yield previously caused the synchronous queue poller to +// run continuously even when no request was queued, saturating one CPU core. +Deno.test({ + name: "serve — idle queue does not continuously poll (regression #397)", + sanitizeOps: true, +}, async () => { + let polls = 0; + _observeServePollsForTesting(() => polls++); + + const server = await createNode({ disableNetworking: true }); + const ac = new AbortController(); + const handle = server.serve( + { signal: ac.signal }, + () => new Response("ok"), + ); + + try { + await waitFor( + () => Promise.resolve(polls > 0), + "serve loop did not inspect its request queue", + ); + const idleBaseline = polls; + await new Promise((resolve) => setTimeout(resolve, 50)); + assertEquals( + polls, + idleBaseline, + "idle serve loop polled without a request-ready notification", + ); + } finally { + _observeServePollsForTesting(undefined); + ac.abort(); + await handle.finished; + await server.close(); + } +}); + Deno.test({ name: "pathChanges — abort releases native subscriptions (regression #279)", sanitizeOps: false, @@ -328,19 +369,19 @@ Deno.test({ // Regression #155: iroh_http_close_all (the SIGINT/SIGTERM handler) used to // only drain the endpoint registry, leaving serve queues live and the JS -// polling loop spinning forever. The Deno process would never exit on CTRL+C +// request loop running forever. The Deno process would never exit on CTRL+C // while .serve() was active. close_all must now also wake all serve queues. import { _closeAllForTesting } from "../src/adapter.ts"; Deno.test({ - name: "serve — close_all wakes pending polling loop (regression #155)", + name: "serve — close_all wakes pending request loop (regression #155)", sanitizeOps: false, sanitizeResources: false, }, async () => { const server = await createNode({ bindAddr: "127.0.0.1:0" }); const handle = server.serve((_req: Request) => new Response("ok")); - // Simulate the SIGINT path. Without the fix this never wakes the polling + // Simulate the SIGINT path. Without the fix this never wakes the request // loop, the test times out, and Deno reports the leaked async op. _closeAllForTesting(); diff --git a/packages/iroh-http-shared/src/serve.ts b/packages/iroh-http-shared/src/serve.ts index db2f73e0..7d71b563 100644 --- a/packages/iroh-http-shared/src/serve.ts +++ b/packages/iroh-http-shared/src/serve.ts @@ -201,7 +201,7 @@ export function makeServe( onPeerEvent?: (event: PeerConnectionEvent) => void, maxChunkSizeBytes?: number, ): ServeFn { - // #114: guard against starting two polling loops on the same endpoint. + // #114: guard against starting two request loops on the same endpoint. let serveRunning = false; return ((...args: unknown[]): ServeHandle => { @@ -256,8 +256,8 @@ export function makeServe( decompress: options.decompress, }; - // rawServe returns a Promise that resolves when its internal polling - // loop exits (i.e. after stopServe() causes nextRequest to drain to null). + // rawServe resolves after its internal request loop observes native + // shutdown and drains all handler tasks. const loopDone = adapter.rawServe( endpointHandle, { onConnectionEvent, serveOptions: ffiServeOpts }, @@ -383,19 +383,19 @@ export function makeServe( // ISS-029 / #59 / #115: finished resolves when the serve loop actually terminates. // `loopDone` is the real loop-lifetime promise returned by rawServe(): - // - Deno: resolves when the nextRequest polling loop exits (null sentinel). + // - Deno: resolves when its callback-woken synchronous drain loop exits. // - Node / Tauri: resolves when waitServeStop() confirms the Rust task drained. // - // #115: finished must NOT resolve via onNodeClose alone. When the node closes, - // close_endpoint() calls serve_registry::remove() which sends the shutdown signal - // to the pending nextRequest Tokio task — but that task is scheduled, not yet run. - // If finished resolved immediately on onNodeClose, the nextRequest FFI op would - // still be in-flight (Deno sanitizeOps / process exit timing bug). + // #115: finished must NOT resolve via onNodeClose alone. When the node + // closes, close_endpoint() removes the serve queue and wakes the Deno + // request loop, but that loop still needs an event-loop turn to settle. If + // finished resolved immediately, Deno could cross an op-sanitizer or process + // exit boundary before the loop completed. // // Fix: when onNodeClose fires, chain it on loopDone. loopDone is guaranteed to - // resolve because close_endpoint always calls serve_registry::remove first, which - // unblocks the pending nextRequest. If loopDone resolves first (normal path when - // stopServe was called explicitly), the race wins immediately. + // resolve because close_endpoint always removes and wakes the native serve + // queue first. If loopDone resolves first (normal explicit stop), the race + // wins immediately. // #119: drain all in-flight body pipes before resolving so callers of // `await finished` see a true "all work done" guarantee. const finished: Promise = Promise.race([