Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
2 changes: 2 additions & 0 deletions src/event_loop/ConcurrentTask.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ pub mod task_tag {
NativeZlib,
NativeZstd,
Open,
Opendir,
Fdreaddir,
PollPendingModulesTask,
PosixSignalTask,
MemoryPressureTask,
Expand Down
115 changes: 44 additions & 71 deletions src/js/node/fs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -600,23 +600,20 @@
return require("internal/fs/watch").watch(path, options, listener);
},
opendir = function opendir(path, options, callback) {
// TODO: validatePath
// validateString(path, "path");
if (typeof options === "function") {
callback = options;
options = undefined;
}
validateFunction(callback, "callback");
// Argument validation errors throw synchronously (node does the same);
// the eager path check runs on an async stat so the JS thread isn't
// blocked and the callback never fires synchronously.
const result = new Dir(1, path, options, kAlreadyValidated);
// construct the Dir up front so options validation runs before the open.
const result = new Dir(-2, path, options);
// Invoke the callback from process.nextTick so an exception thrown by it
// surfaces as an uncaught exception instead of rejecting this internal
// promise chain (same convention as glob() below).
fs.stat(path).then(
onOpendirStatFulfilled.bind(null, callback, path, result),
onOpendirStatRejected.bind(null, callback, path),
fs.opendir(path).then(
onOpendirFulfilled.bind(null, callback, result),
onOpendirRejected.bind(null, callback),
);
};

Expand Down Expand Up @@ -1005,15 +1002,12 @@
throw $ERR_INVALID_ARG_TYPE(name, "number or Date", time);
}

