diff --git a/packages/bun-usockets/src/eventing/libuv.c b/packages/bun-usockets/src/eventing/libuv.c index baa2fa36a85a..2a402a994a3b 100644 --- a/packages/bun-usockets/src/eventing/libuv.c +++ b/packages/bun-usockets/src/eventing/libuv.c @@ -181,11 +181,14 @@ static void close_cb_free(uv_handle_t *h) { us_free(h->data); } /* This one is different for polls, since we need two frees here */ static void close_cb_free_poll(uv_handle_t *h) { - /* It is only in case we called us_poll_stop then quickly us_poll_free that we - * enter this. Most of the time, actual freeing is done by us_poll_free. */ + /* us_poll_free normally re-pointed data at the us_poll_t before this runs. + * If a nested tick's loop_post deferred the closed-socket sweep (see + * tick_depth in us_loop_run), we run first: mark the handle for us_poll_free. */ if (h->data) { us_free(h->data); us_free(h); + } else { + h->data = h; } } @@ -213,6 +216,14 @@ void us_poll_free(struct us_poll_t *p, struct us_loop_t *loop) { us_free(p); return; } + /* close_cb_free_poll already ran: a nested tick deferred this sweep to the + * outermost loop_post (tick_depth), so libuv finished closing the handle + * before we got here. It is done with uv_p; we free both. */ + if (p->uv_p->data == (void *)p->uv_p) { + us_free(p->uv_p); + us_free(p); + return; + } /* The idea here is like so; in us_poll_stop we call uv_close after setting * data of uv-poll to 0. This means that in close_cb_free we call free on 0 * with does nothing, since us_poll_stop should not really free the poll. @@ -329,10 +340,17 @@ void us_loop_pump(struct us_loop_t *loop) { * for unref'd handles (subprocess exit packets, socket events) and due * timers are never processed. Bun's outer drive loops (wait_for_promise, * bun:test) supply their own keep-going predicate, so force exactly one - * non-blocking iteration; UV_RUN_NOWAIT keeps the poll timeout at 0. */ + * non-blocking iteration; UV_RUN_NOWAIT keeps the poll timeout at 0. + * + * This is the nested-tick entry point (wait_for_promise from inside a poll + * callback), and the forced iteration always reaches the check phase, i.e. + * us_internal_loop_post. The tick_depth bracket is what makes that post + * defer the closed-socket sweep to the outermost tick; see us_loop_run. */ + loop->data.tick_depth++; loop->uv_loop->active_handles++; uv_run(loop->uv_loop, UV_RUN_NOWAIT); loop->uv_loop->active_handles--; + loop->data.tick_depth--; } struct us_loop_t *us_create_loop(void *hint, @@ -401,6 +419,11 @@ void us_loop_run(struct us_loop_t *loop) { us_loop_integrate(loop); uv_update_time(loop->uv_loop); + /* us_internal_loop_post runs from the uv_check_t registered above, so it + * fires inside uv_run; the tick_depth bracket mirrors us_loop_run and + * us_loop_run_bun_tick in epoll_kqueue.c. See us_internal_loop_post. */ + loop->data.tick_depth++; + /* UV_RUN_ONCE may block in the poll phase (pending callbacks dispatch * first), making this the JS thread's park hook, the counterpart of * us_loop_run_bun_tick's. jsc_vm is only set on the JS thread's loop. */ @@ -411,6 +434,7 @@ void us_loop_run(struct us_loop_t *loop) { } uv_run(loop->uv_loop, UV_RUN_ONCE); + loop->data.tick_depth--; } struct us_poll_t *us_create_poll(struct us_loop_t *loop, int fallthrough, diff --git a/packages/bun-usockets/src/internal/loop_data.h b/packages/bun-usockets/src/internal/loop_data.h index 3937ea5d70d5..b4fad5c3feb5 100644 --- a/packages/bun-usockets/src/internal/loop_data.h +++ b/packages/bun-usockets/src/internal/loop_data.h @@ -95,10 +95,11 @@ struct us_internal_loop_data_t { /* We do not care if this flips or not, it doesn't matter */ size_t iteration_nr; void* jsc_vm; - /* Reentrancy depth of us_loop_run_bun_tick. When >1, we are inside a - * nested tick (e.g. waitForPromise from a poll callback). Freeing closed - * sockets must be deferred to the outermost tick so the outer dispatch - * doesn't read a freed poll. */ + /* Reentrancy depth of the loop-run entry points (us_loop_run_bun_tick and + * us_loop_run on epoll/kqueue; us_loop_run and us_loop_pump on libuv). + * When >1 we are inside a nested tick (e.g. waitForPromise from a poll + * callback). Freeing closed sockets must be deferred to the outermost + * tick so the outer dispatch doesn't read a freed poll. */ int tick_depth; }; diff --git a/patches/libuv/win-poll-no-reendgame-after-close.patch b/patches/libuv/win-poll-no-reendgame-after-close.patch new file mode 100644 index 000000000000..6f6e536bd262 --- /dev/null +++ b/patches/libuv/win-poll-no-reendgame-after-close.patch @@ -0,0 +1,29 @@ +--- a/src/win/poll.c ++++ b/src/win/poll.c +@@ -217,8 +217,14 @@ static void uv__fast_poll_process_poll_req(uv_loop_t* loop, uv_poll_t* handle, + handle->submitted_events_2)) != 0) { + uv__fast_poll_submit_poll_req(loop, handle); + } else if ((handle->flags & UV_HANDLE_CLOSING) && ++ !(handle->flags & UV_HANDLE_CLOSED) && + handle->submitted_events_1 == 0 && + handle->submitted_events_2 == 0) { ++ /* A nested uv_run entered from inside poll_cb may have already run this ++ * handle's endgame: uv__process_endgames cleared ENDGAME_QUEUED and ++ * uv__handle_close set CLOSED. Re-queuing it here would run the endgame ++ * again, which uv__poll_endgame asserts against and which invokes ++ * close_cb a second time. */ + uv__want_endgame(loop, (uv_handle_t*) handle); + } + } +@@ -418,8 +424,11 @@ static void uv__slow_poll_process_poll_req(uv_loop_t* loop, uv_poll_t* handle, + handle->submitted_events_2)) != 0) { + uv__slow_poll_submit_poll_req(loop, handle); + } else if ((handle->flags & UV_HANDLE_CLOSING) && ++ !(handle->flags & UV_HANDLE_CLOSED) && + handle->submitted_events_1 == 0 && + handle->submitted_events_2 == 0) { ++ /* Same as the fast path: never re-queue an endgame that a nested uv_run ++ * already ran for this handle. */ + uv__want_endgame(loop, (uv_handle_t*) handle); + } + } diff --git a/scripts/build/deps/libuv.ts b/scripts/build/deps/libuv.ts index 3ff7e346c9f0..db4cd838ed01 100644 --- a/scripts/build/deps/libuv.ts +++ b/scripts/build/deps/libuv.ts @@ -53,7 +53,18 @@ export const libuv: Dependency = { // an in-process loopback fetch().abort() can fall into. To upstream: // send to libuv/libuv with the wepoll/ReactOS references in the patch // comment as the rationale. - patches: ["patches/libuv/win-poll-rearm-before-callback.patch", "patches/libuv/win-poll-abort-with-disconnect.patch"], + // + // win-poll-no-reendgame-after-close: the post-poll_cb endgame check in + // uv__fast_poll_process_poll_req (and its slow-poll sibling) re-queues a + // handle whose endgame a nested uv_run, entered from inside poll_cb, + // already ran, which double-invokes close_cb. Guard the check on + // !(flags & UV_HANDLE_CLOSED), which uv__poll_endgame already asserts. + // Nested uv_run is outside libuv's contract, so this is not upstreamable. + patches: [ + "patches/libuv/win-poll-rearm-before-callback.patch", + "patches/libuv/win-poll-abort-with-disconnect.patch", + "patches/libuv/win-poll-no-reendgame-after-close.patch", + ], build: () => ({ kind: "direct", diff --git a/scripts/build/fetch-cli.ts b/scripts/build/fetch-cli.ts index 63d06d5fb8c4..c53fa954c229 100644 --- a/scripts/build/fetch-cli.ts +++ b/scripts/build/fetch-cli.ts @@ -242,13 +242,18 @@ function normalizeLf(s: string): string { * so a CRLF-mangled checkout still applies cleanly. --no-index: dest/ is * not a git repo. --ignore-whitespace / --ignore-space-change: patches are * authored against upstream which may have different trailing whitespace. + * + * GIT_CEILING_DIRECTORIES hides the enclosing repo: run from a repo subdir, + * `git apply` treats a git-format (`diff --git`) patch as toplevel-relative and + * silently skips it with exit 0. -v + LC_ALL=C make that skip detectable below. */ function applyPatch(dest: string, patchPath: string, patchBody: string): void { - const result = spawnSync("git", ["apply", "--ignore-whitespace", "--ignore-space-change", "--no-index", "-"], { + const result = spawnSync("git", ["apply", "--ignore-whitespace", "--ignore-space-change", "--no-index", "-v", "-"], { cwd: dest, input: normalizeLf(patchBody), stdio: ["pipe", "ignore", "pipe"], encoding: "utf8", + env: { ...process.env, GIT_CEILING_DIRECTORIES: join(dest, ".."), LC_ALL: "C" }, }); if (result.error) { @@ -264,6 +269,16 @@ function applyPatch(dest: string, patchPath: string, patchBody: string): void { hint: "The patch may be out of date with the pinned commit", }); } + + // A file git considers outside the current directory is skipped with exit 0. + // Nothing under patches/ should ever be skipped: a skip here means the .ref + // stamp would certify a tree the patch never touched. + if (result.stderr !== null && result.stderr.includes("Skipped patch")) { + throw new BuildError(`git apply skipped one or more files: ${result.stderr.trim()}`, { + file: patchPath, + hint: "GIT_CEILING_DIRECTORIES should prevent this. A skip means git resolved the patch's paths outside the dependency's source directory.", + }); + } } // Only run if this file is the entry point (not imported as a module). diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index 58b7c760ff81..d4e760232fb2 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -3827,3 +3827,59 @@ describe("allowHalfOpen socket whose peer resets behind pending writes", () => { expect(endCount).toBe(1); }); }); + +// On the libuv (Windows) event loop backend, a synchronous re-entrant loop +// tick from inside a socket's data callback ran the closed-socket sweep and +// freed the us_socket_t the suspended outer dispatch frame still held. +it("survives closing a socket and re-entering the event loop from its own data callback", async () => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + ` + // The setImmediate callback only runs inside the nested autoTick that + // transform()'s waitForPromise performs, so reentries === 20 proves + // every round really re-entered the loop and didn't just terminate(). + let reentries = 0; + const server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + socket: { + data(sock) { + // Synchronously close the socket, then synchronously re-enter the + // event loop: the element handler returns a still-pending promise, + // so .transform() spins waitForPromise -> autoTick -> a nested tick. + sock.terminate(); + new HTMLRewriter() + .on("p", { element: () => new Promise(r => setImmediate(() => { reentries++; r(); })) }) + .transform("

"); + }, + close() {}, + error() {}, + }, + }); + for (let i = 0; i < 20; i++) { + const { promise, resolve } = Promise.withResolvers(); + Bun.connect({ + hostname: "127.0.0.1", + port: server.port, + socket: { open: s => s.write("x"), data() {}, close: resolve, end: resolve, error: resolve, connectError: resolve }, + }).catch(resolve); + await promise; + } + server.stop(true); + console.log("SURVIVED " + reentries); + `, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect({ stdout: stdout.trim(), stderr, exitCode }).toEqual({ + stdout: "SURVIVED 20", + stderr: expect.any(String), + exitCode: 0, + }); +});