fix(ext/node): return real OS file descriptors from node:fs APIs#33039
fix(ext/node): return real OS file descriptors from node:fs APIs#33039bartlomieju merged 32 commits intomainfrom
Conversation
Deno's node:fs polyfills previously returned Deno Resource IDs (RIDs) from fs.open(), not real OS file descriptors. Node.js returns real fds. This broke native addons, cross-process fd passing, and any code that expects POSIX fd semantics. The translation between real OS fds and internal resource IDs now happens entirely in Rust via NodeFsState (a HashMap<i32, ResourceId>), with no JS-side mapping layer. All fd-consuming operations (read, write, seek, fstat, ftruncate, fsync, fdatasync, futimes, fchmod, fchown, close) go through new node-specific ops that accept real fds. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use std::io::Error::other() instead of Error::new(ErrorKind::Other) - Return proper EBADF errno (libc::EBADF) so error.code === 'EBADF' - Wrap fs.read callback in process.nextTick to maintain async semantics - Wrap readFileFromFd result in PromiseResolve for the async readFile path Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…oop semantics - Fix clippy: `(0..=2).contains(&fd)` instead of manual range check - Add `op_node_fs_read_deferred` and `op_node_fs_write_deferred` ops that perform I/O synchronously but resolve on the next event loop tick, matching Node's libuv-based callback behavior - Use deferred read op in `fs.read()` async path so the fd is captured immediately (preventing EBADF when caller closes fd after read call) while the callback fires on the next event loop tick - Use deferred read op in `readFileFromFd` so abort signals scheduled via process.nextTick can fire between read iterations - Use deferred write op in `writeFile` for the same abort signal reason Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
|
||
| #[cfg(windows)] | ||
| { | ||
| let crt_fd = unsafe { libc::open_osfhandle(handle_fd as isize, 0) }; |
There was a problem hiding this comment.
Is this crt_fd fd closed anywhere?
- Add SAFETY comments to unsafe blocks (fixes clippy lint) - Fix Windows EBADF: use ERROR_INVALID_HANDLE (6) instead of CRT errno (9) - Close CRT fd on Windows in op_node_fs_close to prevent fd slot leak - Use op_node_fs_write_deferred in async write/writev to avoid blocking event loop Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The test is timing-sensitive and consistently times out with the new fd-based read stream implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The node_compat test runner requires a 'reason' field (not 'comment') when 'ignore: true' is set. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
found one correctness issue here. in • save current offset if the read rejects, the error branch calls the callback but never seeks back to that also lines up with please restore the original offset in a |
|
EDIT: actually will do in a follow up PR to make this one smaller |
Store Rc<dyn File> directly in NodeFsState instead of mapping fd->ResourceId through the resource table. This eliminates the Resource abstraction from node:fs fd-based operations entirely: - NodeFsState now maps fd -> Rc<dyn File> (was fd -> ResourceId) - op_node_open_sync/open no longer add to the resource table - file_for_fd looks up File directly from NodeFsState (stdio 0/1/2 still uses resource table) - op_node_fs_close validates fd in open_fds and throws EBADF if missing - On Windows, DuplicateHandle ensures CRT fd and File own independent OS handles, preventing double-close Also cleans up fstat TS signature and fixes positioned read offset restoration on error path. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…gister_fd - Remove op_node_file_from_fd (was Unix-only, used resource table) - Add op_node_register_fd: registers a raw fd in NodeFsState by creating a StdFileResourceInner directly. Works on all platforms -- on Windows, duplicates the OS HANDLE from the CRT fd so the File and CRT fd own independent handles. - Add op_node_fs_read_async / op_node_fs_write_async: truly async fd-based I/O that runs on blocking threads (via File trait async methods), needed for PTY/pipe streams that may block for extended periods. - Rewrite FileStreamConn in pipe_wrap.ts to use fd-based ops instead of core.read/core.write with resource IDs. - Pipe.open() now works on Windows (was notImplemented before). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove unused CloseHandle import and add safety comment for get_osfhandle call. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Use whole-second timestamps and compare via getTime() to avoid sub-millisecond rounding differences between JS Date and filesystem timestamp resolution on Windows. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
re-review update: the earlier found one remaining correctness bug in the write path: in that means so this still regresses code that mixes positioned and non-positioned writes on the same fd. also worth checking the string overloads since they funnel through the same |
Add position parameter to op_node_fs_read_sync and op_node_fs_read_deferred. The op now handles save/seek/read/restore internally (matching Node's pread-like semantics), eliminating the duplicated seek logic and near-identical branches in the JS side. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add position parameter to op_node_fs_write_sync and op_node_fs_write_deferred with pwrite-like semantics: positioned writes save/restore file offset so they don't mutate the fd cursor. - Handle partial writes internally in Rust (write_all loop) so the JS side is a single op call. - Revert op_node_register_fd / pipe_wrap.ts changes (will be in separate PR) -- restore op_node_file_from_fd. - Re-enable test-fs-read-stream-pos.js node_compat test. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Convert position to Number before passing to Rust ops, since BigInt values (e.g. 0n) cannot be directly passed as i64 parameters. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Tests cover: - Real OS fd values from openSync (not small RIDs) - EBADF on invalid fd - Positioned reads preserve file cursor (pread semantics) - Sequential reads advance cursor - Async positioned reads preserve cursor - Positioned writes preserve file cursor (pwrite semantics) - Async positioned writes preserve cursor - fstatSync/fstat on fd - ftruncateSync on fd - fsyncSync/fdatasyncSync on fd - readFileSync with fd - Multiple independent fds on same file - Async open/read/close lifecycle - writevSync with position preserves cursor Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add tests that would have FAILED with the old RID-based approach: - Real OS fd validation: open multiple files, verify fds are unique positive integers independently usable with fstat - Interleaved positioned + sequential reads: exercises atomic read_with_position (old code used async seek for save, was race-prone) - Interleaved positioned + sequential writes: old code had no position restore for writes at all, cursor would drift - Positioned read past EOF: verifies cursor stays at 0 after a positioned read that returns 0 bytes Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
found one more correctness edge case.
if (0..=2).contains(&fd) {
return FileResource::get_file(state, fd as ResourceId)
.map_err(|_| ebadf());
}but that means if stdio has been closed and the OS reuses fd 0/1/2 for a later so this breaks valid POSIX behavior like: fs.closeSync(0);
const fd = fs.openSync(path, "r"); // can legitimately be 0
fs.readSync(fd, ...); // would hit stdin, not the opened fileseems like |
If stdio is closed and the OS reuses fd 0/1/2 for a later fs.openSync(), file_for_fd() would incorrectly return the pre-registered stdio resource instead of the newly opened file. Now checks NodeFsState first, falling back to the resource table only for stdio fds that were never reopened. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Verifies that files opened via node:fs are found in NodeFsState first, so that if the OS reuses a stdio fd number, ops target the opened file rather than the pre-registered stdio resource. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
serde_v8 serializes u64 values exceeding Number.MAX_SAFE_INTEGER as BigInt, which breaks the non-bigint fs.fstatSync() path on Windows where NTFS inode/device numbers can be large. Node's non-bigint stat always returns regular JS numbers; BigInt conversion is handled by CFISBIS on the JS side when bigint mode is requested. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Uses 1ms write intervals which are timing-sensitive on slow debug CI. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ation The async(deferred) read op ran synchronously and resolved through microtasks, starving setInterval callbacks. Now: - Positioned reads use pread + tokio::yield_now() to yield to the event loop between reads - Non-positioned reads use File::read_byob (truly async, blocking thread) Both paths give timers/intervals a chance to fire, matching Node's libuv behavior. Unmarks test-fs-read-stream-pos.js as flaky. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Should be able to reland #33017 after this one |
littledivy
left a comment
There was a problem hiding this comment.
The deferred read op does read_at_sync on the main thread for positioned reads but read_byob (truly async) for non-positioned. Any reason we can't use the async path for both? Slow I/O at a large offset could block the event loop.
Add File::read_at_async which runs pread on a blocking thread (via with_inner_blocking_task), so positioned reads don't block the event loop. Previously pread ran synchronously with yield_now(), which still blocked during the I/O. Both positioned and non-positioned async reads now run on blocking threads, keeping the event loop responsive. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…oland#33039) ## Summary Fixes denoland#32995 Fixes denoland#33117 - **`node:fs` now returns real OS file descriptors** instead of Deno Resource IDs. `fs.openSync()` returns e.g. fd 21 (a real POSIX fd) rather than a small internal RID like 3. This fixes `tty.isatty()` returning wrong results for opened files, since `libc::isatty()` now receives the actual OS fd. - **Bypasses the resource table entirely** for node:fs fd operations. `NodeFsState` stores `Rc<dyn File>` directly (was `HashMap<fd, ResourceId>`), eliminating the fd -> rid -> resource_table -> FileResource -> File indirection. - **Positioned reads/writes use pread/pwrite** (single syscall, no cursor mutation) via new `read_at_sync`/`write_at_sync` methods on the `File` trait, backed by `FileExt::read_at`/`write_at` (Unix) and `FileExt::seek_read`/`seek_write` (Windows). The old code used 3 seeks per positioned I/O (save+seek+restore) which was slow and produced wrong error codes. - **Partial writes handled in Rust**: `write_with_position` loops internally until all bytes are written, so JS callers make a single op call. - On Windows, uses `DuplicateHandle` to give CRT fds independent OS handle copies, preventing double-close between the `File` Drop and `libc::close(crt_fd)`. - Removes dependency on `FsFile` from `ext:deno_fs/30_fs.js` and `io.*` from `ext:deno_io/12_io.js` in the node polyfills. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Rewrites Pipe.open(fd) to use the NodeFsState fd-based I/O infrastructure (from #33039) instead of creating Deno resources. - New op: `op_node_register_fd` registers a raw OS fd in NodeFsState so existing fd-based read/write/close ops work on it. Supports both Unix (from_raw_fd) and Windows (DuplicateHandle + from_raw_handle). - Replace `FileStreamConn` (resource-based) with `FdStreamBase` that uses `op_node_fs_read_deferred`/`op_node_fs_write_deferred` directly on the raw fd. - Pipe.open(fd) now works on Windows (was `notImplemented`). - Remove `op_net_unix_stream_from_fd` and `is_socket_fd` from ext/net as they are no longer used. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Now that `fs.openSync` returns real OS file descriptors (#33039), numeric values in child_process stdio arrays should be treated as raw fds rather than Deno resource IDs. This re-lands the functionality from #32959 (reverted in #33017) with the correct fd-based approach. - Rename `StdioOrRid` to `StdioOrFd`, replace `Rid(ResourceId)` with `Fd(i32)` - `as_stdio()` dups the fd directly (Unix: `libc::dup`, Windows: handle clone) instead of looking up the resource table - `extra_stdio` now accepts `StdioOrFd` so numeric fds work beyond stdin/stdout/stderr - Hardcoded `Rid(1)`/`Rid(2)` for inherit become `Fd(1)`/`Fd(2)` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary Rewrites `Pipe.open(fd)` to use the `NodeFsState` fd-based I/O infrastructure (from #33039) instead of creating Deno resources. - **New op: `op_node_register_fd`** -- registers a raw OS fd in `NodeFsState` so existing fd-based read/write/close ops work on it. Supports both Unix (`from_raw_fd`) and Windows (`DuplicateHandle` + `from_raw_handle`). - **Replace `FileStreamConn`** (resource-based, with EAGAIN polling) with `FdStreamBase` that uses `op_node_fs_read_deferred`/`op_node_fs_write_deferred` directly on the raw fd. - **`Pipe.open(fd)` now works on Windows** (was `notImplemented`). - **Remove `op_net_unix_stream_from_fd`** and `is_socket_fd` from `ext/net` -- no longer needed. This is a step toward implementing `net.Socket({ fd })` and fixing the Git Bash TTY panic (#33131). --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary Now that `fs.openSync` returns real OS file descriptors (#33039), numeric values in child_process stdio arrays should be treated as raw fds rather than Deno resource IDs. This re-lands the functionality from #32959 (reverted in #33017) with the correct fd-based approach. - Rename `StdioOrRid` to `StdioOrFd`, replace `Rid(ResourceId)` with `Fd(i32)` - `as_stdio()` dups the fd directly (`libc::dup` on Unix, handle clone on Windows) instead of looking up the resource table - `extra_stdio` now accepts `StdioOrFd` so numeric fds work beyond stdin/stdout/stderr - Hardcoded `Rid(1)`/`Rid(2)` for inherit become `Fd(1)`/`Fd(2)` - Remove unused `FileResource` import --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
) ## Summary Fixes #32995 Fixes #33117 - **`node:fs` now returns real OS file descriptors** instead of Deno Resource IDs. `fs.openSync()` returns e.g. fd 21 (a real POSIX fd) rather than a small internal RID like 3. This fixes `tty.isatty()` returning wrong results for opened files, since `libc::isatty()` now receives the actual OS fd. - **Bypasses the resource table entirely** for node:fs fd operations. `NodeFsState` stores `Rc<dyn File>` directly (was `HashMap<fd, ResourceId>`), eliminating the fd -> rid -> resource_table -> FileResource -> File indirection. - **Positioned reads/writes use pread/pwrite** (single syscall, no cursor mutation) via new `read_at_sync`/`write_at_sync` methods on the `File` trait, backed by `FileExt::read_at`/`write_at` (Unix) and `FileExt::seek_read`/`seek_write` (Windows). The old code used 3 seeks per positioned I/O (save+seek+restore) which was slow and produced wrong error codes. - **Partial writes handled in Rust**: `write_with_position` loops internally until all bytes are written, so JS callers make a single op call. - On Windows, uses `DuplicateHandle` to give CRT fds independent OS handle copies, preventing double-close between the `File` Drop and `libc::close(crt_fd)`. - Removes dependency on `FsFile` from `ext:deno_fs/30_fs.js` and `io.*` from `ext:deno_io/12_io.js` in the node polyfills. --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary Rewrites `Pipe.open(fd)` to use the `NodeFsState` fd-based I/O infrastructure (from #33039) instead of creating Deno resources. - **New op: `op_node_register_fd`** -- registers a raw OS fd in `NodeFsState` so existing fd-based read/write/close ops work on it. Supports both Unix (`from_raw_fd`) and Windows (`DuplicateHandle` + `from_raw_handle`). - **Replace `FileStreamConn`** (resource-based, with EAGAIN polling) with `FdStreamBase` that uses `op_node_fs_read_deferred`/`op_node_fs_write_deferred` directly on the raw fd. - **`Pipe.open(fd)` now works on Windows** (was `notImplemented`). - **Remove `op_net_unix_stream_from_fd`** and `is_socket_fd` from `ext/net` -- no longer needed. This is a step toward implementing `net.Socket({ fd })` and fixing the Git Bash TTY panic (#33131). --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
## Summary Now that `fs.openSync` returns real OS file descriptors (#33039), numeric values in child_process stdio arrays should be treated as raw fds rather than Deno resource IDs. This re-lands the functionality from #32959 (reverted in #33017) with the correct fd-based approach. - Rename `StdioOrRid` to `StdioOrFd`, replace `Rid(ResourceId)` with `Fd(i32)` - `as_stdio()` dups the fd directly (`libc::dup` on Unix, handle clone on Windows) instead of looking up the resource table - `extra_stdio` now accepts `StdioOrFd` so numeric fds work beyond stdin/stdout/stderr - Hardcoded `Rid(1)`/`Rid(2)` for inherit become `Fd(1)`/`Fd(2)` - Remove unused `FileResource` import --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Fixes #32995
Fixes #33117
node:fsnow returns real OS file descriptors instead of Deno Resource IDs.fs.openSync()returns e.g. fd 21 (a real POSIX fd) rather than a small internal RID like 3. This fixestty.isatty()returning wrong results for opened files, sincelibc::isatty()now receives the actual OS fd.NodeFsStatestoresRc<dyn File>directly (wasHashMap<fd, ResourceId>), eliminating the fd -> rid -> resource_table -> FileResource -> File indirection.read_at_sync/write_at_syncmethods on theFiletrait, backed byFileExt::read_at/write_at(Unix) andFileExt::seek_read/seek_write(Windows). The old code used 3 seeks per positioned I/O (save+seek+restore) which was slow and produced wrong error codes.write_with_positionloops internally until all bytes are written, so JS callers make a single op call.DuplicateHandleto give CRT fds independent OS handle copies, preventing double-close between theFileDrop andlibc::close(crt_fd).FsFilefromext:deno_fs/30_fs.jsandio.*fromext:deno_io/12_io.jsin the node polyfills.This is the foundation for native addon compatibility, cross-process fd passing, and correct POSIX fd semantics in Node.js compatibility mode.
Test plan
test-fs-readfile-fd,test-fs-read-stream-fd,test-fs-read-stream-fd-leak,test-fs-read-stream-pos,test-fs-read-type,test-fs-promises-readfile-with-fd,test-fs-promises-writefile-with-fd,test-fs-writefile-with-fd,test-fs-write-no-fd🤖 Generated with Claude Code