diff --git a/changelog.d/8044-next-request-import-rooting.md b/changelog.d/8044-next-request-import-rooting.md new file mode 100644 index 0000000000..913010185e --- /dev/null +++ b/changelog.d/8044-next-request-import-rooting.md @@ -0,0 +1,6 @@ +### Fixed + +- Preserve request objects and their nested URL, search-parameter, header, and + body state when generated route modules re-export imported handlers. Imported + handler closures and complete expired-timer batches now remain rooted across + allocation, promise/timer continuations, microtask checkpoints, and moving GC. diff --git a/crates/perry-codegen/src/lower_call/extern_func.rs b/crates/perry-codegen/src/lower_call/extern_func.rs index 2ba4d10487..456d76a051 100644 --- a/crates/perry-codegen/src/lower_call/extern_func.rs +++ b/crates/perry-codegen/src/lower_call/extern_func.rs @@ -1688,26 +1688,48 @@ pub fn try_lower_extern_func_call( // an arrow-bound exported value (hono's `mergePath` from utils/url.js, // any `export const foo = () => …` cross-module use). if ctx.imported_vars.contains(name) { - ctx.pending_declares.push((fname.clone(), DOUBLE, vec![])); - let closure_box = ctx.block().call(DOUBLE, &fname, &[]); - let mut lowered_args: Vec = Vec::with_capacity(args.len()); - for a in args { - lowered_args.push(lower_expr(ctx, a)?); - } - if lowered_args.len() > 16 { + if args.len() > 16 { anyhow::bail!( "perry-codegen Phase D.1: closure call with {} args (max 16)", - lowered_args.len() + args.len() ); } - let blk = ctx.block(); - let closure_handle = unbox_to_i64(blk, &closure_box); - let runtime_fn = format!("js_closure_call{}", lowered_args.len()); - let mut call_args: Vec<(crate::types::LlvmType, &str)> = vec![(I64, &closure_handle)]; - for v in &lowered_args { - call_args.push((DOUBLE, v.as_str())); - } - return Ok(Some(blk.call(DOUBLE, &runtime_fn, &call_args))); + ctx.pending_declares.push((fname.clone(), DOUBLE, vec![])); + // Fetch the callee before evaluating arguments, as JavaScript requires, + // but keep that closure rooted while argument expressions run. Next's + // App Route exports are commonly `export const GET = ...`; constructing + // a request argument can allocate and evacuate the closure before this + // path finally dispatches it (#8036). + let closure_box = ctx.block().call(DOUBLE, &fname, &[]); + let arg_refs: Vec<&Expr> = args.iter().collect(); + let lowered_args = std::cell::RefCell::new(Vec::::new()); + let result = crate::rooting::with_rooted_accumulator( + ctx, + crate::rooting::Repr::Boxed, + &closure_box, + crate::rooting::any_operand_may_collect(ctx, args.iter()), + |ctx, _closure| { + crate::rooting::with_operands_rooted(ctx, &arg_refs, |_ctx, vals| { + *lowered_args.borrow_mut() = vals.to_vec(); + Ok(()) + }) + }, + |ctx, closure_box| { + // Re-read and unbox only after every collecting argument and + // after the argument group's own re-reads have completed. + let lowered = lowered_args.borrow(); + let blk = ctx.block(); + let closure_handle = unbox_to_i64(blk, closure_box); + let runtime_fn = format!("js_closure_call{}", lowered.len()); + let mut call_args: Vec<(crate::types::LlvmType, &str)> = + vec![(I64, &closure_handle)]; + for value in lowered.iter() { + call_args.push((DOUBLE, value.as_str())); + } + Ok(blk.call(DOUBLE, &runtime_fn, &call_args)) + }, + )?; + return Ok(Some(result)); } // Record the cross-module call so the caller can add a `declare` // line for it after the &mut LlFunction borrow is released. The diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots.rs b/crates/perry-runtime/src/gc/tests/runtime_roots.rs index 2ad1858d53..36d46f77f5 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots.rs @@ -485,17 +485,24 @@ extern "C" fn test_async_hook_event_force_minor_gc( thread_local! { static TEST_TIMER_ARG_BITS: Cell = const { Cell::new(0) }; static TEST_TIMER_CALLED: Cell = const { Cell::new(false) }; + static TEST_TIMER_CALLBACK_PTR: Cell = const { Cell::new(0) }; } extern "C" fn test_timer_capture_arg( - _closure: *const crate::closure::ClosureHeader, + closure: *const crate::closure::ClosureHeader, arg: f64, ) -> f64 { + TEST_TIMER_CALLBACK_PTR.with(|slot| slot.set(closure as usize)); TEST_TIMER_ARG_BITS.with(|slot| slot.set(arg.to_bits())); TEST_TIMER_CALLED.with(|slot| slot.set(true)); f64::from_bits(crate::value::TAG_UNDEFINED) } +extern "C" fn test_timer_force_minor_gc(_closure: *const crate::closure::ClosureHeader) -> f64 { + let _ = crate::gc::gc_collect_minor(); + f64::from_bits(crate::value::TAG_UNDEFINED) +} + extern "C" fn test_rest_first_value( _closure: *const crate::closure::ClosureHeader, rest: f64, diff --git a/crates/perry-runtime/src/gc/tests/runtime_roots/hook_dispatch_handles.rs b/crates/perry-runtime/src/gc/tests/runtime_roots/hook_dispatch_handles.rs index e95cc709be..7f1c40e2c8 100644 --- a/crates/perry-runtime/src/gc/tests/runtime_roots/hook_dispatch_handles.rs +++ b/crates/perry-runtime/src/gc/tests/runtime_roots/hook_dispatch_handles.rs @@ -133,6 +133,53 @@ fn test_timer_tick_roots_callback_args_and_previous_context_across_hooks() { crate::timer::clearTimeout(timer_id); } +#[test] +fn test_timer_tick_roots_the_complete_detached_expired_batch() { + let _async_hook_guard = AsyncHookRuntimeTestGuard::new(); + let _guard = CopyingNurseryTestGuard::new(0); + let _trigger_guard = GcTriggerThresholdTestGuard::suppress_automatic_triggers(); + register_runtime_handle_root_scanner_for_tests(); + gc_register_mutable_root_scanner(crate::timer::scan_timer_roots_mut); + + let collecting_callback = + crate::closure::js_closure_alloc(test_timer_force_minor_gc as *const u8, 0); + let second_callback = crate::closure::js_closure_alloc(test_timer_capture_arg as *const u8, 0); + let second_callback_original = second_callback as usize; + let second_arg = test_string_value(b"detached-timer-batch"); + let second_arg_original = (second_arg.to_bits() & POINTER_MASK) as usize; + let second_args = [second_arg]; + + let first_id = crate::timer::js_set_timeout_callback(collecting_callback as i64, 0.0); + let second_id = unsafe { + crate::timer::js_set_timeout_callback_args( + second_callback as i64, + 0.0, + second_args.as_ptr(), + 1, + ) + }; + TEST_TIMER_ARG_BITS.with(|slot| slot.set(0)); + TEST_TIMER_CALLED.with(|slot| slot.set(false)); + TEST_TIMER_CALLBACK_PTR.with(|slot| slot.set(0)); + + assert_eq!(crate::timer::js_callback_timer_tick(), 2); + assert!(TEST_TIMER_CALLED.with(|slot| slot.get())); + let dispatched_callback = TEST_TIMER_CALLBACK_PTR.with(|slot| slot.get()); + assert_ne!( + dispatched_callback, second_callback_original, + "detached timer callback should be refreshed after copied-minor GC" + ); + assert!(crate::closure::is_closure_ptr(dispatched_callback)); + assert_moved_string_value( + f64::from_bits(TEST_TIMER_ARG_BITS.with(|slot| slot.get())), + second_arg_original, + b"detached-timer-batch", + ); + + crate::timer::clearTimeout(first_id); + crate::timer::clearTimeout(second_id); +} + #[test] fn test_next_tick_previous_context_survives_hook_gc() { const ALS_HANDLE: i64 = -8_502; diff --git a/crates/perry-runtime/src/timer.rs b/crates/perry-runtime/src/timer.rs index 4999fd5f5a..b58601a5fd 100644 --- a/crates/perry-runtime/src/timer.rs +++ b/crates/perry-runtime/src/timer.rs @@ -1116,24 +1116,55 @@ pub extern "C" fn js_callback_timer_tick() -> i32 { // #6287: timers phase (by deadline) before check phase (FIFO immediates). order_expired_callback_batch(&mut expired); + // #8036: draining removes the WHOLE expired batch from CALLBACK_TIMERS + // before the first callback runs. A callback can run arbitrary JS and the + // microtask checkpoint below can collect, so rooting only the timer being + // dispatched leaves every later callback, argument, and captured async + // context sitting in an ordinary Rust Vec the collector cannot see. With + // concurrent request timers, the first resolve callback's checkpoint moved + // the next resolve closure and the second timer called its from-space + // address (`TypeError: value is not a function`). Protect the complete + // detached batch for the complete dispatch loop. + let batch_scope = crate::gc::RuntimeHandleScope::new(); + let callback_handles: Vec<_> = expired + .iter() + .map(|timer| { + batch_scope.root_raw_const_ptr(timer.callback as *const crate::closure::ClosureHeader) + }) + .collect(); + let arg_handles: Vec<_> = expired + .iter() + .map(|timer| batch_scope.root_nanbox_f64_slice(&timer.args)) + .collect(); + let context_roots: Vec<_> = expired + .iter() + .map(|timer| crate::async_context::root_snapshot(&batch_scope, &timer.context)) + .collect(); + let mut fired = 0; // Call the callbacks, forwarding any trailing args captured at // `setTimeout(fn, delay, ...args)` time. Refs #665. - for timer in expired { + for (index, mut timer) in expired.into_iter().enumerate() { if !timer.cleared { - let scope = crate::gc::RuntimeHandleScope::new(); - let cb_handle = - scope.root_raw_const_ptr(timer.callback as *const crate::closure::ClosureHeader); - let arg_handles = scope.root_nanbox_f64_slice(&timer.args); + crate::async_context::refresh_snapshot_from_roots( + &mut timer.context, + &context_roots[index], + ); let previous = crate::async_context::enter_context(&timer.context); let mut previous = previous; - let previous_roots = crate::async_context::root_snapshot(&scope, &previous); + let previous_roots = crate::async_context::root_snapshot(&batch_scope, &previous); crate::async_hooks::before(timer.async_id, timer.trigger_async_id); - let a = crate::gc::RuntimeHandleScope::refreshed_nanbox_f64_slice(&arg_handles); - let cb = cb_handle.get_raw_const_ptr::(); let prev_this = crate::object::js_implicit_this_set(timer_handle_value(timer.id)); enter_timer_callback_dispatch(); with_timer_uncaught_trap(|| { + // Installing the timer receiver above is itself a collecting + // boundary. Re-read both roots inside the trap, immediately + // before dispatch, so this callback cannot be evacuated in + // between the handle read and js_closure_callN. + let a = + crate::gc::RuntimeHandleScope::refreshed_nanbox_f64_slice(&arg_handles[index]); + let cb = + callback_handles[index].get_raw_const_ptr::(); match a.len() { 0 => { js_closure_call0(cb); diff --git a/crates/perry/src/commands/compile/run_pipeline.rs b/crates/perry/src/commands/compile/run_pipeline.rs index 2d842e6a66..42af9a2948 100644 --- a/crates/perry/src/commands/compile/run_pipeline.rs +++ b/crates/perry/src/commands/compile/run_pipeline.rs @@ -4313,6 +4313,22 @@ pub fn run_with_parse_cache( } if references_interface { for (src_pathbuf, src_hir) in &ctx.native_modules { + // #8036: this augmentation supplies FOREIGN implementors + // to a consumer's polymorphic dispatch tower. Feeding the + // module its own exported classes back through + // `imported_classes` creates a second, imported-constructor + // identity for each local class. `lower_new` then treats a + // local class as cross-module and calls its standalone + // constructor metadata instead of the local implicit-super + // path. That is observably wrong for native-backed derived + // classes such as Next's ReadonlyURLSearchParams: the local + // path installs the URLSearchParams backing, while the + // accidental self-import path loses it. Local classes are + // already present in codegen's class table, so they must + // never be added by this foreign-class fallback. + if src_pathbuf == path { + continue; + } let src_path = src_pathbuf.to_string_lossy().to_string(); for class in &src_hir.classes { if !class.is_exported { diff --git a/test-files/fixtures/gc_next_request_import/lazy_work.ts b/test-files/fixtures/gc_next_request_import/lazy_work.ts new file mode 100644 index 0000000000..c65009a6bb --- /dev/null +++ b/test-files/fixtures/gc_next_request_import/lazy_work.ts @@ -0,0 +1,7 @@ +export function checksum(iterations: number): number { + let value = 0x811c9dc5; + for (let index = 0; index < iterations; index += 1) { + value = Math.imul(value ^ index, 0x01000193) >>> 0; + } + return value; +} diff --git a/test-files/fixtures/gc_next_request_import/request.ts b/test-files/fixtures/gc_next_request_import/request.ts new file mode 100644 index 0000000000..23290d41d3 --- /dev/null +++ b/test-files/fixtures/gc_next_request_import/request.ts @@ -0,0 +1,37 @@ +export class ReadonlyFixtureSearchParams extends URLSearchParams { + set(_name: string, _value: string): void { + throw new TypeError("readonly search params"); + } +} + +export class FixtureNextUrl { + readonly pathname: string; + readonly searchParams: ReadonlyFixtureSearchParams; + + constructor(url: string) { + const parsed = new URL(url); + this.pathname = parsed.pathname; + this.searchParams = new ReadonlyFixtureSearchParams(parsed.search); + } +} + +export class FixtureNextRequest { + readonly method: string; + readonly url: string; + readonly nextUrl: FixtureNextUrl; + readonly headers: Headers; + private readonly requestBody: string; + + constructor(method: string, url: string, id: string, requestBody = "") { + this.method = method; + this.url = url; + this.nextUrl = new FixtureNextUrl(url); + this.headers = new Headers({ "x-request-id": id }); + this.requestBody = requestBody; + } + + async text(): Promise { + await Promise.resolve(); + return this.requestBody; + } +} diff --git a/test-files/fixtures/gc_next_request_import/route.ts b/test-files/fixtures/gc_next_request_import/route.ts new file mode 100644 index 0000000000..d5143d0ea7 --- /dev/null +++ b/test-files/fixtures/gc_next_request_import/route.ts @@ -0,0 +1 @@ +export { GET, POST, syncSummary } from "./route_impl.ts"; diff --git a/test-files/fixtures/gc_next_request_import/route_impl.ts b/test-files/fixtures/gc_next_request_import/route_impl.ts new file mode 100644 index 0000000000..59b2ae0d6a --- /dev/null +++ b/test-files/fixtures/gc_next_request_import/route_impl.ts @@ -0,0 +1,60 @@ +import type { FixtureNextRequest } from "./request.ts"; + +function churn(seed: number): number { + const allocations: Array<{ index: number; label: string; pad: number[] }> = []; + for (let index = 0; index < 300; index += 1) { + allocations.push({ + index, + label: "request-churn-" + seed + "-" + index, + pad: [seed, index, seed + index], + }); + } + return allocations.length; +} + +function snapshot(request: FixtureNextRequest): string { + return [ + request.method, + request.url, + request.nextUrl.pathname, + request.nextUrl.searchParams.get("id"), + request.nextUrl.searchParams.get("iterations"), + request.headers.get("x-request-id"), + ].join("|"); +} + +async function handle(request: FixtureNextRequest): Promise> { + const beforeAwait = snapshot(request); + const { checksum } = await import("./lazy_work.ts"); + const iterations = Number(request.nextUrl.searchParams.get("iterations")); + const checksumValue = checksum(iterations); + const allocationCount = churn(iterations); + await new Promise((resolve) => setTimeout(resolve, 1)); + const afterAwait = snapshot(request); + const requestBody = request.method === "POST" ? await request.text() : ""; + const method = request.method; + const pathname = request.nextUrl.pathname; + const header = request.headers.get("x-request-id"); + const id = request.nextUrl.searchParams.get("id"); + + return { + beforeAwait, + afterAwait, + method, + pathname, + id, + header, + iterations, + checksum: checksumValue, + allocationCount, + requestBody, + }; +} + +export function syncSummary(request: FixtureNextRequest): string { + churn(17); + return snapshot(request); +} + +export const GET = handle; +export const POST = handle; diff --git a/test-files/test_gap_gc_next_request_import.ts b/test-files/test_gap_gc_next_request_import.ts new file mode 100644 index 0000000000..bc2e1435e3 --- /dev/null +++ b/test-files/test_gap_gc_next_request_import.ts @@ -0,0 +1,48 @@ +// #8036: preserve a production-shaped request object when a generated route +// module re-exports its imported user handler. The request and all of its +// nested state must remain live while the callee allocates, dynamically +// imports a module, resumes a promise, resumes a timer, and reads the body. +// The GC moving-witness matrix registers this test below and runs it with +// forced/verified evacuation so a stale-but-unreused pointer cannot pass. + +import { GET, POST, syncSummary } from "./fixtures/gc_next_request_import/route.ts"; +import { FixtureNextRequest } from "./fixtures/gc_next_request_import/request.ts"; + +function makeRequest(id: string, iterations: number, method = "GET", body = "") { + return new FixtureNextRequest( + method, + "https://perry.invalid/api/benchmark?id=" + id + "&iterations=" + iterations, + id, + body, + ); +} + +const syncRequest = makeRequest("sync-request", 17); +console.log("sync", syncSummary(syncRequest)); + +// Mirror the production verifier's 20 concurrent GETs without making this +// request-boundary regression depend on Promise.all's separate array-forwarding +// contract. Starting every handler before awaiting the first result also makes +// cross-request ID/header swaps observable. +const pending: Array>> = []; +for (let index = 0; index < 20; index += 1) { + pending.push(GET(makeRequest("request-" + index, index + 1))); +} + +// Perry exposes an explicit collector hook; Node does not unless launched with +// --expose-gc. Under the matrix's force/verify arm this evacuates the request +// graphs while every handler is suspended and all 20 timers are still queued. +const collect = (globalThis as unknown as { gc?: () => void }).gc; +if (collect) { + collect(); +} + +for (const result of pending) { + console.log(JSON.stringify(await result)); +} + +console.log( + JSON.stringify( + await POST(makeRequest("post-request", 31, "POST", "perry-request-body")), + ), +); diff --git a/test-parity/gc_repsel_corpus.txt b/test-parity/gc_repsel_corpus.txt index d011fdc7b7..21aee8b35b 100644 --- a/test-parity/gc_repsel_corpus.txt +++ b/test-parity/gc_repsel_corpus.txt @@ -311,6 +311,11 @@ test_gap_gc_closure_call_argument_rooting # mistake. Registering the file is the whole fix. test_gap_gc_call_argument_rooting +# #8036: a NextRequest-shaped class crosses a generated-route-style re-export +# into an imported sync/async handler and stays live through allocation, +# dynamic import, promise/timer continuations, and body reads. +test_gap_gc_next_request_import + # --- #7270's two witnesses, registered here rather than dark ----------------- # `rest_argument_rooting` and `same_module_call_argument_rooting` shipped with # #7270. Registering them at merge rather than after: they are the FOURTH