From 55fa08e74354e880fce8ceec210fcb7974b4f240 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 09:25:04 +0000 Subject: [PATCH] fix(child_process): throw an unhandled spawn/fork `error` event, as Node does A failed spawn()/fork() emits a deferred `error` on the ChildProcess. With no `error` listener Perry dropped it, still fired `close`, and carried on; Node's EventEmitter throws it (`Unhandled 'error' event`) and `close` never fires. This is the remaining half of #10730: test_gap_9592 read as a Perry pass on macOS because the missing /bin/true's ENOENT went nowhere, not because Perry resolved the absolute path through PATH (it does not). The emit now reports whether a listener took the error; if not, the error object is thrown as-is once the frame's handle scopes have dropped, and `close` is not scheduled. Adds test_gap_10730_child_unhandled_error (byte-identical to node; prints `close without listener` and no `uncaught` lines without the fix) and two failed_spawn unit tests. --- changelog.d/10730-child-unhandled-error.md | 11 +++ .../src/child_process/failed_spawn.rs | 96 +++++++++++++++++-- .../src/child_process/reactor.rs | 9 +- .../test_gap_10730_child_unhandled_error.ts | 45 +++++++++ 4 files changed, 148 insertions(+), 13 deletions(-) create mode 100644 changelog.d/10730-child-unhandled-error.md create mode 100644 test-files/test_gap_10730_child_unhandled_error.ts diff --git a/changelog.d/10730-child-unhandled-error.md b/changelog.d/10730-child-unhandled-error.md new file mode 100644 index 0000000000..d419aa0ec2 --- /dev/null +++ b/changelog.d/10730-child-unhandled-error.md @@ -0,0 +1,11 @@ +**child_process: an `error` event with no listener now throws, as in Node (#10730).** When `spawn()` or `fork()` fails (for example `ENOENT` for a missing binary), Perry emits a deferred `error` event on the ChildProcess. If no `error` listener was registered, Perry used to drop the error, fire `close`, and let the program continue. Node's EventEmitter throws an unhandled `error` event instead: the process dies with `Unhandled 'error' event` before `close` fires. + +That gap is why `test_gap_9592_child_timeout_threads` passed for Perry on macOS while Node crashed. The fixture spawned `/bin/true`, which macOS does not have, and never listened for `error`. #10730 guessed Perry was resolving the missing absolute path through `PATH`. It was not: Perry reported `ENOENT` correctly, and nothing was listening for it. + +The emit now reports whether any listener took the error (`failed_spawn::emit_spawn_error`). If none did, the error object is thrown as-is from the `setImmediate` callback that delivers it, after that frame's handle scopes have dropped, so `uncaughtException` and the uncaught-exit path see the same object Node rethrows. `close` is not scheduled in that case. A handled error behaves as before. No JS-callable native signature changed. + +Regression coverage: +- `test-files/test_gap_10730_child_unhandled_error.ts` checks three cases: a handled error, an unhandled one, and a listener that was removed. It is byte-identical to Node with the fix. Without the fix Perry prints `close without listener` and no `uncaught` lines. +- `child_process::failed_spawn::tests::spawn_error_{without,with}_listener_is_{unhandled,handled}` are the unit-level half. + +`perry-runtime --lib` single-threaded: main 4704 passed, 0 failed. This change: 4706 passed, 0 failed (the two new tests). diff --git a/crates/perry-runtime/src/child_process/failed_spawn.rs b/crates/perry-runtime/src/child_process/failed_spawn.rs index d15ec5e5c7..2b7c16cc4c 100644 --- a/crates/perry-runtime/src/child_process/failed_spawn.rs +++ b/crates/perry-runtime/src/child_process/failed_spawn.rs @@ -6,15 +6,54 @@ use super::*; /// time can already be overdue before the error's setImmediate callback runs. /// Keep fork's separate failure contract unchanged by using this for spawn only. pub(super) extern "C" fn emit_error_then_close(closure: *const ClosureHeader) -> f64 { - let scope = crate::gc::RuntimeHandleScope::new(); - let cp = scope.root_nanbox_f64(cp_this(closure)); - reactor::cp_emit_spawn_error(closure); - let close = crate::closure::js_closure_alloc(reactor::cp_emit_spawn_close as *const u8, 1); - crate::closure::js_closure_set_capture_ptr(close, 0, cp.get_nanbox_f64().to_bits() as i64); - crate::timer::js_set_timeout_callback(close as i64, 1.0); + let unhandled = { + let scope = crate::gc::RuntimeHandleScope::new(); + let cp = scope.root_nanbox_f64(cp_this(closure)); + let unhandled = emit_spawn_error(cp.get_nanbox_f64()); + // An unhandled error ends the process in Node before `close` fires. + if unhandled.is_none() { + let close = + crate::closure::js_closure_alloc(reactor::cp_emit_spawn_close as *const u8, 1); + crate::closure::js_closure_set_capture_ptr( + close, + 0, + cp.get_nanbox_f64().to_bits() as i64, + ); + crate::timer::js_set_timeout_callback(close as i64, 1.0); + } + unhandled + }; + throw_if_unhandled(unhandled); cp_undefined() } +/// Emit the deferred spawn/fork failure `error` on the ChildProcess `cp`. +/// +/// Returns the error when no `error` listener took it (#10730). Node's +/// EventEmitter throws an `error` event nobody listens for, so a failed +/// `spawn("/no/such/file")` without a handler dies with `Unhandled 'error' +/// event`. Perry used to drop it and carry on, which made a missing binary +/// look like a child that ran and exited. +pub(super) fn emit_spawn_error(cp: f64) -> Option { + let scope = crate::gc::RuntimeHandleScope::new(); + let cp = scope.root_nanbox_f64(cp); + let err = scope.root_nanbox_f64(cp_get_field(cp.get_nanbox_f64(), b"__cpError")); + if JSValue::from_bits(err.get_nanbox_f64().to_bits()).is_undefined() { + return None; + } + let handled = cp_emit(cp.get_nanbox_f64(), "error", &[err.get_nanbox_f64()]); + cp_set_field(cp.get_nanbox_f64(), b"signalCode", TAG_NULL_F64); + (!handled).then(|| err.get_nanbox_f64()) +} + +/// Throw `unhandled` as an uncaught exception. Called only once every handle +/// scope in the emitting frame has dropped, so the throw skips no cleanup. +pub(super) fn throw_if_unhandled(unhandled: Option) { + if let Some(err) = unhandled { + crate::exception::js_throw(err); + } +} + pub(super) fn finish_outputs(cp: f64) { let scope = crate::gc::RuntimeHandleScope::new(); let cp = scope.root_nanbox_f64(cp); @@ -72,6 +111,51 @@ mod tests { } } + extern "C" fn swallow_error(_closure: *const ClosureHeader, _err: f64) -> f64 { + cp_undefined() + } + + /// A failed-spawn ChildProcess stand-in: just the fields the emit reads. + fn failed_child<'s>(scope: &'s crate::gc::RuntimeHandleScope) -> crate::gc::RuntimeHandle<'s> { + let cp = scope.root_nanbox_f64(cp_box_ptr(crate::object::js_object_alloc(0, 4).cast())); + let err = cp_make_error( + "spawn /missing ENOENT", + &[("code", cp_box_string("ENOENT"))], + ); + cp_set_field(cp.get_nanbox_f64(), b"__cpError", err); + cp + } + + /// #10730: with no `error` listener the error comes back to be thrown. + /// Before the fix the emit reported nothing, so the error was dropped. + #[test] + fn spawn_error_without_listener_is_unhandled() { + let scope = crate::gc::RuntimeHandleScope::new(); + let cp = failed_child(&scope); + let err = cp_get_field(cp.get_nanbox_f64(), b"__cpError"); + let unhandled = emit_spawn_error(cp.get_nanbox_f64()) + .expect("an error event nobody listens for must be thrown"); + assert_eq!( + unhandled.to_bits(), + err.to_bits(), + "Node rethrows the same object" + ); + } + + #[test] + fn spawn_error_with_listener_is_handled() { + crate::closure::js_register_closure_arity(swallow_error as *const u8, 1); + let scope = crate::gc::RuntimeHandleScope::new(); + let cp = failed_child(&scope); + let listener = crate::closure::js_closure_alloc(swallow_error as *const u8, 0); + super::super::emitter::cp_register( + cp.get_nanbox_f64(), + cp_box_string("error"), + cp_box_ptr(listener.cast()), + ); + assert!(emit_spawn_error(cp.get_nanbox_f64()).is_none()); + } + #[test] fn absent_failed_output_is_ignored() { finish_output(TAG_NULL_F64); diff --git a/crates/perry-runtime/src/child_process/reactor.rs b/crates/perry-runtime/src/child_process/reactor.rs index 2ec9071b8a..8d154c1a2d 100644 --- a/crates/perry-runtime/src/child_process/reactor.rs +++ b/crates/perry-runtime/src/child_process/reactor.rs @@ -1033,13 +1033,8 @@ pub extern "C" fn js_child_process_spawn_streams( /// Deferred single-`error` emit for the spawn/fork failure path. Slot 0 /// captures the ChildProcess value. pub(super) extern "C" fn cp_emit_spawn_error(closure: *const ClosureHeader) -> f64 { - let scope = crate::gc::RuntimeHandleScope::new(); - let cp = scope.root_nanbox_f64(cp_this(closure)); - let err = scope.root_nanbox_f64(cp_get_field(cp.get_nanbox_f64(), b"__cpError")); - if !JSValue::from_bits(err.get_nanbox_f64().to_bits()).is_undefined() { - cp_emit(cp.get_nanbox_f64(), "error", &[err.get_nanbox_f64()]); - cp_set_field(cp.get_nanbox_f64(), b"signalCode", TAG_NULL_F64); - } + let unhandled = super::failed_spawn::emit_spawn_error(cp_this(closure)); + super::failed_spawn::throw_if_unhandled(unhandled); cp_undefined() } diff --git a/test-files/test_gap_10730_child_unhandled_error.ts b/test-files/test_gap_10730_child_unhandled_error.ts new file mode 100644 index 0000000000..20e78642fa --- /dev/null +++ b/test-files/test_gap_10730_child_unhandled_error.ts @@ -0,0 +1,45 @@ +// #10730 — a ChildProcess `error` event with no listener must throw, as every +// Node EventEmitter does. Perry dropped it: a failed spawn of a missing binary +// looked like a child that ran, `close` fired, and the program carried on. That +// is how test_gap_9592's missing `/bin/true` on macOS read as a Perry pass +// while Node died with `Unhandled 'error' event`. +// +// The throw is observed through `uncaughtException` so the result is printed +// rather than inferred from an exit status. +import { spawn } from "node:child_process"; + +// Absolute, and absent on every platform the suite runs on. +const MISSING = "/nonexistent-perry-10730/true"; + +const events: string[] = []; +process.on("uncaughtException", (err: any) => { + events.push(`uncaught ${err.code} ${err.syscall} ${err instanceof Error}`); +}); + +// 1. With a listener: the error is delivered, nothing is thrown, `close` fires. +await new Promise((resolve) => { + const child = spawn(MISSING, [], { stdio: "ignore" }); + child.on("error", (err: any) => events.push(`handled ${err.code} ${err.path}`)); + child.on("close", (code: any) => { + events.push(`close ${code}`); + resolve(); + }); +}); + +// 2. Without one: the error is thrown, and `close` never fires. +{ + const child = spawn(MISSING, ["a"], { stdio: "ignore" }); + child.on("close", () => events.push("close without listener")); +} + +// 3. A listener that was removed again is no listener. +{ + const child = spawn(MISSING, ["b"], { stdio: "ignore" }); + const onError = () => events.push("removed listener ran"); + child.on("error", onError); + child.removeListener("error", onError); +} + +setTimeout(() => { + for (const line of events) console.log(line); +}, 200);