diff --git a/packages/bun-usockets/src/eventing/libuv.c b/packages/bun-usockets/src/eventing/libuv.c index ea5e72904745..0f6f627b1a4a 100644 --- a/packages/bun-usockets/src/eventing/libuv.c +++ b/packages/bun-usockets/src/eventing/libuv.c @@ -170,10 +170,48 @@ static void prepare_cb(uv_prepare_t *p) { us_internal_loop_pre(loop); } -/* Note: libuv timers execute AFTER the post callback */ +/* uv_run finishes closing handles - unlink from the loop, then the close + * callback, which is where an owner frees its handle - right after the check + * phase (uv__process_endgames). A nested tick (see us_internal_uv_run) must + * not: it runs inside a callback that an outer uv_run dispatched, and libuv's + * dispatch reads that callback's handle again once it returns. A poll closed + * from its own poll_cb (us_socket_close -> us_poll_stop) is the common case: + * the nested run would complete its close, and uv__fast_poll_process_poll_req + * would then find the handle "closing, nothing outstanding" a second time and + * queue its endgame again - two close callbacks, and uv__handle_close on an + * already unlinked handle. So a nested tick takes what it queued for closing + * off the loop and the outermost tick puts it back. A held handle keeps + * UV_HANDLE_ENDGAME_QUEUED set, so uv__want_endgame leaves it alone. */ +static void us_internal_hold_endgames(struct us_loop_t *loop) { + uv_handle_t *queued = loop->uv_loop->endgame_handles; + if (!queued) return; + uv_handle_t *last = queued; + while (last->endgame_next) last = last->endgame_next; + last->endgame_next = (uv_handle_t *)loop->data.held_endgames; + loop->data.held_endgames = queued; + loop->uv_loop->endgame_handles = NULL; +} + +static void us_internal_release_held_endgames(struct us_loop_t *loop) { + uv_handle_t *held = (uv_handle_t *)loop->data.held_endgames; + if (!held) return; + uv_handle_t *last = held; + while (last->endgame_next) last = last->endgame_next; + last->endgame_next = loop->uv_loop->endgame_handles; + loop->uv_loop->endgame_handles = held; + loop->data.held_endgames = NULL; +} + +/* Note: libuv timers execute AFTER the post callback; uv__process_endgames is + * what runs right after it. */ static void check_cb(uv_check_t *p) { struct us_loop_t *loop = p->data; us_internal_loop_post(loop); + if (loop->data.tick_depth > 1) { + us_internal_hold_endgames(loop); + } else { + us_internal_release_held_endgames(loop); + } } /* Not used for polls, since polls need two frees */ @@ -326,6 +364,44 @@ void us_internal_poll_set_type(struct us_poll_t *p, int poll_type) { LIBUS_SOCKET_DESCRIPTOR us_poll_fd(struct us_poll_t *p) { return p->fd; } +extern void Bun__JSEventLoop__enter(void *event_loop); +extern void Bun__JSEventLoop__exit(void *event_loop); + +/* The jsc::EventLoop this loop belongs to (parent_tag 1; 2 is a MiniEventLoop, + * which runs no JS), while its VM is alive (jsc_vm is cleared at teardown). */ +static void *us_internal_js_event_loop(struct us_loop_t *loop) { + return loop->data.parent_tag == 1 && loop->data.jsc_vm ? loop->data.parent_ptr : NULL; +} + +/* Every uv_run of a JS thread's loop is one event-loop scope (EventLoop::enter + * / exit). libuv dispatches its callbacks from inside uv_run, so each JS + * callback they run is a nested scope whose exit is not the outermost one and + * therefore not a microtask checkpoint: the nextTicks and promise reactions a + * callback queues run here, once uv_run has returned, instead of on libuv's + * dispatch frame - where a continuation that drives the loop again + * (waitForPromise) would nest uv_run inside the callback. */ +static void us_internal_uv_run(struct us_loop_t *loop, uv_run_mode mode) { + void *js_event_loop = us_internal_js_event_loop(loop); + if (js_event_loop) Bun__JSEventLoop__enter(js_event_loop); + /* The scope above only moves microtask-driven re-entry out of uv_run. A + * callback whose own body drives the loop again (waitForPromise: bun:test's + * expect(promise).resolves, process.exit()'s drain) still nests a tick inside + * the outer uv_run's dispatch, which reads what it dispatched on once the + * callback returns. tick_depth > 1 marks that nested tick, and what the outer + * dispatch may still point at is then left for the outermost tick to free: + * closed sockets (us_internal_loop_post) and closing libuv handles + * (check_cb). Same bracket as us_loop_run / us_loop_run_bun_tick on + * epoll/kqueue. */ + loop->data.tick_depth++; + uv_run(loop->uv_loop, mode); + loop->data.tick_depth--; + /* A nested tick run from a timer callback holds after this run's check + * phase; nothing of libuv is on the stack any more, so hand those back for + * the next run to close. */ + if (loop->data.tick_depth == 0) us_internal_release_held_endgames(loop); + if (js_event_loop) Bun__JSEventLoop__exit(js_event_loop); +} + void us_loop_pump(struct us_loop_t *loop) { /* POSIX parity: us_loop_run_bun_tick polls epoll/kqueue and dispatches * regardless of ref state (it only early-outs on num_polls == 0). libuv's @@ -335,7 +411,7 @@ void us_loop_pump(struct us_loop_t *loop) { * bun:test) supply their own keep-going predicate, so force exactly one * non-blocking iteration; UV_RUN_NOWAIT keeps the poll timeout at 0. */ loop->uv_loop->active_handles++; - uv_run(loop->uv_loop, UV_RUN_NOWAIT); + us_internal_uv_run(loop, UV_RUN_NOWAIT); loop->uv_loop->active_handles--; } @@ -414,7 +490,7 @@ void us_loop_run(struct us_loop_t *loop) { Bun__JSC_onBeforeWait(loop->data.jsc_vm, (uint64_t) uv_now(loop->uv_loop) * 1000000ULL); } - uv_run(loop->uv_loop, UV_RUN_ONCE); + us_internal_uv_run(loop, UV_RUN_ONCE); } 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..364916c19c9a 100644 --- a/packages/bun-usockets/src/internal/loop_data.h +++ b/packages/bun-usockets/src/internal/loop_data.h @@ -95,11 +95,18 @@ 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 tick (us_loop_run_bun_tick on epoll/kqueue, + * us_loop_run / 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; +#ifdef LIBUS_USE_LIBUV + /* uv_handle_t list (linked through endgame_next): handles a nested tick + * would have finished closing, held for the outermost tick. See check_cb + * in libuv.c. */ + void *held_endgames; +#endif }; #endif // LOOP_DATA_H diff --git a/src/jsc/event_loop.rs b/src/jsc/event_loop.rs index 31020cc33854..4d00b26f4527 100644 --- a/src/jsc/event_loop.rs +++ b/src/jsc/event_loop.rs @@ -1414,6 +1414,18 @@ pub fn event_loop_exit(global: &JSGlobalObject) { global.bun_vm().event_loop_mut().exit(); } +/// `this` is the loop's `internal_loop_data.parent_ptr` (uSockets' libuv +/// backend brackets `uv_run` with these). +// HOST_EXPORT(Bun__JSEventLoop__enter, c) +pub fn js_event_loop_enter(this: &mut crate::event_loop::EventLoop) { + this.enter(); +} + +// HOST_EXPORT(Bun__JSEventLoop__exit, c) +pub fn js_event_loop_exit(this: &mut crate::event_loop::EventLoop) { + this.exit(); +} + // ────────────────────────────────────────────────────────────────────────── // `bun_event_loop::any_event_loop::js` extern impls // diff --git a/src/uws_sys/InternalLoopData.rs b/src/uws_sys/InternalLoopData.rs index 5add2c03a003..9d39d073964c 100644 --- a/src/uws_sys/InternalLoopData.rs +++ b/src/uws_sys/InternalLoopData.rs @@ -63,6 +63,10 @@ pub struct InternalLoopData { // Higher tier (`bun_runtime`) casts this back when reading. pub jsc_vm: *const c_void, pub tick_depth: c_int, + /// `uv_handle_t *` list of closing handles held back from a nested tick + /// (libuv.c `check_cb`). + #[cfg(windows)] + pub held_endgames: *mut c_void, } impl InternalLoopData { diff --git a/test/js/bun/net/close-inside-data-reentrant-fixture.ts b/test/js/bun/net/close-inside-data-reentrant-fixture.ts new file mode 100644 index 000000000000..b56957a32542 --- /dev/null +++ b/test/js/bun/net/close-inside-data-reentrant-fixture.ts @@ -0,0 +1,49 @@ +// Spawned by socket.test.ts as `bun test `: it has to run under the +// test runner because expect(promise).resolves waits by driving the event loop +// synchronously, which is what nests event-loop ticks inside the socket's data +// callback while the dispatch for that socket is still on the stack. +import { expect, test } from "bun:test"; + +test("a socket closed inside its data callback survives nested event-loop ticks until the dispatch returns", async () => { + using server = Bun.listen({ + hostname: "127.0.0.1", + port: 0, + socket: { + open(socket) { + socket.write("x"); + }, + data() {}, + }, + }); + + for (let i = 0; i < 8; i++) { + const returned = Promise.withResolvers(); + const churn: Promise[] = []; + await Bun.connect({ + hostname: "127.0.0.1", + port: server.port, + socket: { + data(socket) { + // Closing moves the socket to the loop's closed list; it may only be + // freed once this callback (and the dispatch that called it) is done. + socket.terminate(); + // Nested ticks: timers, I/O and the loop's post phase all run here. + expect(new Promise(resolve => setTimeout(resolve, 5))).resolves.toBeUndefined(); + // Allocations of the same size class as the closed socket, so a + // prematurely freed block is likely to be handed out again before + // the outer dispatch looks at it. + for (let j = 0; j < 16; j++) { + churn.push( + Bun.connect({ hostname: "127.0.0.1", port: server.port, socket: { data() {} } }).then(s => s.terminate()), + ); + } + returned.resolve(); + }, + }, + }); + await returned.promise; + await Promise.all(churn); + // Let the outer dispatch unwind and the loop reach its post phase. + await new Promise(resolve => setImmediate(resolve)); + } +}); diff --git a/test/js/bun/net/socket.test.ts b/test/js/bun/net/socket.test.ts index a6ab1896db09..074acc7f136a 100644 --- a/test/js/bun/net/socket.test.ts +++ b/test/js/bun/net/socket.test.ts @@ -4433,3 +4433,25 @@ describe.concurrent("a socket closed by data() while its peer's reset is being d expect(exitCode).toBe(0); }); }); + +describe.concurrent("a socket closed by data() which then re-enters the event loop before returning", () => { + // The fixture runs under `bun test` so that expect(promise).resolves can drive + // nested event-loop ticks from inside the data callback. The closed socket must + // stay allocated until the dispatch that invoked data() has returned; the loop + // used to free it from a nested tick on Windows, and the outer dispatch then + // read (and the allocator reused) freed memory. + it("is not freed until the dispatch that called data() has returned", async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), "test", fileURLToPath(new URL("./close-inside-data-reentrant-fixture.ts", import.meta.url))], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // stdout carries only the runner's version banner; results go to stderr. + expect(stdout).toMatch(/^bun test v\S+ \(\S+\)\n$/); + expect(stderr).toContain(" 1 pass"); + expect(proc.signalCode).toBeNull(); + expect(exitCode).toBe(0); + }); +});