diff --git a/CHANGELOG.md b/CHANGELOG.md index af246284cf7c..218e86960f14 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ ## Unreleased +- Count uncaught errors during isolated test-file cleanup against the outgoing file and include them in JUnit; handled cleanup callbacks continue running. Adapts the file-error reporting pattern from [oven-sh/bun#44435](https://github.com/oven-sh/bun/pull/44435); thanks @robobun! + - Drain queued Worker stdout and stderr before emitting `exit`, preserving output when a synchronous worker exit races the parent's stdio setup. - Avoid an extra tracing collection when destroying Workers while preserving native-resource finalizers and the mandatory final heap sweep. diff --git a/src/runtime/cli/test/parallel/runner.rs b/src/runtime/cli/test/parallel/runner.rs index d591d588b4e9..6e0435839307 100644 --- a/src/runtime/cli/test/parallel/runner.rs +++ b/src/runtime/cli/test/parallel/runner.rs @@ -578,6 +578,11 @@ impl<'a> WorkerLoop<'a> { let before_unhandled = self.reporter.jest.unhandled_errors_between_tests; let started_ns = bun_core::Timespec::now(bun_core::TimespecMockMode::ForceRealTime).ns(); + let isolation = if vm.test_isolation_enabled { + test_command::FileIsolation::SwapGlobal + } else { + test_command::FileIsolation::KeepGlobal + }; // A worker never knows which file is its last, so preload-level hooks wrap every file (with or without --isolate). if let Err(err) = TestCommand::run( @@ -588,17 +593,11 @@ impl<'a> WorkerLoop<'a> { first: true, last: true, }, + isolation, ) { test_command::handle_top_level_test_error_before_javascript_start(&err); } - if vm.test_isolation_enabled { - crate::jsc_hooks::stop_active_handles_for_test_isolation(vm); - vm.swap_global_for_test_isolation(); - self.reporter - .jest - .bun_test_root - .reset_hook_scope_for_test_isolation(); - } else { + if !vm.test_isolation_enabled { Global::mimalloc_cleanup(false); } self.reporter.jest.default_timeout_override = u32::MAX; diff --git a/src/runtime/cli/test_command.rs b/src/runtime/cli/test_command.rs index 05ccab10dd03..64fc2519f210 100644 --- a/src/runtime/cli/test_command.rs +++ b/src/runtime/cli/test_command.rs @@ -374,9 +374,9 @@ impl TestFailure { /// VirtualMachine::on_print_error_zig_exception thunk. pub(crate) fn record_cb(ctx: *mut core::ffi::c_void, exception: &jsc::ZigException) { - // SAFETY: `ctx` was set to `&mut CommandLineReporter.test_failure` by - // `on_uncaught_exception` for the duration of a single - // `run_error_handler` call; single-threaded, no other borrow live. + // SAFETY: `on_uncaught_exception` points `ctx` at an Option + // for one synchronous `run_error_handler` call, then clears the callback. + // The slot is either the reporter's test failure or a local file failure. let slot = unsafe { &mut *ctx.cast::>() }; TestFailure::record(slot, exception); } @@ -1261,6 +1261,30 @@ impl CommandLineReporter { &mut self.jest.summary } + /// The uncaught hook already counted this error. Attach it to its file + /// without turning the last completed test into a failure. + pub(crate) fn handle_unhandled_file_error( + &mut self, + file_id: jest::FileId, + failure: Option, + ) { + let report = TestCaseReport { + file: junit_file_name(self.jest.files.items_source()[file_id as usize].path.text), + scopes: Vec::new(), + name: b"(unhandled error)", + status: bun_test::Execution::Result::Fail, + assertions: 0, + elapsed_ns: 0, + line_number: 0, + failure, + }; + if let Some(idx) = self.worker_ipc_file_idx { + ParallelRunner::worker_emit_test_done(idx, b"", Some(&report)); + } else if let Some(junit) = self.reporters.junit.as_mut() { + junit.record_test_case(&report).expect("oom"); + } + } + pub(crate) fn handle_test_completed( buntest: &mut bun_test::BunTest, sequence: &mut bun_test::Execution::ExecutionSequence, @@ -1730,6 +1754,12 @@ extern "C" fn BunTest__shouldGenerateCodeCoverage(test_name_str: &bun_core::Stri true } +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum FileIsolation { + KeepGlobal, + SwapGlobal, +} + pub(crate) struct TestCommand; impl TestCommand { @@ -2759,6 +2789,11 @@ impl TestCommand { first: isolate || i == 0, last: isolate, }, + if isolate { + FileIsolation::SwapGlobal + } else { + FileIsolation::KeepGlobal + }, ) { handle_top_level_test_error_before_javascript_start(&err); } @@ -2767,14 +2802,6 @@ impl TestCommand { } reporter.jest.default_timeout_override = u32::MAX; Global::mimalloc_cleanup(false); - if isolate { - crate::jsc_hooks::stop_active_handles_for_test_isolation(vm); - vm.swap_global_for_test_isolation(); - reporter - .jest - .bun_test_root - .reset_hook_scope_for_test_isolation(); - } } } @@ -2788,6 +2815,7 @@ impl TestCommand { first: isolate || files.len() == 1, last: true, }, + FileIsolation::KeepGlobal, ) { handle_top_level_test_error_before_javascript_start(&err); } @@ -2819,6 +2847,7 @@ impl TestCommand { vm: &mut VirtualMachine, file_name: &[u8], first_last: bun_test::FirstLast, + isolation: FileIsolation, ) -> crate::Result<()> { // Capture the raw log pointer (Copy) so the guard does not borrow `vm`. let vm_log = vm.log; @@ -2873,6 +2902,13 @@ impl TestCommand { let repeat_count = reporter.repeat_count; let mut repeat_index: u32 = 0; + scopeguard::defer! { + // SAFETY: the caller-owned reporter outlives the run and every + // per-repeat cleanup guard below. Teardown errors are recorded first. + if let Some(junit) = unsafe { &mut *reporter_ptr }.reporters.junit.as_mut() { + let _ = junit.end_file(None); + } + } vm.on_unhandled_rejection_ctx = None; vm.on_unhandled_rejection = jest::on_unhandled_rejection::on_unhandled_rejection; @@ -2893,13 +2929,45 @@ impl TestCommand { bun_test_root.enter_file(file_id, reporter, should_run_concurrent, first_last); let bun_test_root_ptr: *mut bun_test::BunTestRoot = bun_test_root; let global = vm.global(); + let vm_ptr: *mut VirtualMachine = vm; + // An early return also finalizes the file; successful intermediate + // reruns only release their BunTest, retaining the current global. + let finalize_file = core::cell::Cell::new(true); scopeguard::defer! { + let swap = finalize_file.get() && isolation == FileIsolation::SwapGlobal; + if swap { + // SAFETY: the root and active file remain live through this guard. + // End collection/execution before callbacks can re-enter the scheduler. + if let Some(active) = unsafe { (*bun_test_root_ptr).clone_active_file() } { + active.get().begin_file_teardown(); + } + } + // A pending patch must be forgotten before its global is retired. + bun_jsc::cpp::JSMock__forgetPendingModulePatches(global); + if swap { + // SAFETY: the caller's VM remains live on this JS thread. + // Keep the active file and reporter through the existing close + // callbacks so uncaught teardown errors belong to this file. + let vm = unsafe { &mut *vm_ptr }; + crate::jsc_hooks::stop_active_handles_for_test_isolation(vm); + vm.swap_global_for_test_isolation(); + } // SAFETY: `bun_test_root` is `&'static mut` from `Jest::runner()`; // raw-ptr escape so the closure does not hold a borrowck lock on // it for the loop body. unsafe { (*bun_test_root_ptr).exit_file(); } - // A mock.module() patch still pending must not hold up the next file. - bun_jsc::cpp::JSMock__forgetPendingModulePatches(global); + if swap { + // SAFETY: exit_file and the loop's local Rc releases precede + // dropping the old preload scopes those BunTests referenced. + unsafe { (*bun_test_root_ptr).reset_hook_scope_for_test_isolation(); } + } + if Output::is_github_action() + // SAFETY: reporter is caller-owned and remains live through this guard. + && unsafe { (*reporter_ptr).worker_ipc_file_idx.is_none() } + { + pretty_errorln!("\n::endgroup::\n"); + Output::flush(); + } } // SAFETY: `set()` reads only `reporter.{worker_ipc_file_idx, reporters}` @@ -3043,11 +3111,6 @@ impl TestCommand { let _ = vm.global().handle_rejected_promises(); - if Output::is_github_action() && reporter.worker_ipc_file_idx.is_none() { - pretty_errorln!("\n::endgroup::\n"); - Output::flush(); - } - if !vm.test_isolation_enabled { // Ensure these never linger across files. Under --isolate this // is done by swapGlobalForTestIsolation() (kill+clear) and we @@ -3057,9 +3120,7 @@ impl TestCommand { } repeat_index += 1; - } - if let Some(junit) = reporter.reporters.junit.as_mut() { - let _ = junit.end_file(None); + finalize_file.set(repeat_index == repeat_count); } Ok(()) } diff --git a/src/runtime/test_runner/bun_test.rs b/src/runtime/test_runner/bun_test.rs index b67a04b69cfc..e81afbc53643 100644 --- a/src/runtime/test_runner/bun_test.rs +++ b/src/runtime/test_runner/bun_test.rs @@ -903,6 +903,17 @@ impl BunTest { let _ = self.result_queue.write_item(result); // OOM/capacity: fire-and-forget } + /// Retain error attribution while closing a file, including one whose load + /// failed before execution. Close callbacks must not resume its tests or hooks. + pub(crate) fn begin_file_teardown(&mut self) { + debug_assert!(!self.in_run_loop); + self.phase = Phase::Done; + self.wants_wakeup = false; + if self.timer.state == EventLoopTimerState::ACTIVE { + vm_timer().remove(&raw mut self.timer); + } + } + pub(crate) fn run(this_strong: &BunTestPtr, global_this: &JSGlobalObject) -> JsResult<()> { let _g = group_begin!(); // `Collection::step` / `Execution::step` re-enter and call `.get()` on @@ -1287,17 +1298,20 @@ impl BunTest { return; // the exception should not be visible (eg m_terminationException) }; + let mut file_failure = None; + let is_file_error = self.phase == Phase::Done; let failure_ctx: *mut core::ffi::c_void = 'ctx: { - if handle_status != HandleUncaughtExceptionResult::ShowHandledError { - break 'ctx core::ptr::null_mut(); - } let Some(reporter) = self.reporter else { break 'ctx core::ptr::null_mut(); }; // SAFETY: `BunTest.reporter` carries write provenance from `enter_file`'s // `&mut`; single-threaded test runner, no other borrow live here. let reporter = unsafe { &mut *reporter.as_ptr() }; - if reporter.jest.test_options.reporters.junit { + if !reporter.jest.test_options.reporters.junit { + core::ptr::null_mut() + } else if is_file_error { + core::ptr::from_mut(&mut file_failure).cast() + } else if handle_status == HandleUncaughtExceptionResult::ShowHandledError { core::ptr::from_mut(&mut reporter.test_failure).cast() } else { core::ptr::null_mut() @@ -1343,6 +1357,14 @@ impl BunTest { } Output::flush(); + if is_file_error && !failure_ctx.is_null() { + // SAFETY: the active file retains the caller-owned reporter through + // teardown. The formatter callback was cleared before moving its slot. + unsafe { + (*self.reporter.unwrap().as_ptr()) + .handle_unhandled_file_error(self.file_id, file_failure); + } + } } } @@ -1974,4 +1996,3 @@ pub(crate) use super::execution as Execution; pub(crate) use super::debug; pub(crate) use super::scope_functions as ScopeFunctions; pub(crate) use super::order as Order; - diff --git a/test/cli/test/isolation.test.ts b/test/cli/test/isolation.test.ts index 38999d3e92bc..64da6e27dd8b 100644 --- a/test/cli/test/isolation.test.ts +++ b/test/cli/test/isolation.test.ts @@ -73,6 +73,197 @@ describe.concurrent("bun test --isolate", () => { expect(exitCode).toBe(0); }); + describe.each([ + { + label: "serial non-last file", + mode: "--isolate", + owner: "a-owner.test.ts", + observer: "b-observer.test.ts", + scale: "0", + }, + { + label: "parallel file", + mode: "--parallel=2", + owner: "a-owner.test.ts", + observer: "b-observer.test.ts", + scale: "0", + }, + { + label: "last file on a worker", + mode: "--parallel=2", + owner: "z-owner.test.ts", + observer: "a-observer.test.ts", + scale: "60000", + }, + ])("file teardown errors: $label", ({ label, mode, owner, observer, scale }) => { + test.each(["throw", "handled"] as const)("%s during cleanup retains file ownership", async outcome => { + using dir = tempDir("isolate-teardown-error", { + [owner]: ` + import { test } from "bun:test"; + import { appendFileSync } from "node:fs"; + import { Socket } from "node:net"; + const record = message => appendFileSync("events.txt", message + "\\n"); + const worker = process.env.BUN_TEST_WORKER_ID ?? "serial"; + test("owner passes", async () => { + record("start owner " + worker); + const { promise: accepted, resolve } = Promise.withResolvers(); + const listener = Bun.listen({ + hostname: "127.0.0.1", port: 0, + socket: { + open() { resolve(); }, + data() {}, + close() { + record("close owner " + worker); + const reason = new Error("file teardown sentinel"); + ${ + outcome === "throw" + ? "throw reason;" + : ` + const socket = new Socket(); + socket.on("error", error => record("handled socket " + (error === reason))); + socket.destroy(reason); + Promise.reject(reason).catch(error => record("caught rejection " + (error === reason))); + ` + } + }, + }, + }); + await Bun.connect({ hostname: "127.0.0.1", port: listener.port, socket: { data() {} } }); + await accepted; + }); + `, + [observer]: ` + import { test } from "bun:test"; + import { appendFileSync } from "node:fs"; + test("observer passes", () => { + appendFileSync("events.txt", "start observer " + (process.env.BUN_TEST_WORKER_ID ?? "serial") + "\\n"); + }); + `, + }); + const { stdout, stderr, exitCode } = await runTests( + String(dir), + [mode, "--reporter=junit", "--reporter-outfile=out.xml"], + [`./${owner}`, `./${observer}`], + { ...bunEnv, BUN_TEST_PARALLEL_SCALE_MS: scale }, + ); + const events = fs + .readFileSync(join(String(dir), "events.txt"), "utf8") + .trim() + .split("\n"); + expect(events.filter(event => event.startsWith("close owner "))).toHaveLength(1); + expect(stderr).toContain("2 pass"); + expect(stderr).toContain("0 fail"); + if (mode.startsWith("--parallel")) { + expect(stdout).toContain("PARALLEL"); + expect(events.find(event => event.startsWith("start owner "))).toMatch(/^start owner \d+$/); + } + if (label === "last file on a worker") { + const starts = events.filter(event => event.startsWith("start ")).map(event => event.split(" ")); + expect(starts.map(([, name]) => name)).toEqual(["observer", "owner"]); + expect(starts[0][2]).toBe(starts[1][2]); + } + const report = await require("xml2js").parseStringPromise(fs.readFileSync(join(String(dir), "out.xml"), "utf8"), { + strict: true, + }); + const ownerSuite = report.testsuites.testsuite.find(suite => suite.$.file === owner); + const observerSuite = report.testsuites.testsuite.find(suite => suite.$.file === observer); + const failures = ownerSuite.testcase.filter(testcase => testcase.failure); + expect(observerSuite.$.failures).toBe("0"); + expect(ownerSuite.testcase.find(testcase => testcase.$.name === "owner passes").failure).toBeUndefined(); + if (outcome === "throw") { + expect(exitCode).toBe(1); + expect(stderr).toContain("1 error"); + expect(ownerSuite.$.failures).toBe("1"); + expect(failures).toHaveLength(1); + expect(failures[0].$).toMatchObject({ name: "(unhandled error)", file: owner, assertions: "0" }); + expect(failures[0].failure[0].$).toEqual({ type: "Error", message: "file teardown sentinel" }); + expect(failures[0].failure[0]._).toContain(owner); + } else { + expect(exitCode).toBe(0); + expect(stderr).not.toContain("Unhandled error"); + expect(ownerSuite.$.failures).toBe("0"); + expect(failures).toEqual([]); + expect(events.filter(event => event === "handled socket true")).toHaveLength(1); + expect(events.filter(event => event === "caught rejection true")).toHaveLength(1); + } + }); + }); + + test.each(["--isolate", "--parallel=2"])("%s does not resume a failed file during teardown", async mode => { + using dir = tempDir("isolate-failed-load-teardown", { + "preload.ts": ` + import { beforeAll, afterAll } from "bun:test"; + import { appendFileSync } from "node:fs"; + beforeAll(() => { + if (globalThis.pendingOwnerHook) { + appendFileSync("events.txt", "beforeAll owner\\n"); + return globalThis.pendingOwnerHook.promise; + } + }); + afterAll(() => appendFileSync("events.txt", "afterAll " + (globalThis.pendingOwnerHook ? "owner" : "observer") + "\\n")); + `, + "a-owner.test.ts": ` + import { test } from "bun:test"; + import { appendFileSync } from "node:fs"; + const record = message => appendFileSync("events.txt", message + "\\n"); + globalThis.pendingOwnerHook = Promise.withResolvers(); + test("must not run", () => record("unexpected test")); + const { promise: accepted, resolve } = Promise.withResolvers(); + const listener = Bun.listen({ + hostname: "127.0.0.1", port: 0, + socket: { + open() { resolve(); }, + data() {}, + close() { + record("close owner"); + globalThis.pendingOwnerHook.resolve(); + throw new Error("failed-load teardown sentinel"); + }, + }, + }); + await Bun.connect({ hostname: "127.0.0.1", port: listener.port, socket: { data() {} } }); + await accepted; + throw new Error("module load sentinel"); + `, + "b-observer.test.ts": ` + import { test } from "bun:test"; + import { appendFileSync } from "node:fs"; + test("observer passes", () => appendFileSync("events.txt", "observer\\n")); + `, + }); + const { stdout, stderr, exitCode } = await runTests( + String(dir), + [mode, "--preload=./preload.ts", "--reporter=junit", "--reporter-outfile=out.xml"], + ["./a-owner.test.ts", "./b-observer.test.ts"], + { ...bunEnv, BUN_TEST_PARALLEL_SCALE_MS: "0" }, + ); + expect(exitCode).toBe(1); + if (mode.startsWith("--parallel")) expect(stdout).toContain("PARALLEL"); + expect(stderr).toContain("module load sentinel"); + expect(stderr).toContain("failed-load teardown sentinel"); + const events = fs + .readFileSync(join(String(dir), "events.txt"), "utf8") + .trim() + .split("\n"); + expect(events.filter(event => event === "beforeAll owner")).toHaveLength(1); + expect(events.filter(event => event === "close owner")).toHaveLength(1); + expect(events.indexOf("beforeAll owner")).toBeLessThan(events.indexOf("close owner")); + expect(events.filter(event => event === "observer")).toHaveLength(1); + expect(events.filter(event => event === "afterAll observer")).toHaveLength(1); + expect(events).not.toContain("afterAll owner"); + expect(events).not.toContain("unexpected test"); + const report = await require("xml2js").parseStringPromise(fs.readFileSync(join(String(dir), "out.xml"), "utf8"), { + strict: true, + }); + const ownerSuite = report.testsuites.testsuite.find(suite => suite.$.file === "a-owner.test.ts"); + const observerSuite = report.testsuites.testsuite.find(suite => suite.$.file === "b-observer.test.ts"); + expect(observerSuite.$.failures).toBe("0"); + expect(ownerSuite.$.failures).toBe("1"); + expect(ownerSuite.testcase).toHaveLength(1); + expect(ownerSuite.testcase[0].$).toMatchObject({ name: "(unhandled error)", file: "a-owner.test.ts" }); + expect(ownerSuite.testcase[0].failure[0].$.message).toBe("failed-load teardown sentinel"); + }); + test.each(["--isolate", "--parallel=2"])("%s releases each file's stdio polls and preserves output", async mode => { const names = ["a", "b", "c", "d"].map(c => `${c}-stdio.test.ts`); using dir = tempDir( diff --git a/test/js/node/http/node-http.test.ts b/test/js/node/http/node-http.test.ts index 9296bd11e22f..d190cf0f0b3a 100644 --- a/test/js/node/http/node-http.test.ts +++ b/test/js/node/http/node-http.test.ts @@ -619,7 +619,6 @@ describe("node:http", () => { describe("request", () => { function runTest(done: Function, callback: (server: Server, port: number, done: (err?: Error) => void) => void) { - let timer; const server = createServer((req, res) => { if (Object.getPrototypeOf(req.headers) !== Object.prototype) { // Like Node.js, req.headers is a plain object. @@ -630,7 +629,7 @@ describe("node:http", () => { if (reqUrl.pathname === "/redirect") { // Temporary redirect res.writeHead(301, { - Location: `http://localhost:${server.port}/redirected`, + Location: `http://localhost:${(server.address() as AddressInfo).port}/redirected`, }); res.end("Got redirect!\n"); return; @@ -660,22 +659,8 @@ describe("node:http", () => { res.end("Hello World"); return; } - if (reqUrl.pathname.includes("timeout")) { - if (timer) clearTimeout(timer); - req.on("timeout", () => { - console.log("req timeout"); - }); - res.on("timeout", () => { - console.log("res timeout"); - }); - timer = setTimeout(() => { - if (res.closed) { - return; - } - - res.end("Hello World"); - timer = null; - }, 3000).unref(); + if (reqUrl.pathname === "/timeout") { + // The client owns timeout cleanup; this route must never send a response. return; } if (reqUrl.pathname === "/pathTest") { @@ -752,12 +737,11 @@ describe("node:http", () => { res.end("Hello World"); } }); - server.listen({ port: 0 }, (_, __, port) => { - var _done = (...args) => { - server.close(); - done(...args); + server.listen({ port: 0 }, () => { + const _done = (err?: Error) => { + server.close(closeError => done(err ?? closeError)); }; - callback(server, port, _done); + callback(server, (server.address() as AddressInfo).port, _done); }); } @@ -913,40 +897,76 @@ describe("node:http", () => { }); it("should allow us to set timeout with request.setTimeout or `timeout` in options", done => { - runTest(done, (server, serverPort, done) => { - const createDone = createDoneDotAll(done); - const req1Done = createDone(); - const req2Done = createDone(); - - const req1 = request( - { - host: "localhost", - port: serverPort, - path: "/timeout", - timeout: 500, - }, - res => { - req1Done(new Error("Should not have received response")); - }, - ); - req1.on("timeout", () => req1Done()); - - const req2 = request( - { - host: "localhost", - port: serverPort, - path: "/timeout", - }, - res => { - req2Done(new Error("Should not have received response")); - }, - ); - - req2.setTimeout(500, () => { - req2Done(); - }); - req1.end(); - req2.end(); + runTest(done, async (server, serverPort, done) => { + const requests: { + req: http.ClientRequest; + reason: Error; + events: string[]; + errors: Error[]; + destroyedAtTimeout: boolean[]; + closed: Promise; + }[] = []; + let failure; + try { + for (const useOptions of [true, false]) { + const reason = new Error(useOptions ? "options timeout cleanup" : "setTimeout cleanup"); + const req = request({ + host: "localhost", + port: serverPort, + path: "/timeout", + ...(useOptions ? { timeout: 500 } : {}), + }); + const { promise: closed, resolve: onClose } = Promise.withResolvers(); + const state = { + req, + reason, + events: [] as string[], + errors: [] as Error[], + destroyedAtTimeout: [] as boolean[], + closed, + }; + requests.push(state); + req.on("socket", () => state.events.push("socket")); + req.on("response", res => { + state.events.push("response"); + res.resume(); + req.destroy(reason); + }); + req.on("error", error => { + state.events.push("error"); + state.errors.push(error); + }); + req.once("close", () => { + state.events.push("close"); + onClose(); + }); + const onTimeout = () => { + state.events.push("timeout"); + state.destroyedAtTimeout.push(req.destroyed); + req.destroy(reason); + }; + if (useOptions) req.on("timeout", onTimeout); + else req.setTimeout(500, onTimeout); + req.end(); + } + // Timeout only notifies. Settle both explicit destroys before assertions or server teardown. + await Promise.all(requests.map(({ closed }) => closed)); + for (const { req, reason, events, errors, destroyedAtTimeout } of requests) { + expect(events).toEqual(["socket", "timeout", "error", "close"]); + expect(destroyedAtTimeout).toEqual([false]); + expect(errors).toHaveLength(1); + expect(errors[0]).toBe(reason); + expect(req.destroyed).toBe(true); + } + } catch (error) { + failure = error; + } finally { + for (const { req, reason } of requests) { + if (!req.destroyed) req.destroy(reason); + } + await Promise.all(requests.map(({ closed }) => closed)); + done(failure); + } }); }); @@ -1351,16 +1371,30 @@ describe("node:http", () => { }); }); - it("should emit a socket event when connecting", async done => { - runTest(done, async (server, serverPort, done) => { + it("should emit a socket event when connecting", done => { + runTest(done, (server, serverPort, done) => { const req = request(`http://localhost:${serverPort}`, {}); - // Destroying an in-flight request surfaces a "socket hang up" error, - // exactly like Node.js; swallow it so it does not leak into the next - // test as an unhandled error. - req.on("error", () => {}); - req.on("socket", function onRequestSocket(socket) { + const events: string[] = []; + const errors: Error[] = []; + req.on("error", error => { + events.push("error"); + errors.push(error); + }); + req.on("socket", () => { + events.push("socket"); req.destroy(); - done(); + }); + req.once("close", () => { + events.push("close"); + try { + expect(events).toEqual(["socket", "error", "close"]); + expect(errors).toHaveLength(1); + expect(errors[0]).toHaveProperty("code", "ECONNRESET"); + expect(req.destroyed).toBe(true); + done(); + } catch (error) { + done(error); + } }); req.end(); }); diff --git a/test/js/node/net/net-abort-lookup-fixture.cjs b/test/js/node/net/net-abort-lookup-fixture.cjs new file mode 100644 index 000000000000..013a6380541d --- /dev/null +++ b/test/js/node/net/net-abort-lookup-fixture.cjs @@ -0,0 +1,79 @@ +const assert = require("node:assert/strict"); +const { once } = require("node:events"); +const { createServer, Socket } = require("node:net"); + +async function main() { + const autoSelectFamily = process.argv[2] === "true"; + const alreadyAborted = process.argv[3] === "true"; + const controller = new AbortController(); + const reason = new Error("lookup aborted"); + if (alreadyAborted) controller.abort(reason); + + const accepted = []; + const server = createServer(connection => { + accepted.push(connection); + connection.destroy(); + }); + const events = []; + const errors = []; + let socket; + let closed; + + try { + await once(server.listen(0, "127.0.0.1"), "listening"); + socket = new Socket({ signal: controller.signal }); + closed = new Promise(resolve => + socket.once("close", () => { + events.push("close"); + resolve(); + }), + ); + socket.on("error", error => { + events.push("error"); + errors.push(error); + }); + socket.on("connectionAttempt", () => events.push("connectionAttempt")); + socket.on("connect", () => events.push("connect")); + const { promise: lookupStarted, resolve: onLookup } = Promise.withResolvers(); + let completeLookup; + socket.connect({ + host: "held-lookup.test", + port: server.address().port, + autoSelectFamily, + lookup(_hostname, options, callback) { + completeLookup = () => + options.all ? callback(null, [{ address: "127.0.0.1", family: 4 }]) : callback(null, "127.0.0.1", 4); + onLookup(); + }, + }); + if (!alreadyAborted) { + await lookupStarted; + controller.abort(reason); + } + await closed; + + // A late successful lookup may emit "lookup", but must not resurrect a closed socket. + completeLookup?.(); + await new Promise(resolve => setImmediate(resolve)); + assert.deepEqual(events, ["error", "close"]); + assert.equal(errors.length, 1); + assert.equal(errors[0].name, "AbortError"); + assert.equal(errors[0].code, "ABORT_ERR"); + assert.equal(errors[0].cause, reason); + assert.equal(socket.destroyed, true); + assert.equal(accepted.length, 0); + } finally { + if (socket) { + socket.destroy(); + await closed; + } + for (const connection of accepted) connection.destroy(); + if (server.listening) await new Promise(resolve => server.close(resolve)); + } + console.log("abort assertions passed"); +} + +main().catch(error => { + console.error(error); + process.exitCode = 1; +}); diff --git a/test/js/node/net/node-net.test.ts b/test/js/node/net/node-net.test.ts index 89622e5814ff..c0a3e2099715 100644 --- a/test/js/node/net/node-net.test.ts +++ b/test/js/node/net/node-net.test.ts @@ -954,43 +954,40 @@ it("should not hang after destroy", async () => { } }, 120_000); -it("should trigger error when aborted even if connection failed #13126", async () => { - const signal = AbortSignal.timeout(100); - const socket = createConnection({ - host: "example.com", - port: 999, - signal: signal, - }); - const { promise, resolve, reject } = Promise.withResolvers(); - - socket.on("connect", reject); - socket.on("error", resolve); - - // Node destroys the socket with an AbortError carrying the signal's reason as `cause`. - const err = (await promise) as Error & { code?: string; cause?: Error }; - expect(err.name).toBe("AbortError"); - expect(err.code).toBe("ABORT_ERR"); - expect(err.cause?.name).toBe("TimeoutError"); -}); +describe("abort during lookup #13126", () => { + async function runFixture(exe: string, autoSelectFamily: boolean, alreadyAborted: boolean) { + await using proc = Bun.spawn({ + cmd: [ + exe, + join(import.meta.dir, "net-abort-lookup-fixture.cjs"), + String(autoSelectFamily), + String(alreadyAborted), + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: "abort assertions passed\n", + stderr: "", + exitCode: 0, + }); + } -it("should trigger error when aborted even if connection failed, and the signal is already aborted #13126", async () => { - const signal = AbortSignal.timeout(1); - await Bun.sleep(10); - const socket = createConnection({ - host: "example.com", - port: 999, - signal: signal, + describe.each(runtimesWithNode(24))("%s pending abort", (_, exe) => { + for (const autoSelectFamily of [false, true]) { + it(`autoSelectFamily=${autoSelectFamily}`, () => runFixture(exe, autoSelectFamily, false)); + } }); - const { promise, resolve, reject } = Promise.withResolvers(); - socket.on("connect", reject); - socket.on("error", resolve); - - // Node destroys the socket with an AbortError carrying the signal's reason as `cause`. - const err = (await promise) as Error & { code?: string; cause?: Error }; - expect(err.name).toBe("AbortError"); - expect(err.code).toBe("ABORT_ERR"); - expect(err.cause?.name).toBe("TimeoutError"); + // Node destroys a pre-aborted Socket during construction and only guarantees + // reconnecting after close. Bun's immediate-connect behavior is a separate contract. + describe("bun already-aborted immediate connect", () => { + for (const autoSelectFamily of [false, true]) { + it(`autoSelectFamily=${autoSelectFamily}`, () => runFixture(bunExe(), autoSelectFamily, true)); + } + }); }); // Regression test for #30697: net.connect({ localPort, lookup }) on the @@ -3649,6 +3646,54 @@ function closeWithReplacedTimers(mode, replaceBeforeImport) { }); }); } +// The raw libc listener used by blackholePortSource is unavailable on Windows and musl. +it.skipIf(isWindows || isMusl)("aborting a pending native connection emits one AbortError and closes", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + ${blackholePortSource} + const controller = new AbortController(); + const reason = new Error("pending connection aborted"); + const events = []; + const errors = []; + let attempts = 0; + const socket = new net.Socket({ signal: controller.signal }); + socket.on("connectionAttempt", () => attempts++); + socket.on("connect", () => events.push("connect")); + socket.on("error", error => { + events.push("error"); + errors.push({ name: error.name, code: error.code, causeMatches: error.cause === reason }); + }); + socket.on("close", () => { + events.push("close"); + filler.destroy(); + console.log(JSON.stringify({ attempts, events, errors, destroyed: socket.destroyed })); + }); + // Let connect() create its native pending handle before aborting it. + socket.once("connectionAttempt", () => setImmediate(() => controller.abort(reason))); + socket.connect({ host: "127.0.0.1", port }); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout, stderr, exitCode }).toEqual({ + stdout: + JSON.stringify({ + attempts: 1, + events: ["error", "close"], + errors: [{ name: "AbortError", code: "ABORT_ERR", causeMatches: true }], + destroyed: true, + }) + "\n", + stderr: "", + exitCode: 0, + }); +}); + // There is nothing to half-close yet. shutdown() used to mark the native socket as one that had // opened, so closing it raised a connection's events ('end') and the attempt never failed. it.skipIf(isWindows || isMusl)("closing a handle that was shut down while connecting fails its attempt", async () => {