fix(fs): openSync dropped O_NONBLOCK, and readSync reported errors as EOF#6403
Conversation
… EOF
Two defects that together make a non-blocking read impossible.
`apply_numeric_open_flags` translated only O_ACCMODE/O_CREAT/O_EXCL/O_TRUNC/
O_APPEND into Rust's `OpenOptions` and silently discarded everything else, so
O_NONBLOCK never reached open(2). The descriptor came back blocking and
const fd = openSync("/dev/tty", O_RDONLY | O_NONBLOCK);
readSync(fd, buf, 0, 1024, null); // Node: throws EAGAIN
parked forever instead of raising EAGAIN. Forward the flags `OpenOptions` does
not model via `custom_flags` — Node passes the flag word to open(2) verbatim.
(std masks O_ACCMODE out of custom flags, so the access mode cannot be clobbered.)
`js_fs_read_sync` then mapped every read error to `0.0`, which a caller cannot
distinguish from a clean end-of-file. Thread the `io::Error` out of the read core
so the sync surface throws the Node-shaped error (EAGAIN, EISDIR, EIO…) and the
callback surface reports it as `cb(err)` — the callback form must not throw.
📝 WalkthroughWalkthroughFilesystem open flags now preserve unmodeled Unix bits, read operations retain syscall errors, callbacks dispatch those errors as filesystem values, and a FIFO regression test validates nonblocking ChangesFilesystem read error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/issue_fs_nonblocking_read.js`:
- Around line 14-38: Wrap the temporary FIFO test flow beginning after
mkdtempSync in a try/finally block, and call rmSync on the created dir in
finally so cleanup occurs even when assertions or reads fail. Keep the existing
readSync error validation and closeSync behavior intact.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2f52e028-7f52-4bda-a593-eb463c3533ed
📒 Files selected for processing (4)
crates/perry-runtime/src/fs/callbacks.rscrates/perry-runtime/src/fs/fd_ops.rstests/issue_fs_nonblocking_read.jstests/test_fs_nonblocking_read.sh
The fixture created a mkdtemp directory and an fd and freed neither on the throwing path, so a failing run leaked a temp directory (and an open descriptor) on every retry. Wrap both in `finally`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/issue_fs_nonblocking_read.js`:
- Around line 31-41: Restructure the try/catch around readSync in the test so
the explicit “readSync resolved instead of raising EAGAIN” failure is thrown
after the catch block, not from inside it. Preserve assigning the caught system
error’s code to code and keep the existing EAGAIN validation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: af3b851c-80ba-44c1-93f6-a4bf64827163
📒 Files selected for processing (1)
tests/issue_fs_nonblocking_read.js
| let code = null; | ||
| try { | ||
| readSync(fd, buf, 0, 16, null); | ||
| throw new Error("readSync resolved instead of raising EAGAIN"); | ||
| } catch (err) { | ||
| code = err.code; | ||
| } | ||
|
|
||
| if (code !== "EAGAIN") { | ||
| throw new Error(`readSync on an empty non-blocking FIFO reported ${code}, expected EAGAIN`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve the intended test failure message.
Currently, if readSync successfully resolves instead of throwing, the script intentionally throws new Error("readSync resolved..."). However, the catch block immediately intercepts this error. Because standard error objects don't have a .code property, code is assigned undefined. The script then throws the generic second error (readSync on an empty non-blocking FIFO reported undefined, expected EAGAIN), and the original, more descriptive failure message is completely lost.
To avoid catching the test's own failure error, move the explicit throw outside the catch block.
💚 Proposed fix to properly report test failures
let code = null;
try {
readSync(fd, buf, 0, 16, null);
- throw new Error("readSync resolved instead of raising EAGAIN");
} catch (err) {
code = err.code;
}
+ if (code === null) {
+ throw new Error("readSync resolved instead of raising EAGAIN");
+ }
if (code !== "EAGAIN") {
throw new Error(`readSync on an empty non-blocking FIFO reported ${code}, expected EAGAIN`);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let code = null; | |
| try { | |
| readSync(fd, buf, 0, 16, null); | |
| throw new Error("readSync resolved instead of raising EAGAIN"); | |
| } catch (err) { | |
| code = err.code; | |
| } | |
| if (code !== "EAGAIN") { | |
| throw new Error(`readSync on an empty non-blocking FIFO reported ${code}, expected EAGAIN`); | |
| } | |
| let code = null; | |
| try { | |
| readSync(fd, buf, 0, 16, null); | |
| } catch (err) { | |
| code = err.code; | |
| } | |
| if (code === null) { | |
| throw new Error("readSync resolved instead of raising EAGAIN"); | |
| } | |
| if (code !== "EAGAIN") { | |
| throw new Error(`readSync on an empty non-blocking FIFO reported ${code}, expected EAGAIN`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/issue_fs_nonblocking_read.js` around lines 31 - 41, Restructure the
try/catch around readSync in the test so the explicit “readSync resolved instead
of raising EAGAIN” failure is thrown after the catch block, not from inside it.
Preserve assigning the caught system error’s code to code and keep the existing
EAGAIN validation unchanged.
|
The Its scratch directory is The giveaway: this PR failed one run and passed the next on the same commit. Running 8 copies of the binary concurrently reproduces it — 2 of 8 fail before the fix, 0 of 8 after. Fix is up as #6410 ( |
1 participant
The bugs
Two defects that compound: a non-blocking read is impossible, and a failing read
is indistinguishable from EOF.
1.
openSyncdiscards every flag it does not model.Node hands the flag word to open(2) verbatim. Perry rebuilds it from the handful
of bits
OpenOptionsmodels, soO_NONBLOCKnever reaches the syscall:2.
readSyncswallows the syscall error.0means end-of-file to every caller, so a failure is silently reported as aclean EOF — and with flag #1 fixed, a non-blocking read with no data would report
EOF instead of EAGAIN.
The fix
OpenOptions::custom_flags. std masksO_ACCMODEout of custom flags, so the access mode cannot be clobbered.io::Errorout of aread_sync_resultcore.fs.readSyncthrowsthe Node-shaped error (
code/errno/syscall), and the callback forms(
fs.read(fd, …, cb)/ options form) report it ascb(err)rather thanthrowing, which is what Node does.
Test
tests/issue_fs_nonblocking_read.js+tests/test_fs_nonblocking_read.sh: open awriter-less FIFO with
O_RDONLY | O_NONBLOCKand assertreadSyncraisesEAGAIN. The harness caps the run, so the old behaviour surfaces as a failurerather than a hung suite.
Summary by CodeRabbit
Bug Fixes
readSyncand callback-based reads to preserve and surface underlying syscall errors (no longer collapse failures into EOF/0-like results).openflag handling to preserve nonblocking and other non-modeled flags, reducing the risk of hangs.Tests
EAGAINand do not hang.