fs: handle early writeFile stream errors - #63472
Conversation
a3d19d5 to
c1d1472
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #63472 +/- ##
=======================================
Coverage 90.16% 90.16%
=======================================
Files 746 746
Lines 242760 242820 +60
Branches 45765 45789 +24
=======================================
+ Hits 218875 218946 +71
+ Misses 15375 15352 -23
- Partials 8510 8522 +12
🚀 New features to boost your workflow:
|
|
Pushed a follow-up for the failing writeFile stream cases. The already-errored stream path now keeps the temporary Local validation:
|
|
Current blocker looks unrelated to this patch: the only failed check is I tried to rerun the failed jobs, but GitHub requires repository admin permissions. Could someone with access rerun the failed shared-libraries job? |
|
Following up now that the PR is approved: the only red check is still I do not have permission to rerun that job. Could someone with access rerun the failed shared-libraries check when convenient? |
|
Following up once more since the PR is approved and mergeable. The only red job remains I still lack permission to rerun that job. Could a maintainer rerun that shared-libraries job when convenient? |
|
I don't see an option to re-run the failed CI. @cookesan Can you please rebase from main and force push with lease? |
Attach a temporary error listener to readable stream inputs before opening the destination file. This lets writeFile() reject with the stream error instead of allowing an early source error to become an uncaught exception. Remove the listener when the write finishes or when opening the destination fails. Signed-off-by: cookesan <6601329+cookesan@users.noreply.github.com>
Signed-off-by: cookesan <6601329+cookesan@users.noreply.github.com>
Keep the temporary writeFile error listener through the pending next-tick error emission when a stream is already errored at entry. This lets writeFile reject from the stored stream error without letting the stream emit an unhandled error, and still removes the listener after the pending emission. Signed-off-by: cookesan <6601329+cookesan@users.noreply.github.com>
2af6b28 to
e2a951d
Compare
|
Done — rebased onto main and force-pushed with lease. Locally, the release build, the two focused filesystem tests, and targeted JS lint pass. |
| checkAborted(options.signal); | ||
| const streamErrorHandler = makeWriteFileStreamErrorHandler(data); |
There was a problem hiding this comment.
The abort check runs before the temporary stream error listener is attached. If data is already errored and the signal is already aborted, writeFile() rejects with AbortError, but the pending stream error is still emitted as an uncaughtException. Attach the handler first and clean it up when the abort check throws:
| checkAborted(options.signal); | |
| const streamErrorHandler = makeWriteFileStreamErrorHandler(data); | |
| const streamErrorHandler = makeWriteFileStreamErrorHandler(data); | |
| try { | |
| checkAborted(options.signal); | |
| } catch (err) { | |
| streamErrorHandler?.cleanup(); | |
| throw err; | |
| } |
A regression test would look like this:
controller.abort();
stream.destroy(sourceError);
process.once('uncaughtException', mustNotCall());
await assert.rejects(writeFile(target, stream, { signal }), { code: 'ABORT_ERR' });
await nextTick();
assert.strictEqual(stream.listenerCount('error'), 0);The same case should be covered for both path-based writeFile() and FileHandle.writeFile().
The
fs.promises.writeFile()stream path could miss a readableerror emitted before the destination file opened. The returned promise
could reject while the source stream error also surfaced as an uncaught
exception.
This attaches a temporary error listener for readable stream inputs
before opening the destination, checks recorded stream errors before and
during async iteration, and removes the listener when the write finishes
or when opening the destination fails.
FileHandle.writeFile()uses thesame helper.
Tests cover early readable errors for path and
FileHandlewrites, pluslistener cleanup when opening the destination fails.
Tests:
Fixes: #58742