Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 40 additions & 2 deletions src/js/node/fs.promises.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,26 @@ async function opendir(dir: string, options) {
return promise;
}

// Node.js closes a FileHandle's fd in its native finalizer and raises
// ERR_INVALID_STATE (DEP0137 end-of-life) when collected without close().
// Mirror that with a FinalizationRegistry so dropped handles don't leak fds.
let fileHandleRegistry: FinalizationRegistry<{ fd: number; path: string | undefined }> | undefined;
function onFileHandleCollected(held: { fd: number; path: string | undefined }) {
try {
fs.closeSync(held.fd);
} catch {}
Comment thread
robobun marked this conversation as resolved.
const suffix = held.path !== undefined ? ` (${held.path})` : "";
const err: NodeJS.ErrnoException = new Error(
"A FileHandle object was closed during garbage collection. This used to be allowed " +
"with a deprecation warning but is now considered an error. Please close FileHandle " +
`objects explicitly. File descriptor: ${held.fd}${suffix}`,
);
err.code = "ERR_INVALID_STATE";
Comment thread
robobun marked this conversation as resolved.
process.nextTick(() => {
throw err;
});
}

const private_symbols = {
kRef,
kUnref,
Expand Down Expand Up @@ -264,7 +284,11 @@ const exports = {
},
statfs: asyncWrap(fs.statfs, "statfs"),
open: async (path, flags = "r", mode = 0o666) => {
return new private_symbols.FileHandle(await fs.open(path, flags, mode), flags);
// Snapshot the path as a string before the fd is opened so a throwing
// Buffer/URL toString cannot leak the fd, and the registry never retains
// the caller's object.
const pathForDiag = typeof path === "string" ? path : path == null ? undefined : String(path);
return new private_symbols.FileHandle(await fs.open(path, flags, mode), flags, pathForDiag);
},
read: asyncWrap(fs.read, "read"),
write: asyncWrap(fs.write, "write"),
Expand Down Expand Up @@ -383,12 +407,15 @@ function asyncWrap(fn: any, name: string) {
// These functions await the result so that errors propagate correctly with
// async stack traces and so that the ref counting is correct.
class FileHandle extends EventEmitter {
constructor(fd, flag) {
constructor(fd, flag, path?: string) {
super();
this[kFd] = fd ? fd : -1;
this[kRefs] = 1;
this[kClosePromise] = null;
this[kFlag] = flag;
if (this[kFd] !== -1) {
(fileHandleRegistry ??= new FinalizationRegistry(onFileHandleCollected)).register(this, { fd, path }, this);
}
}

getAsyncId() {
Expand Down Expand Up @@ -671,6 +698,8 @@ function asyncWrap(fn: any, name: string) {
return this[kClosePromise];
}

fileHandleRegistry?.unregister(this);

if (--this[kRefs] === 0) {
this[kFd] = -1;
this[kClosePromise] = PromisePrototypeFinally.$call(close(fd), () => {
Expand Down Expand Up @@ -1357,6 +1386,7 @@ function asyncWrap(fn: any, name: string) {
}
const fd = this[kFd];
this[kFd] = -1;
fileHandleRegistry?.unregister(this);
(nodeFsForIter ??= require("node:fs")).closeSync(fd);
this.emit("close");
}
Expand All @@ -1369,6 +1399,7 @@ function asyncWrap(fn: any, name: string) {
const fd = this[kFd];
const flag = this[kFlag];
this[kFd] = -1;
fileHandleRegistry?.unregister(this);
return {
data: { fd, flag },
deserializeInfo: "internal/fs/promises:FileHandle",
Expand All @@ -1382,6 +1413,13 @@ function asyncWrap(fn: any, name: string) {
[kDeserialize]({ fd, flag }) {
this[kFd] = fd;
this[kFlag] = flag;
if (fd !== -1) {
(fileHandleRegistry ??= new FinalizationRegistry(onFileHandleCollected)).register(
this,
{ fd, path: undefined },
this,
);
}
}

[kRef]() {
Expand Down
2 changes: 1 addition & 1 deletion test/cli/install/bun-lock.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ it("should write plaintext lockfiles", async () => {
await access(join(packageDir, "bun.lock"));

// Assert that the lockfile has the correct permissions
const file = await open(join(packageDir, "bun.lock"), "r");
await using file = await open(join(packageDir, "bun.lock"), "r");
const stat = await file.stat();

// in unix, 0o644 == 33188
Expand Down
2 changes: 1 addition & 1 deletion test/cli/install/bun-lockb.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ it("should not print anything to stderr when running bun.lockb", async () => {
expect(await exists(join(packageDir, "bun.lockb"))).toBe(true);

// Assert that the lockfile has the correct permissions
const file = await open(join(packageDir, "bun.lockb"), "r");
await using file = await open(join(packageDir, "bun.lockb"), "r");
const stat = await file.stat();

// in unix, 0o755 == 33261
Expand Down
3 changes: 1 addition & 2 deletions test/js/bun/util/bun-file.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ test("writer.end() should not close the fd if it does not own the fd", async ()
const fd = fileHandle.fd;

await Bun.file(fd).writer().end();
// @ts-ignore
await fsPromises.close(fd);
await fileHandle.close();
expect(await Bun.file(filename).text()).toBe("");
}
});
Expand Down
72 changes: 72 additions & 0 deletions test/js/node/fs/fs.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,78 @@ describe("FileHandle", () => {

expect(readFileSync(path, "utf8")).toBe("Test file written successfully");
});

// Node.js closes a FileHandle's fd in its native finalizer and raises
// ERR_INVALID_STATE (DEP0137 end-of-life) when the handle is collected
// without close(). Bun must reclaim the fd and surface the same diagnostic.
it.concurrent.skipIf(isWindows)(
"FileHandle collected without close() closes the fd and raises ERR_INVALID_STATE",
async () => {
const fixture = /* js */ `
const fsp = require("node:fs/promises");
const fs = require("node:fs");
const os = require("node:os");
const path = require("node:path");
const fdDir = process.platform === "darwin" ? "/dev/fd" : "/proc/self/fd";
const nfds = () => fs.readdirSync(fdDir).length;
const dir = fs.mkdtempSync(path.join(os.tmpdir(), "fh-gc-"));
const N = 50;
const diags = [];
process.on("uncaughtException", e => diags.push({ code: e.code, message: e.message }));

(async () => {
const before = nfds();
await (async () => {
for (let i = 0; i < N; i++) await fsp.open(path.join(dir, "f" + i), "w");
})();
// force GC until every leaked fd is reclaimed and every diagnostic lands
for (let i = 0; i < 40 && (nfds() - before > 0 || diags.length < N); i++) {
Bun.gc(true);
await new Promise(r => setTimeout(r, 25));
}
const afterGC = nfds();
const sample = diags[0] ?? {};

// properly closed handles must not trip the finalizer
const marker = diags.length;
await (async () => {
for (let i = 0; i < N; i++) await (await fsp.open(path.join(dir, "g" + i), "w")).close();
})();
for (let i = 0; i < 10; i++) {
Bun.gc(true);
await new Promise(r => setTimeout(r, 25));
}

console.log(JSON.stringify({
leakedAfterGC: afterGC - before,
diagCount: diags.length,
sampleCode: sample.code,
sampleHasFd: typeof sample.message === "string" && sample.message.includes("File descriptor: "),
falsePositives: diags.length - marker,
}));
fs.rmSync(dir, { recursive: true, force: true });
})();
`;
await using proc = Bun.spawn({
cmd: [bunExe(), "-e", fixture],
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ result: JSON.parse(stdout.trim()), stderr, exitCode }).toEqual({
result: {
leakedAfterGC: 0,
diagCount: 50,
sampleCode: "ERR_INVALID_STATE",
sampleHasFd: true,
falsePositives: 0,
},
stderr: expect.not.stringContaining("error"),
exitCode: 0,
});
},
);
});

it("fdatasyncSync", () => {
Expand Down
4 changes: 2 additions & 2 deletions test/js/node/fs/promises.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -120,11 +120,11 @@ describe("access", () => {

describe("open", () => {
it("should work", async () => {
await open(__filename);
await using _ = await open(__filename);
});

it("should return an object", async () => {
const fh = await open(__filename);
await using fh = await open(__filename);
assert.strictEqual(typeof fh, "object");
assert.strictEqual(typeof fh.fd, "number");
});
Expand Down
4 changes: 4 additions & 0 deletions test/js/node/test/parallel/test-whatwg-readablebytestream.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,10 @@ class Source {
this.controller = controller;
}

async cancel() {
await this.file.close();
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

async pull(controller) {
const byobRequest = controller.byobRequest;
assert.match(inspect(byobRequest), /ReadableStreamBYOBRequest/);
Expand Down
Loading