-
Notifications
You must be signed in to change notification settings - Fork 29.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fs: fix writeFile[Sync] for non-seekable files #32006
Conversation
Completely disables the use of positioned writes at writeFile and writeFileSync, which allows it to work with non-seekable files. Fixes: nodejs#31926
This variant is the one that does change behaviour in the custom-FD use case, right? I feel like 99.9 % of people wouldn’t be affected, but maybe we could still keep that for a separate PR? |
Nope, that behaviour change was already done in #23433 / #23709 (you voted for that option), i.e. the current code never does positioned writes on FDs: Lines 1289 to 1293 in 3d894d0
So this is a patch/minor change (unless I'm missing something) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, thanks for reminding me :)
No prob! It was more than a year ago, I'd have forgotten too 😹 |
CI: https://ci.nodejs.org/job/node-test-pull-request/29452/ (:white_check_mark:) |
Codecov Report
@@ Coverage Diff @@
## master #32006 +/- ##
=======================================
Coverage 96.16% 96.16%
=======================================
Files 196 196
Lines 64918 64918
=======================================
Hits 62429 62429
Misses 2489 2489 Continue to review full report at Codecov.
|
Completely disables the use of positioned writes at writeFile and writeFileSync, which allows it to work with non-seekable files. Fixes: #31926 PR-URL: #32006 Reviewed-By: Anna Henningsen <[email protected]>
Landed in f69de13 🎉 |
This does not land cleanly on v13.x. Should it be backported? |
@MylesBorins Wrong label? And, yes, it should be backported. 👍 |
Completely disables the use of positioned writes at writeFile and writeFileSync, which allows it to work with non-seekable files. Fixes: nodejs#31926 PR-URL: nodejs#32006 Reviewed-By: Anna Henningsen <[email protected]>
Completely disables the use of positioned writes at writeFile and writeFileSync, which allows it to work with non-seekable files. Fixes: #31926 Backport-PR-URL: #32172 PR-URL: #32006 Reviewed-By: Anna Henningsen <[email protected]>
Completely disables the use of positioned writes at writeFile and writeFileSync, which allows it to work with non-seekable files. Fixes: #31926 Backport-PR-URL: #32172 PR-URL: #32006 Reviewed-By: Anna Henningsen <[email protected]>
Currently
writeFile
andwriteFileSync
perform a positioned write at the beginning of the file, which prevents them from working with non-seekable files (#31926).Positioned writes were first removed for
createWriteStream
(#19329) and for custom FDs (#23433, #23709) as well as for files opened in append mode... but they're still used for filenames in normal mode. This PR removes positioned writes completely, as they're not needed for this case either.This also makes it consistent with
readFile
(which does work with non-seekable files).Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes