fs: Fix default params for fs.write by papandreou · Pull Request #7856 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

fs

Description of change

Add support for fs.write(fd, buffer, cb) and fs.write(fd, buffer, offset, cb) as documented at https://nodejs.org/api/fs.html#fs_fs_write_fd_data_position_encoding_callback (it says that data can be either a string or a Buffer).

Update: Extended to apply to fs.writeSync as well, which turned out to have the same issue (#7879).

This looks a bit hectic. What about moving this common.mustCall(...) to a variable and passing that as the fourth argument instead? That way the fs.write() fits all on one line.

Seems like I copy/pasted from a very old version of the https://github.com/nodejs/node/blob/master/test/parallel/test-fs-buffer.js test. Will try to fix it up.

https://github.com/nodejs/node/blob/master/test/parallel/test-fs-write-buffer.js, actually, and that's still just as messy :)

Perhaps, but let's not continue that trend ;-)

Agreed. Fixed in c9d7a77

In both tests I had to keep the definition of cb inside the other callback because it references fd in fs.closeSync(fd);.

Any reason the tests are in sequential/ instead of parallel/?

@mscdex Not a good one, no. I just misunderstood the sequential vs. parallel classification. Moved them to parallel now :)

You can use assert.ifError

Fixed the nits, including the test that I copied from.

Also fixed fs.writeSync in the same way (came up in #7879).

Why removing this line?

@thefourtheye suggested that: #7856 (comment)

Question for @nodejs/ctc : is this a patch or a semver-minor?

The comment at the top of the function does not match public documentation. Mind fixing that.

Sure thing, fixed in 4062d4cf. I changed both the comment and the documentation to match the implementation.

While doing the review I realized that our documented API doesn't come close to following current implementation. For example, src/node_file.cc:node::WriteBuffer() still has all the same throw logic as before. While src/node_file.cc:node::WriteString() relies on coercion rules from external calls to ParseEncoding(), etc.

There is a deeper issue that needs to be fixed. This PR does help bring it closer to compliance to public docs, but is only a superficial fix. I'm fine with this landing, but there should be a follow-up PR that fixes things all the way down.

@jasnell Technically a patch since it aligns implementation closer to documentation, and it shouldn't introduce any regressions.

@trevnorris +1

Squashed and rebased on master as conflicts had been introduced.

The code that works out the default values of the default parameters for fs.write got a bit more complex because the line callback = makeCallback(arguments[arguments.length - 1]); was removed from master in the mean time.

I believe this work is finished and ready to land now, but let me know if you have any feedback, especially about the code that sets the default parameters and finds the callback.

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

I will land this

Add support for fs.write(fd, buffer, cb) and fs.write(fd, buffer, offset, cb) as documented at https://nodejs.org/api/fs.html#fs_fs_write_fd_data_position_encoding_callback and equivalently for fs.writeSync Update docs and code comments to reflect the implementation. PR-URL: nodejs#7856 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Landed in 0b5191f, thanks @papandreou

Add support for fs.write(fd, buffer, cb) and fs.write(fd, buffer, offset, cb) as documented at https://nodejs.org/api/fs.html#fs_fs_write_fd_data_position_encoding_callback and equivalently for fs.writeSync Update docs and code comments to reflect the implementation. PR-URL: #7856 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

added lts labels. Is this going to be reverted as per #10242?

It sounds like this is a minor behavioural change, where the previous code didn't do exactly what the docs said. Given that this was reported as an issue in v6.x, it might be worth backporting.

Thoughts @nodejs/lts ?

This adds support for new arg combinations, shouldn't it be tagged semver-minor?
@papandreou Can you think of any negative side-effects? Is this purely additive, or does it change an existing behaviour? An existing behaviour that is useful, I mean, changing the existing behaviour of writing absolutely none of the buffer doesn't count.

@sam-github, no, I can't think of any negative side effects of backporting, especially now that it has been on v7 for a while without causing any problems. To my knowledge the change in behavior has only been brought up once (#10242), and that was resolved amicably.

Based on the number of reports of the original problem, I agree that it seems to be something that comes up often enough that we could prevent a few wtf experiences for 6.x users by backporting it.

Above makes sense to me, @papandreou, thanks. Do you have time to do the backport this by opening a PR against the v6.x-staging branch?

@sam-github, sure! #13179

(Backported from nodejs#7856 to v6.x-staging) Add support for fs.write(fd, buffer, cb) and fs.write(fd, buffer, offset, cb) as documented at https://nodejs.org/api/fs.html#fs_fs_write_fd_data_position_encoding_callback and equivalently for fs.writeSync Update docs and code comments to reflect the implementation.

Add support for fs.write(fd, buffer, cb) and fs.write(fd, buffer, offset, cb) as documented at https://nodejs.org/api/fs.html#fs_fs_write_fd_data_position_encoding_callback and equivalently for fs.writeSync Update docs and code comments to reflect the implementation. Backport-PR-URL: #13179 PR-URL: #7856 Reviewed-By: Sam Roberts <vieuxtech@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net> Reviewed-By: Yorkie Liu <yorkiefixer@gmail.com> Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>

Notable Changes: * assert: - assert.fail() can now take one or two arguments (Rich Trott) #12293 * crypto: - add sign/verify support for RSASSA-PSS (Tobias Nießen) #11705 * deps: - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu) #16691 - upgrade libuv to 1.15.0 (cjihrig) #15745 - upgrade libuv to 1.14.1 (cjihrig) #14866 - upgrade libuv to 1.13.1 (cjihrig) #14117 - upgrade libuv to 1.12.0 (cjihrig) #13306 * fs: - Add support for fs.write/fs.writeSync(fd, buffer, cb) and fs.write/fs.writeSync(fd, buffer, offset, cb) as documented (Andreas Lind) #7856 * inspector: - enable --inspect-brk (Refael Ackermann) #12615 * process: - add --redirect-warnings command line argument (James M Snell) #10116 * src: - allow CLI args in env with NODE_OPTIONS (Sam Roberts) #12028) - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts) #13932 - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts) #13172 - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts) #12677 * test: - remove common.fail() (Rich Trott) #12293 PR-URL: #16263

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Jul 23, 2016

mscdex reviewed Jul 23, 2016
View reviewed changes

thefourtheye reviewed Jul 25, 2016
View reviewed changes

papandreou mentioned this pull request Jul 26, 2016

openSync/writeSync/closeSync produces empty file #7879

Closed

yorkie reviewed Aug 1, 2016
View reviewed changes

trevnorris reviewed Aug 2, 2016
View reviewed changes

papandreou force-pushed the fix/defaultParameterForFsWriteWithBuffer branch 3 times, most recently from 8a81850 to 984cd0b Compare August 11, 2016 09:50

sam-github approved these changes Nov 8, 2016

View reviewed changes

sam-github closed this Nov 8, 2016

bnoordhuis mentioned this pull request Dec 13, 2016

Change in fs.statSync behavior between 7.1.0 and 7.2.0 #10242

Closed

MylesBorins added lts-watch-v4.x labels Dec 13, 2016

papandreou mentioned this pull request May 23, 2017

fs: Fix default params for fs.write(Sync) (backport to v6) #13179

Closed

4 tasks

gibfahn added the semver-minor PRs that contain new features and should be released in the next minor version. label May 30, 2017

vsemozhetbyt mentioned this pull request Jun 25, 2017

test: test/parallel/test-fs-write-sync.js failed when node < 7.2.0 #13899

Closed

MylesBorins added backport-requested-v6.x and removed lts-watch-v4.x labels Sep 19, 2017

MylesBorins mentioned this pull request Oct 17, 2017

v6.12.0 proposal #16263

Merged

MylesBorins mentioned this pull request Nov 3, 2017

v4.8.6 proposal #16500

Merged

fs: Fix default params for fs.write by papandreou · Pull Request #7856 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: Fix default params for fs.write - #7856

fs: Fix default params for fs.write#7856papandreou wants to merge 1 commit intonodejs:masternodejs/node:masterfrom assetgraph:fix/defaultParameterForFsWriteWithBufferassetgraph/node:fix/defaultParameterForFsWriteWithBufferCopy head branch name to clipboard

Conversation

papandreou commented Jul 23, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mscdex commented Jul 23, 2016

Uh oh!

papandreou commented Jul 24, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

papandreou commented Aug 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

trevnorris commented Aug 2, 2016

Uh oh!

jasnell commented Aug 3, 2016

Uh oh!

papandreou commented Aug 11, 2016

Uh oh!

trevnorris commented Aug 11, 2016

Uh oh!

sam-github commented Nov 8, 2016

Uh oh!

sam-github commented Nov 8, 2016

Uh oh!

MylesBorins commented Dec 13, 2016

Uh oh!

gibfahn commented May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sam-github commented May 16, 2017

Uh oh!

papandreou commented May 16, 2017

Uh oh!

sam-github commented May 23, 2017

Uh oh!

papandreou commented May 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

fs: Fix default params for fs.write#7856
papandreou wants to merge 1 commit into
nodejs:masterfrom
assetgraph:fix/defaultParameterForFsWriteWithBuffer

papandreou commented Jul 23, 2016 •
edited
Loading

gibfahn commented May 16, 2017 •
edited
Loading