diff --git a/src/ast/ast_memory_allocator.rs b/src/ast/ast_memory_allocator.rs index 322e5921eb76..ab3e58bea463 100644 --- a/src/ast/ast_memory_allocator.rs +++ b/src/ast/ast_memory_allocator.rs @@ -26,9 +26,14 @@ use crate::stmt; // it here; the next `ASTMemoryAllocator` on this thread reclaims it, reusing // its committed pages. The pool holds at most one arena (nested scopes — rare // — fall back to a fresh `Arena::new()`). `#[thread_local]` (not the -// `thread_local!` macro) so there is no destructor: a parked arena at thread -// exit is reclaimed by mimalloc's own thread-teardown, avoiding an unspecified -// destructor-ordering hazard with `mi_heap_destroy`. +// `thread_local!` macro) so there is no destructor racing mimalloc's own +// thread teardown. mimalloc does NOT destroy a parked heap when its thread +// exits (heaps are not thread-bound in our fork), so an exiting thread strands +// one empty `mi_heap_t`. That is acceptable only because the threads that park +// here (bundler/install pool threads, the bundle thread) live as long as the +// process; per-thread state on threads that come and go (Worker VMs) must be +// owned by something torn down with the thread instead, like +// `RuntimeState::text_format_arena` in `bun_runtime`. #[thread_local] static ARENA_POOL: Cell> = Cell::new(None); diff --git a/src/runtime/api.rs b/src/runtime/api.rs index 680029fafff1..ee280827fc00 100644 --- a/src/runtime/api.rs +++ b/src/runtime/api.rs @@ -265,22 +265,27 @@ fn with_text_format_source_encoded( use crate::node::{BlobOrStringOrBuffer, StringOrBuffer}; // A private mi_heap costs microseconds to create, more than parsing a - // small document: keep one per thread and recycle it between calls. - // `#[thread_local]` rather than `thread_local!` so there is no - // destructor racing mimalloc's own thread teardown (as in - // `ast_memory_allocator.rs`); a parked heap is reclaimed with the thread. - #[thread_local] - static ARENA: core::cell::Cell> = core::cell::Cell::new(None); + // small document: keep one per VM thread (`RuntimeState::text_format_arena`, + // destroyed with the VM) and recycle it between calls. The slot is empty + // while a call is in flight, so a re-entrant call (the argument's + // `toString()` parsing another document) gets its own arena; whichever + // parks last wins and the other is destroyed. struct Recycle(Option); impl Drop for Recycle { fn drop(&mut self) { - if let Some(mut arena) = self.0.take() { + let Some(mut arena) = self.0.take() else { + return; + }; + // Re-fetched rather than captured at entry: if the VM state is + // gone by now the arena is simply dropped (`mi_heap_destroy`). + if let Some(slot) = crate::jsc_hooks::text_format_arena_slot() { arena.reset_retain_with_limit(2 * 1024 * 1024); - ARENA.set(Some(arena)); + slot.set(Some(arena)); } } } - let recycle = Recycle(Some(ARENA.take().unwrap_or_default())); + let parked = crate::jsc_hooks::text_format_arena_slot().and_then(core::cell::Cell::take); + let recycle = Recycle(Some(parked.unwrap_or_default())); let arena = recycle.0.as_ref().expect("set above"); let mut ast_memory_allocator = bun_ast::ASTMemoryAllocator::borrowing(arena); let _ast_scope = ast_memory_allocator.enter(); diff --git a/src/runtime/jsc_hooks.rs b/src/runtime/jsc_hooks.rs index edb355d3b465..74e8911f0a56 100644 --- a/src/runtime/jsc_hooks.rs +++ b/src/runtime/jsc_hooks.rs @@ -103,6 +103,12 @@ pub(crate) struct RuntimeState { /// The resolver's PackageManager wake-handler context (module queue + VM /// handle); the resolver holds a raw pointer to it. Freed with the state. pub(crate) wake_ctx: Option>, + /// Parked `mi_heap` recycled between `Bun.{TOML,YAML,JSON5,JSONC,XML}.parse` + /// calls on this thread (see `api::with_text_format_source_encoded`). + /// Owned here rather than in a `#[thread_local]` so Worker teardown + /// destroys it: mimalloc does not free a `mi_heap_t` when the thread that + /// created it exits, so a heap parked in TLS leaks with every Worker. + pub(crate) text_format_arena: Cell>, } #[derive(Clone, Copy, PartialEq, Eq, Hash)] @@ -239,6 +245,24 @@ pub(crate) fn global_dns_data() -> &'static core::cell::OnceCell Option<&'static Cell>> { + let state = runtime_state(); + if state.is_null() { + return None; + } + // SAFETY: `state` is the live per-thread `RuntimeState` box; the field + // address is stable until `deinit_runtime_state`, which nulls + // `RUNTIME_STATE` before freeing the box, so a non-null `state` here is + // never a freed one. Mutation goes through the `Cell`. + Some(unsafe { &(*state).text_format_arena }) +} + /// Recover the [`RuntimeState`] owned by a specific `vm` (not the calling /// thread's). `WTFTimer` may be entered off the VM's JS thread (the locked /// `All.wtf_timers` heap exists for exactly that), and the @@ -400,6 +424,7 @@ unsafe fn init_runtime_state( }, active_handles: ActiveHandles::default(), wake_ctx: None, + text_format_arena: Cell::new(None), })); RUNTIME_STATE.with(|c| c.set(state)); diff --git a/test/js/node/worker_threads/worker-heap-leak-fixture.js b/test/js/node/worker_threads/worker-heap-leak-fixture.js new file mode 100644 index 000000000000..35ca26e685f7 --- /dev/null +++ b/test/js/node/worker_threads/worker-heap-leak-fixture.js @@ -0,0 +1,34 @@ +// Several APIs give the calling thread a private mimalloc heap and keep it around for the next call: +// Bun.{TOML,YAML,JSON5,JSONC,XML}.parse park one between calls, the module loader keeps one per VM for +// transpiling. mimalloc does not destroy such heaps when their thread exits, so unless the Worker's VM +// teardown frees them, every Worker that used one of these APIs leaks a heap plus whatever pages it +// still holds. heapStats({ dump: true }) lists every live heap in the process, so the count must not +// grow with the number of Workers that have come and gone. +const { Worker, isMainThread } = require("node:worker_threads"); + +if (!isMainThread) { + Bun.TOML.parse("a = 1"); + Bun.YAML.parse("a: 1"); + Bun.JSON5.parse("{ a: 1 }"); + Bun.JSONC.parse('{ /* a */ "a": 1 }'); + Bun.XML.parse("1"); + new Bun.Transpiler().transformSync("export const b = 2;"); + await import("data:text/javascript,export default 1"); +} else { + const { heapStats } = require("bun:jsc"); + const liveHeaps = () => heapStats({ dump: true }).mimallocDump.heaps.length; + + const runWorker = () => + new Promise((resolve, reject) => { + const worker = new Worker(__filename); + worker.on("error", reject); + worker.on("exit", code => (code === 0 ? resolve() : reject(new Error(`worker exited with ${code}`)))); + }); + + // Whatever the process sets up lazily for its first Worker is part of the baseline. + await runWorker(); + const before = liveHeaps(); + const workers = 3; + for (let i = 0; i < workers; i++) await runWorker(); + console.log(JSON.stringify({ workers, leaked: liveHeaps() - before })); +} diff --git a/test/js/node/worker_threads/worker_destruction.test.ts b/test/js/node/worker_threads/worker_destruction.test.ts index 3a54646ace91..321b8766b032 100644 --- a/test/js/node/worker_threads/worker_destruction.test.ts +++ b/test/js/node/worker_threads/worker_destruction.test.ts @@ -70,4 +70,19 @@ describe("Worker destruction", () => { expect(stdout).toBe("worker exit 0\n"); expect(exitCode).toBe(0); }); + + // Allocator heaps that APIs park on the Worker's thread (Bun.TOML.parse and friends, the module + // loader) must die with the Worker's VM; see the fixture for details. + test.concurrent("a Worker that used per-thread allocator heaps does not leak them when it exits", async () => { + await using proc = Bun.spawn({ + cmd: [bunExe(), join(import.meta.dir, "worker-heap-leak-fixture.js")], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stderr).toBe(""); + expect(JSON.parse(stdout)).toEqual({ workers: 3, leaked: 0 }); + expect(exitCode).toBe(0); + }); });