GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — fs: fix writeFile[Sync] for non-seekable files by mildsunrise · Pull Request #32006 · nodejs/node

Conversation

mildsunrise commented Feb 28, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss, addaleax, and Tomato27 reacted with thumbs up emoji

All reactions

👍 3 reactions


          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 file-system APIs and the fs module. 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?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax approved these changes

Feb 28, 2020

View reviewed changes

addaleax left a comment

Copy link

Copy Markdown

Member

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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nodejs-github-bot commented Feb 28, 2020

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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 😹

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Feb 29, 2020 •

edited by addaleax

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Collaborator

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax added the author ready PRs with CI started, the required approvals, and no outstanding review comments. 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 <[email protected]>

addaleax commented Mar 7, 2020

Copy link

Copy Markdown

Member

Landed in f69de13 🎉

mildsunrise reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax closed this

Mar 7, 2020

MylesBorins added the backported-to-v13.x label

Mar 9, 2020

MylesBorins commented Mar 9, 2020

Copy link

Copy Markdown

Contributor

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

addaleax commented Mar 10, 2020

Copy link

Copy Markdown

Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

MylesBorins added backport-requested-v13.x and removed backported-to-v13.x labels

Mar 10, 2020

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 <[email protected]>

mildsunrise mentioned this pull request

Mar 10, 2020

[v13.x] fs: fix writeFile[Sync] for non-seekable files #32172

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 <[email protected]>

MylesBorins added backported-to-v13.x and removed backport-requested-v13.x labels

Mar 10, 2020

MylesBorins mentioned this pull request

Mar 10, 2020

v13.11.0 proposal #32185

Merged

targos pushed a commit that referenced this pull request

Apr 20, 2020


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

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]>

targos mentioned this pull request

Apr 22, 2020

v12.16.3 release proposal #33009

Merged

Reviewers

addaleax

addaleax approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready

PRs with CI started, the required approvals, and no outstanding review comments.

Issues and PRs related to file-system APIs and the fs module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

GeezerVison

Conversation

mildsunrise commented Feb 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

addaleax commented Feb 28, 2020

Uh oh!

mildsunrise commented Feb 28, 2020

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Feb 28, 2020

Uh oh!

mildsunrise commented Feb 28, 2020

Uh oh!

nodejs-github-bot commented Feb 29, 2020 • edited by addaleax Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-io commented Feb 29, 2020

Codecov Report

Uh oh!

addaleax commented Mar 7, 2020

Uh oh!

MylesBorins commented Mar 9, 2020

Uh oh!

addaleax commented Mar 10, 2020

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mildsunrise commented Feb 28, 2020 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

nodejs-github-bot commented Feb 29, 2020 •

edited by addaleax

Loading

Uh oh!

There was an error while loading. Please reload this page.