FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fs: fix writeFile[Sync] for non-seekable files by mildsunrise · Pull Request #32006 · nodejs/node · GitHub

/ node Public

fs: fix writeFile[Sync] for non-seekable files - #32006

Closed
mildsunrise wants to merge 1 commit into
nodejs:masterfrom
mildsunrise:writefile-non-seekable
ConversationCommits (1)ChecksFiles changed
Closed

fs: fix writeFile[Sync] for non-seekable files#32006
mildsunrise wants to merge 1 commit into
nodejs:masterfrom
mildsunrise:writefile-non-seekable

Conversation

mildsunrise commented Feb 28, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

fs: fix writeFile[Sync] for non-seekable files
d1e694c 
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-bot added the fs Issues and PRs related to the fs subsystem / file system. label Feb 28, 2020

addaleax commented Feb 28, 2020

Copy link
Copy Markdown
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 commented Feb 28, 2020

Copy link
Copy Markdown
Contributor Author

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)) {
const isUserFd = true;
writeAll(path, isUserFd, data, 0, data.byteLength, null, callback);
return;
}

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

addaleax approved these changes Feb 28, 2020
View reviewed changes

addaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Right, thanks for reminding me :)

nodejs-github-bot commented Feb 28, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/29439/

mildsunrise commented Feb 28, 2020

Copy link
Copy Markdown
Contributor Author

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

nodejs-github-bot commented Feb 29, 2020 •
edited by addaleax
Loading

Copy link
Copy Markdown
Collaborator

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

codecov-io commented Feb 29, 2020

Copy link
Copy Markdown

Codecov Report

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

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

addaleax added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 29, 2020
addaleax pushed a commit that referenced this pull request Mar 7, 2020
fs: fix writeFile[Sync] for non-seekable files
f69de13 
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 <anna@addaleax.net>

addaleax commented Mar 7, 2020

Copy link
Copy Markdown
Member

Landed in f69de13 🎉

addaleax closed this Mar 7, 2020

MylesBorins commented Mar 9, 2020

Copy link
Copy Markdown
Contributor

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

addaleax commented Mar 10, 2020

Copy link
Copy Markdown
Member

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

mildsunrise 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
fs: fix writeFile[Sync] for non-seekable files
33ca1a6 
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 <anna@addaleax.net>
mildsunrise mentioned this pull request Mar 10, 2020
Closed
MylesBorins pushed a commit that referenced this pull request Mar 10, 2020
fs: fix writeFile[Sync] for non-seekable files
1c4f4cc 
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 <anna@addaleax.net>
MylesBorins mentioned this pull request Mar 10, 2020
Merged
targos pushed a commit that referenced this pull request Apr 20, 2020
fs: fix writeFile[Sync] for non-seekable files
0108148 
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 <anna@addaleax.net>
targos mentioned this pull request Apr 22, 2020
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

addaleax addaleax approved these changes

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL