diff --git a/test/expectations.txt b/test/expectations.txt index 0e2990c51be7..8c6966380941 100644 --- a/test/expectations.txt +++ b/test/expectations.txt @@ -85,7 +85,6 @@ test/js/bun/spawn/spawn-maxbuf.test.ts [ FLAKY ] # Tests failed due to memory leaks [ ASAN ] test/js/node/url/pathToFileURL.test.ts [ LEAK ] # pathToFileURL doesn't leak memory -[ ASAN ] test/js/node/fs/abort-signal-leak-read-write-file.test.ts [ LEAK ] # should not leak memory with already aborted signals [ ASAN ] test/js/web/streams/streams-leak.test.ts [ LEAK ] # Absolute memory usage remains relatively constant when reading and writing to a pipe [ ASAN ] test/cli/run/require-cache.test.ts [ LEAK ] # files transpiled and loaded don't leak file paths > via require() diff --git a/test/js/node/fs/abort-signal-leak-read-write-file-fixture.ts b/test/js/node/fs/abort-signal-leak-read-write-file-fixture.ts deleted file mode 100644 index abba695ee60f..000000000000 --- a/test/js/node/fs/abort-signal-leak-read-write-file-fixture.ts +++ /dev/null @@ -1,41 +0,0 @@ -import fs from "fs"; -import { join } from "path"; -import { isASAN, tmpdirSync } from "harness"; -import { heapStats } from "bun:jsc"; - -const tmpdir = tmpdirSync(); - -for (let i = 0; i < 100_000; i++) { - try { - const signal = AbortSignal.abort(); - await fs.promises.readFile("blah", { signal }); - } catch (e) {} - try { - const signal = AbortSignal.abort(); - await fs.promises.writeFile("blah", "blah", { signal }); - } catch (e) {} - - // aborting later does not leak in writeFile - const controller = new AbortController(); - const signal = controller.signal; - const prom = fs.promises.writeFile(join(tmpdir, "blah"), "blah", { signal }); - process.nextTick(() => controller.abort()); - try { - await prom; - } catch (e) {} -} - -Bun.gc(true); - -const numAbortSignalObjects = heapStats().objectTypeCounts.AbortSignal; -if (numAbortSignalObjects > 10) { - throw new Error(`AbortSignal objects > 10, received ${numAbortSignalObjects}`); -} - -// ASAN's quarantine retains freed allocations (default 256 MB) and shadow -// memory raises the absolute RSS floor; widen the cap to avoid false positives. -const limitMB = isASAN ? 700 : 200; -const rss = (process.memoryUsage().rss / 1024 / 1024) | 0; -if (rss > limitMB) { - throw new Error(`Memory leak detected: ${rss} MB, expected < ${limitMB} MB`); -} diff --git a/test/js/node/fs/abort-signal-leak-read-write-file.test.ts b/test/js/node/fs/abort-signal-leak-read-write-file.test.ts index 6133e17ee005..07872b0d4bf2 100644 --- a/test/js/node/fs/abort-signal-leak-read-write-file.test.ts +++ b/test/js/node/fs/abort-signal-leak-read-write-file.test.ts @@ -1,6 +1,48 @@ +import { heapStats } from "bun:jsc"; import { expect, test } from "bun:test"; -import path from "path"; +import fs from "fs"; +import { expectMaxObjectTypeCount, isASAN, isDebug, tempDir } from "harness"; +import { join } from "path"; -test("should not leak memory with already aborted signals", async () => { - expect([path.join(import.meta.dir, "abort-signal-leak-read-write-file-fixture.ts")]).toRun(); +// https://github.com/oven-sh/bun/pull/16788 +test("fs.promises readFile/writeFile does not leak AbortSignal", async () => { + using dir = tempDir("fs-abort-signal-leak", { blah: "blah" }); + const target = join(String(dir), "blah"); + const noop = () => {}; + + // pre-aborted signal: readFile/writeFile reject immediately and must release + // the signal reference on the early-return path. + await expect(fs.promises.readFile(target, { signal: AbortSignal.abort() })).rejects.toMatchObject({ + name: "AbortError", + }); + await expect(fs.promises.writeFile(target, "blah", { signal: AbortSignal.abort() })).rejects.toMatchObject({ + name: "AbortError", + }); + + // abort after the write has been scheduled: the async completion path must + // also release the signal reference. The write may legitimately finish before + // nextTick fires, so either resolution or AbortError is acceptable here. + { + const controller = new AbortController(); + const prom = fs.promises.writeFile(target, "blah", { signal: controller.signal }); + process.nextTick(() => controller.abort()); + await prom.catch(e => expect(e).toMatchObject({ name: "AbortError" })); + } + + // Leak detector. A JSAbortSignal wrapper is only pinned by hasPendingActivity() + // when the signal is NOT aborted AND has an abort listener (see + // JSAbortSignalCustom.cpp), so the loop below uses a never-aborted signal with + // a no-op listener: if readFile/writeFile forget to drop the pending-activity + // ref they take, every one of these wrappers survives GC and the object count + // below climbs with ITERATIONS instead of staying flat. + const baseline = heapStats().objectTypeCounts.AbortSignal ?? 0; + const ITERATIONS = isDebug || isASAN ? 100 : 2_000; + for (let i = 0; i < ITERATIONS; i++) { + const controller = new AbortController(); + controller.signal.addEventListener("abort", noop); + await fs.promises.writeFile(target, "blah", { signal: controller.signal }); + await fs.promises.readFile(target, { signal: controller.signal }); + } + + await expectMaxObjectTypeCount(expect, "AbortSignal", baseline + 10); });