function onOpendirStatFulfilled(callback, path, result, stats) {
if (!stats.isDirectory()) {
process.nextTick(callback, opendirNotDirError(path));
return;
}
function onOpendirFulfilled(callback, result, fd) {
dirSetHandle(result, fd);
process.nextTick(callback, null, result);
}
function onOpendirStatRejected(callback, path, err) {
process.nextTick(callback, typeof err?.errno === "number" ? opendirStatError(err, path) : err);
function onOpendirRejected(callback, err) {
process.nextTick(callback, err);
}
function callOnceWithNull(callback) {
callback(null);
Expand All @@ -1023,49 +1017,32 @@
}

function opendirSync(path, options) {
// TODO: validatePath
// validateString(path, "path");
return new Dir(1, path, options);
const result = new Dir(-2, path, options);
dirSetHandle(result, fs.opendirSync(path));
return result;
}

// Reshape a stat error as node's eager opendir error. Stat errors arrive as
// "ECODE: <description>, stat '<path>'"; pull out just the description before
// re-prefixing (avoids "EACCES: EACCES: ...").
function opendirStatError(err, path) {
err.syscall = "opendir";
const description = err.message.replace(/^[A-Z]+: /, "").replace(/, l?stat '.*'$/, "");
err.message = `${err.code}: ${description}, opendir '${path}'`;
return err;
}

function opendirNotDirError(path) {
const err = new Error(`ENOTDIR: not a directory, opendir '${path}'`);
err.code = "ENOTDIR";
// libuv's UV_ENOTDIR: -ENOTDIR on POSIX, -4052 on Windows
err.errno = process.platform === "win32" ? -4052 : -20;
err.syscall = "opendir";
err.path = path;
return err;
}

// Passed as the Dir constructor's 4th argument by the async opendir paths,
// which run the eager path check with an async stat instead.
const kAlreadyValidated = Symbol("kAlreadyValidated");
let dirSetHandle;

class Dir {
/**
* `-1` when closed. stdio handles (0, 1, 2) don't actually get closed by
* {@link close} or {@link closeSync}.
* The directory fd opened by `fs.opendir`. `-1` once closed; `-2` between
* option validation and the native open completing (async `opendir`).
Comment thread
robobun marked this conversation as resolved.
Outdated
*/
#handle: number;

Check failure on line 1032 in src/js/node/fs.ts

View check run for this annotation

Claude / Claude Code Review

Stale 'handle > 2' guard leaks directory fds 0/1/2

The `if (handle > 2)` guards in `#closeOp()` and `closeSync()` are stale now that `#handle` is a real fd from `openat(O_DIRECTORY)`: if any of stdin/stdout/stderr is closed (common daemonization pattern), `openat()` returns fd 0/1/2 and `Dir.close()` will silently skip `fs.closeSync(handle)`, leaking the directory fd. The `< 0` check already handles the `-1`/`-2` sentinels, so both `> 2` guards should be dropped (the PR removed the doc comment that justified them under the old sentinel semantics
Comment thread
robobun marked this conversation as resolved.
Outdated
#path: PathLike;
#options;
#entries: DirentType[] | null = null;
#entriesIdx = 0;

constructor(handle, path: PathLike, options, validated?) {
static {
dirSetHandle = (dir: Dir, fd: number) => {
dir.#handle = fd;
};
}

Check failure on line 1042 in src/js/node/fs.ts

View check run for this annotation

Claude / Claude Code Review

Dir now owns a real fd but has no GC-time release

`Dir` now owns a real directory fd (previously `#handle` was the placeholder `1` and close was a no-op), but it's a pure JS class with no `FinalizationRegistry` hook — a `Dir` that is garbage-collected without `close()`/`closeSync()` permanently leaks the fd, which is a regression for existing code that abandons handles. Mirror the `FileHandle` pattern in `src/js/node/fs.promises.ts` (`fileHandleRegistry` / `onFileHandleCollected`): register in `dirSetHandle` and unregister in `#closeOp`/`closeS
Comment thread
robobun marked this conversation as resolved.

constructor(handle, path: PathLike, options) {
if ($isUndefinedOrNull(handle)) throw $ERR_MISSING_ARGS("handle");
validateInteger(handle, "handle", 0);
if (options != null && typeof options !== "object" && typeof options !== "string") {
throw $ERR_INVALID_ARG_TYPE("options", "object", options);
}
Expand All @@ -1078,20 +1055,7 @@
if (options?.bufferSize !== undefined) {
validateInteger(options.bufferSize, "options.bufferSize", 1);
}
if (handle === 1 && validated !== kAlreadyValidated) {
// node's opendir opens the directory eagerly and reports ENOTDIR/ENOENT
let stats;
try {
stats = fs.statSync(path);
} catch (err: any) {
if (typeof err?.errno !== "number") throw err; // argument validation errors throw as-is
throw opendirStatError(err, path);
}
if (!stats.isDirectory()) {
throw opendirNotDirError(path);
}
}
this.#handle = $toLength(handle);
this.#handle = handle;
this.#path = path;
this.#options = options;
}
Expand Down Expand Up @@ -1131,11 +1095,16 @@
if (this.#handle < 0) throw $ERR_DIR_CLOSED();
if (this.#pendingCount > 0) throw this.#dirConcurrentError();

let entries = (this.#entries ??= fs.readdirSync(this.#path, {
withFileTypes: true,
encoding: this.#options?.encoding,
recursive: this.#options?.recursive,
}));
let entries = (this.#entries ??= this.#options?.recursive
? fs.readdirSync(this.#path, {
withFileTypes: true,
encoding: this.#options?.encoding,
recursive: true,
})
: fs.fdreaddirSync(this.#handle, this.#path, {
withFileTypes: true,
encoding: this.#options?.encoding,
}));
return this.#entriesIdx < entries.length ? entries[this.#entriesIdx++] : null;
}

Expand All @@ -1158,13 +1127,17 @@
if (this.#handle < 0) throw $ERR_DIR_CLOSED();
const entries = this.#entries;
if (entries) return this.#entriesIdx < entries.length ? entries[this.#entriesIdx++] : null;
return fs
.readdir(this.#path, {
withFileTypes: true,
encoding: this.#options?.encoding,
recursive: this.#options?.recursive,
})
.then(this.#onReaddir.bind(this));
const p = this.#options?.recursive
? fs.readdir(this.#path, {
withFileTypes: true,
encoding: this.#options?.encoding,
recursive: true,
})
: fs.fdreaddir(this.#handle, this.#path, {
withFileTypes: true,
encoding: this.#options?.encoding,
});
return p.then(this.#onReaddir.bind(this));
}

#onReaddir(entries) {
Expand Down
7 changes: 4 additions & 3 deletions src/runtime/dispatch.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ use bun_jsc::event_loop::{EventLoop, JsTerminated};
use bun_jsc::task::report_error_or_terminate;
use bun_jsc::virtual_machine::VirtualMachine;

/// X-macro: the 42 `node:fs` async ops dispatched via `run_from_js_thread`.
/// X-macro: the 44 `node:fs` async ops dispatched via `run_from_js_thread`.
Comment thread
robobun marked this conversation as resolved.
Outdated
///
/// Row shape: `$tag $ty;` — `$tag` is the `bun_event_loop::task_tag::*` const,
/// `$ty` is the `fs_async::*` alias. They differ in exactly three rows
Expand All @@ -64,7 +64,8 @@ macro_rules! for_each_fs_async_op {
RealpathNonNative RealpathNonNative; Mkdir Mkdir; Fsync Fsync;
Fdatasync Fdatasync; Access Access; AppendFile AppendFile;
Mkdtemp Mkdtemp; Exists Exists; Futimes Futimes; Lchmod Lchmod;
Lchown Lchown; Unlink Unlink; StatFS Statfs;
Lchown Lchown; Unlink Unlink; StatFS Statfs; Opendir Opendir;
Fdreaddir Fdreaddir;
}
};
}
Expand Down Expand Up @@ -584,7 +585,7 @@ fn run_task_cold(task: Task) {
/// Compile-time guard that the arm count above tracks
/// `bun_event_loop::task_tag::COUNT`. Bump when adding a variant.
const _: () = assert!(
task_tag::COUNT == 97,
task_tag::COUNT == 99,
"dispatch::run_task arm count out of sync with bun_event_loop::task_tag",
);

Expand Down
25 changes: 17 additions & 8 deletions src/runtime/node/dir_iterator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,10 +162,15 @@ mod platform {
self.received_eof = true;
return Ok(None);
}
return Err(sys::Error::from_code_int(
sys::last_errno(),
Tag::getdirentries64,
));
let e = sys::last_errno();
// __getdirentries64 can fail with ENOENT when the open
// directory has been rmdir'd. POSIX requires treating
// this as EOF (matches glibc readdir() / node).
Comment thread
robobun marked this conversation as resolved.
Outdated
if e == libc::ENOENT {
self.received_eof = true;
return Ok(None);
}
return Err(sys::Error::from_code_int(e, Tag::getdirentries64));
}

