Skip to content

Conversation

@mildsunrise
Copy link
Contributor

@mildsunrisemildsunrise commented Feb 28, 2020

Currently writeFile and writeFileSync 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), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included ← I'd like to test for this, but I can't find an OS-independent way of doing it...
  • documentation is changed or added ← should we mention this change?
  • commit message follows commit guidelines

Completely disables the use of positioned writes at writeFile and writeFileSync, which allows it to work with non-seekable files. Fixes: nodejs#31926
@nodejs-github-botnodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 28, 2020
@addaleax
Copy link
Member

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?

@mildsunrise
Copy link
ContributorAuthor

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:

node/lib/fs.js

Lines 1289 to 1293 in 3d894d0

if(isFd(path)){
constisUserFd=true;
writeAll(path,isUserFd,data,0,data.byteLength,null,callback);
return;
}

So this is a patch/minor change (unless I'm missing something)

Copy link
Member

@addaleaxaddaleax left a 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 :)

@nodejs-github-bot
Copy link
Collaborator

@mildsunrise
Copy link
ContributorAuthor

No prob! It was more than a year ago, I'd have forgotten too 😹

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 29, 2020

CI: https://ci.nodejs.org/job/node-test-pull-request/29452/ (:white_check_mark:)

@codecov-io
Copy link

Codecov Report

Merging #32006 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@ 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.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d894d0...d1e694c. Read the comment docs.

@addaleaxaddaleax added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 29, 2020
addaleax pushed a commit that referenced this pull request Mar 7, 2020
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]>
@addaleax
Copy link
Member

Landed in f69de13 🎉

@MylesBorins
Copy link
Contributor

This does not land cleanly on v13.x. Should it be backported?

@addaleax
Copy link
Member

@MylesBorins Wrong label? And, yes, it should be backported. 👍

@mildsunrisemildsunrise deleted the writefile-non-seekable branch March 10, 2020 11:33
mildsunrise added a commit to mildsunrise/node that referenced this pull request Mar 10, 2020
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]>
MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
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]>
@MylesBorinsMylesBorins mentioned this pull request Mar 10, 2020
targos pushed a commit that referenced this pull request Apr 20, 2020
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]>
@targostargos mentioned this pull request Apr 22, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.fsIssues and PRs related to the fs subsystem / file system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@mildsunrise@addaleax@nodejs-github-bot@codecov-io@MylesBorins