Repository navigation
node:fs: accept any object with aborted as options.signal in readFile, writeFile and appendFile - #48124
Conversation
…File, writeFile and appendFile
`args::signal_option` accepts what Node's `validateAbortSignal` accepts.
After its class test fails, `validators::validate_abort_signal` asks the
rule that the JS-visible `validateAbortSignal` uses. `undefined`, `null`
and `""` stay absent.
An async call keeps a signal that is no `AbortSignal` GC-protected in
`args::FileSignal`. The JS thread asks it at the two points where it asks
an `AbortSignal`: before the task is scheduled and when it completes.
`validateAbortSignal` now throws Node's sentence ("must be an instance of
AbortSignal") and refuses a function that has `aborted`, for every caller.
`FileHandle#appendFile` forwards `signal`. `fs.promises.writeFile` checks
it for iterable data, after the encoding. The `AbortError` for a signal
has an own `cause`, also for a reason that is `undefined`.
|
Status
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Beyond the inline findings, a few things were checked and ruled out: the switch to ERR::INVALID_ARG_INSTANCE in NodeValidatorBodies.h keeps the ERR_INVALID_ARG_TYPE code (only the "an instance of" wording changes, which is how Node formats a capitalized expected type); the FileSignal::Native pending-activity ref is released by its Drop on every early-return path of the three from_js parsers, so the removed scopeguards do not unbalance it; and removing the undefined-cause branch from Bun__wrapAbortError only affects callers that pass undefined (the h2 caller passes a real reason, and Node's AbortError installs an own cause: undefined too).
Extended reasoning...
The change touches the native node:fs argument parsers (node_fs.rs, node_fs_binding.rs), the shared C++ validateAbortSignal, the AbortError wrapper, fs.promises.ts and a vendored Node test file; it touches no auth, crypto or injection surface. Five verified findings are posted inline, including a Node-compat divergence for FileHandle#appendFile and a type-check failure in fs.promises.ts, so a human needs to weigh them; this note only records the adjacent concerns that were examined and found not to be issues.
Findings marked 🟡 are optional suggestions and need no follow-up push.
…re it is typed `signal === ""` compared a variable of type `AbortSignal | null` with a string. The type check of the built-in modules refuses that comparison.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Commit 77f22a0 resolves the AbortSignal | null vs "" comparison flagged on the previous push (the untyped given is compared now, so the built-in module type check is satisfied).
Beyond the inline note, I also checked: the refcount pairing after the Drop impls and scopeguards were replaced — FileSignal::drop releases the pending activity and AbortSignalRef (ExternalShared) releases the signal ref itself, so parse-error, success and abort paths stay balanced; the Bun__wrapAbortError change also reaches h2_frame_parser.rs, where it only makes cause: undefined an own property as Node's AbortError does; and the edit to test-fs-promises-file-handle-append-file.js matches the upstream Node version of that file rather than a Bun-authored addition.
Extended reasoning...
The change spans Rust argument parsing in src/runtime/node/node_fs.rs (new FileSignal enum with a GC-protected non-native variant and a two-point JS-thread read), a new C++ export shared with the JS-visible validateAbortSignal that changes its error code and now rejects callables for all 42 call sites, the AbortError constructor helper, and roughly 500 lines of new tests. It touches no auth, crypto or injection surface. It is not approved because it is a large native change with deliberate, documented divergences from Node (null/"" as absent, completion-time rejection for FileHandle#appendFile) and a behavior change to a widely shared validator, which a maintainer should weigh; the four optional/pre-existing threads from the prior review were author-resolved with replies whose text is not visible here.
…e test The test for a signal that is aborted after the call flipped the signal of the three FileHandle forms before their call, while the file was still being opened. It now flips it once the method has been called.
There was a problem hiding this comment.
I reviewed the latest push and found no bugs; both points from my earlier inline comments (the TS2367 comparison in writeFileAsyncIterator and the FileHandle rows aborting before the call) are addressed in the current code. Because this changes the shared validateAbortSignal contract for every caller and moves a GC-protected user object across the pool round-trip, a human look is still worthwhile.
What was reviewed:
- FileSignal ownership and thread affinity: the pending-activity ref is released in its own Drop on every exit; the NonNative Protected is only read (aborted/reason) and dropped on the JS thread, since Job is always freed there (job.rs take/release_unrun) and the pool only reads the native flag.
- Exception handling in validateNonNativeAbortSignal (hasProperty, name.toWTFString) and in AsyncFSTask::then, where getter errors now reject the completion after the arguments are dropped; INVALID_ARG_INSTANCE keeps code ERR_INVALID_ARG_TYPE with Node's "must be an instance of AbortSignal" sentence and rejecting callables matches Node's typeof check.
- Removing the undefined-cause special case in Bun__wrapAbortError: the only other caller (h2_frame_parser) always passes a defined reason.
- The edit to test-fs-promises-file-handle-append-file.js restores Node's upstream doAppendAndCancel verbatim rather than diverging from it.
Extended reasoning...
The change replaces the AbortSignalRef fields and scopeguards in src/runtime/node/node_fs.rs with an args::FileSignal enum (native ref or GC-protected arbitrary object), adds a C++ export Bun__validateNonNativeAbortSignal shared with the JS-visible validateAbortSignal (NodeValidatorBodies.h, 42 call sites), makes AsyncFSTask::then run user getters after dropping the arguments, forwards signal from FileHandle#appendFile and validates it on the iterable writeFile path in fs.promises.ts, and adds roughly 570 lines of tests. It touches no auth, crypto or injection surface; the sensitive part is native memory and thread affinity of a user object held across the work pool. Deciding against approval: the validator's message changes for every caller (one existing test assertion updated), several Bun-only divergences from Node are pinned deliberately (null and "" accepted as no signal, callback-form getter errors delivered to the callback, FileHandle#appendFile rejecting after data landed), and soundness of the NonNative variant rests on an unsafe ThreadIsolatedArg impl; the hunt ran dry with no findings and the prior inline feedback is addressed in db5aeaa.
…llers `args::ReadFile` has no `Drop` impl any more, so clippy asks for the initializer in place of assignments after `default()`.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked three things in this push and they hold up: the removed undefined-cause early return in Bun__wrapAbortError does not change the two h2_frame_parser.rs callers (their reason comes from a native signal, never undefined) and for a non-native signal with reason === undefined the resulting own cause: undefined matches Node's new AbortError(undefined, { cause }); FileSignal::NonNative's Protected is only dropped on the JS thread, since Job<C> is freed by its completion or by release_unrun, both JS-thread; and ERR::INVALID_ARG_INSTANCE still emits code ERR_INVALID_ARG_TYPE, so the new "must be an instance of AbortSignal" text matches Node's validateAbortSignal wording and code.
Extended reasoning...
This push reshapes options.signal handling for readFile/writeFile/appendFile across the Rust parsers, the C++ validator, and the fs.promises builtin, replacing hand-written Drop impls with a FileSignal enum that owns the native ref or a GC-protected JS object. The checks above (shared-helper callers, drop thread affinity, error code of the shared validator) were ruled out from the code and are not covered by the posted inline finding or earlier reviews.
The three callers now build `args::ReadFile` with `..Default::default()`. One of them reads a descriptor, so mordant can see that `flag` matters for a path only. The shape of the struct is as on main.
Stacked on #47993 (the sync rule). Merge it first.
Problem
fs.readFile,fs.writeFileandfs.appendFileaccept only Bun's ownAbortSignalasoptions.signal. Node v26.3.0 accepts any object that hasaborted.bun testwith happy-dom,new AbortController().signalthrowsThe "signal" argument must be of type AbortSignal. Received an instance of AbortSignal.args::signal_option(src/runtime/node/node_fs.rs:4491).Fix
validateAbortSignalrule. The JS-visible validator shares it and throws Node's sentence.abortedbefore the schedule and at completion.test/js/node/fs/promises.test.js(36 of 39 new tests fail on main),fs.test.ts, 89 upstream files, 519test/aot/parallelprograms.signalbeforeflag, a real happy-dom test, the root onAsyncFSJs.Background
abortedonce and keeping nothing is about 30 lines (not built).b.anyand.variadic#46789 rewrites these parsers. There the rule is one schema member.Downsides
FileHandle#appendFile. The call rejects with the data maybe on disk.validateAbortSignalnow saysan instance of AbortSignalfor every caller and refuses a function withaborted. Matches on the old text break.Notes
The stack and the tracker
options.signalonly for the async forms of readFile and writeFile #47993 and one commit on it. node:fs: keepoptions.signalonly for the async forms of readFile and writeFile #47993 stops the sync forms from keeping a signal (readFileSync(p, { signal: AbortSignal.abort() })fails withEINTRon main) and has the tests for that. The diff here is the accept rule on top.FileHandle#appendFileforwardssignal,fs.promises.writeFilechecks it for iterable data, the upstream test fordoAppendAndCancel). It takes the accept side from node:fs: accept AbortSignal-shaped objects in readFile/writeFile/watch #35275, which the stale sweep closed with no review.fs.watch. Its readFile and writeFile half is here.flagin readFileSync/readFile and an emptymodein writeFile/appendFile #46575 and node:fs: take a falsyflagas the default in writeFile and appendFile #45645 edit the same two parse blocks and rebase after this.b.anyand.variadic#46789 removesArgumentsSliceand writes the fs options as a schema. This PR does not touchArgumentsSlice. On that basesignal_optionbecomes an arm of the sharedsignalmember for a non-native object (WatchOptionsstays native-only), andFileSignalwith its two reads stays as it is.Self-review
ENOENT), had none of the native size checks, and used the fixed chunk size that fs.readFile: honor AbortSignal between read chunks #36259 was asked not to use. That shape is gone.signalbeforeflagandmodein the native parsers. Main's order is kept, and bindgen: deleteb.anyand.variadic#46789 writes the same order. With{ signal: 1, flag: "zz" }Node names the signal, Bun names the flag.test/cli/test/test-environment.test.ts. The real-package tests of that file time out at 5 s in a plain debug run. The new test uses a stand-in package, asplugins.test.tsandimport-meta-env.test.tsdo. With the real happy-dom 17.0.3 (@happy-dom/global-registrator, run by hand on the release build) the nine forms answer as in Node.AsyncFSJs, the JS-thread half of the task. It adds 8 bytes to all 41 async fs tasks. In the arguments only three grow. A test counts the protected objects while 50 calls are pending and after each way they end.Other files in the diff
SSLConfig.rs,Blob.rsandfetch.rsbuildargs::ReadFilewith..Default::default(). The struct has noDropimpl any more (FileSignalreleases the signal), so clippy asks for that form.mordant-baseline.tomlgets one entry,field_valid_only_whenforargs::ReadFile::flag. With that form of the three callers, mordant can see thatflagmatters for a path only. The struct is as on main.What a non-native signal gets
aborted.abortedbefore the task is scheduled (parse_async_args), and one when it completes (AsyncFSTask::then, after the arguments are dropped, because the read runs code of the program).reasonis read ifabortedis truthy. An error that a getter throws is the error of the call.abortevent of the object would let the pool thread stop early too. It needs a listener per call or a map per signal, and an object that sends no event gets nothing from it. It is not built.AbortError. So anAbortErrorcan take the place ofENOENTfor a signal that is aborted by then.NodeValidatorBodies.h. The JS-visiblevalidateAbortSignal(42 call sites) and the fs option both call it.Answers that only Bun gives, pinned and labelled in the tests
signal: nullandsignal: ""are no signal, as on main. Node throwsERR_INVALID_ARG_TYPE.fast_getin place offast_get_truthyinsignal_optionchanges that, if a maintainer wants the break.fs.readFilegoes to the callback. Node raises it as an uncaught exception.FileHandle#appendFilewith anAbortSignalthat is aborted after the call: Node and main resolve with the data appended (main ignored the signal). Now it rejects withAbortError, asfs.promises.appendFiledoes, and the data was in the file in 19 of 20 runs.Measurements (release builds of main b1305dd and this branch, linux-x64)
valgrind,perf,straceandbloatyare not in the container. Aperf_event_opencounter of user instructions, gdb breakpoints by address, an inotify counter of opens,sizeandnmreplace them.readFileSync(p)readFileSync(p, "utf8")readFileSync(p, { encoding })readFileSync(p, { signal })writeFileSync(p, d)writeFileSync(p, d, { encoding })writeFileSync(p, d, { signal })readdirSync(dir, { withFileTypes })fs.promises.readFile(p)fs.promises.readFile(p, { encoding })fs.promises.readFile(p, { signal })fs.readFile(p, { signal }, cb)fast_get_truthylookssignalup with the identifier that the VM already has. It is also the malloc:readFileSync(p, {})2.03 -> 1.03 per call,writeFileSync(p, d, {})4.01 -> 3.01.readFileSync3,030 instructions,fs.promises.readFile12,156 (10,288 with a native signal).args::ReadFile96 -> 104 bytes,args::WriteFile136 -> 144.Job<ReadFile>272 -> 280,Job<WriteFile>andJob<AppendFile>296 -> 304,Job<Access>224 -> 224. Release binary: text +1,971 bytes, 0 bytes on disk. 1 new C++ export, 0 host functions, no JS module.AbortSignalper sync call with a native signal: 6 -> 1 (node:fs: keepoptions.signalonly for the async forms of readFile and writeFile #47993 has this too). Promise read: 8 -> 8. LiveAbortSignalafter 100k calls: 1. Protected objects after 10k calls with a non-native signal: 4, as before the loop.has1 in every form,get0 in the sync forms and 2 in the async forms (Node: 2 to 4).openatof the target before a rejection: 0. Calls of the export without a non-native signal: 0.nulland""lines. Matrix of 24 values ofsignalin every form: 59 of 323 rows differ (264 on main). Polyfill rows equal: 4 of 4. Existing assertions edited: 1 (sql-reserve-abort.test.ts, the sentence ofvalidateAbortSignal). AOT table: 6 rows of the binding changed.bun build --compile --aotexecutables: 0 bytes of difference for 12 programs (no fs, sync fs, promise and callback forms without a signal, a native signal given to the sync, promise and callback forms, tofetch, toBun.serve, with a listener).The 59 rows that still differ from Node
nulland""in the 13 forms. Kept as they are on main.Proxyof a native signal in the async forms. The proxy is noAbortSignalcell, soabortedis read through it, and Bun's getter throws for athisthat is a proxy. The sync forms do the I/O, as in Node.{ aborted: true },{ aborted: true, reason }). In the 3 callback forms the callback comes on the next tick (node:fs: call back before return for an already-aborted signal in readFile, writeFile and appendFile #47489). Infs.readFile(fd)Node'sReadFileContextdrops theAbortErrorfor a descriptor of the caller.Object.create(AbortSignal.prototype)in the async forms.Siblings left out
fs.watchrefuses the same signal. It needs the abort listener that Node has (lib/fs.js), insrc/js/internal/fs/watch.ts: a call ofvalidate_abort_signalinnode_fs_watcher.rsalone gives a watcher that never closes on abort.fs.promises.watchand the http2requestfollow the signal in JS already and fail at a second, native class test. http2 belongs to node:http2: free a request's slot when its stream closes on the wire, abort request({ signal }) from JS only #42763 and http: consolidate the open fetch, Bun.serve, node:http, node:http2 and HTTP/3 pull requests #46528.Bun.spawn, a Bun API with its own rule.URLof another realm as the path (types.rs:1148).flagandmodeare read beforesignal, and the path before the options. Node checkssignalfirst.The "flush" argument ...).getStreamOptions, thefs.statcallback,add-abort-signal.ts.readdir,readlink,realpath,mkdtempandopendirnever readoptions.signal.Suites run on the debug build
test/js/node/fs/promises.test.js69 pass,sql-reserve-abort.test.ts9 pass,abort-signal-leak-read-write-file.test.ts,fs-path-length,fs-promises-writeFile-async-iterator,fs-leak,event-emitter.test.ts, the abort tests ofnode-http2.test.js.fs.test.ts: 636 pass with the 16 cases of node:fs: keepoptions.signalonly for the async forms of readFile and writeFile #47993. 13 tests time out on this machine (readdir ... x 100, the WebAssembly.Memory reads and others), the same 13 on main's build.test/js/node/test/parallel: 41test-fs-*files forreadFile,writeFile,appendFileandfs.promises, 26 files about abort signals (events, timers, streams, child_process, fs.watch), 22 http2 files about abort and destroy.Suites run on the release build
test/internal/source-lints194 pass,test/internal/builtins-table133 pass.test/aot/parallel(each one with a bound on what the build counts, or that usesnode:fs): the same result as on main. The new programbuiltin-modules/signal-that-is-no-abort-signal-given-to-read-file-and-write-file.mjspasses, also with--aot-validateand the checks of the run time.