self.index = 0;
Expand Down Expand Up @@ -369,10 +374,14 @@ mod platform {
)
};
if rc < 0 {
return Err(sys::Error::from_code_int(
sys::last_errno(),
Tag::getdents64,
));
let e = sys::last_errno();
// getdents64 fails with ENOENT when the open directory
// has been rmdir'd. POSIX requires treating this as EOF;
// glibc's readdir() does the same, so node sees EOF here.
Comment thread
robobun marked this conversation as resolved.
Outdated
if e == libc::ENOENT {
return Ok(None);
}
return Err(sys::Error::from_code_int(e, Tag::getdents64));
}
if rc == 0 {
return Ok(None);
Expand Down
4 changes: 4 additions & 0 deletions src/runtime/node/node.classes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,10 @@ export default [
mkdtempSync: { async: false, fn: "mkdtempSync", length: 2 },
open: { async: true, fn: "open", length: 4 },
openSync: { async: false, fn: "openSync", length: 3 },
opendir: { async: true, fn: "opendir", length: 1 },
opendirSync: { async: false, fn: "opendirSync", length: 1 },
fdreaddir: { async: true, fn: "fdreaddir", length: 3 },
fdreaddirSync: { async: false, fn: "fdreaddirSync", length: 3 },
readdir: { async: true, fn: "readdir", length: 3 },
readdirSync: { async: false, fn: "readdirSync", length: 2 },
read: { async: true, fn: "read", length: 6 },
Expand Down
Loading
Loading