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 default params for fs.write #7856

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@papandreou
Contributor

papandreou commented Jul 23, 2016

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

@mscdex

View changes

Show outdated Hide outdated test/sequential/test-fs-write-buffer-without-length.js Outdated
@mscdex

View changes

Show outdated Hide outdated test/sequential/test-fs-write-buffer-without-offset-and-length.js Outdated
@mscdex

View changes

Show outdated Hide outdated test/sequential/test-fs-write-buffer-without-length.js Outdated
@mscdex

View changes

Show outdated Hide outdated test/sequential/test-fs-write-buffer-without-offset-and-length.js Outdated
@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Jul 23, 2016

Contributor

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

Contributor

mscdex commented Jul 23, 2016

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

@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Jul 24, 2016

Contributor

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

Contributor

papandreou commented Jul 24, 2016

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

@thefourtheye

View changes

Show outdated Hide outdated test/parallel/test-fs-write-buffer-without-length.js Outdated
@thefourtheye

View changes

Show outdated Hide outdated test/parallel/test-fs-write-buffer-without-length.js Outdated
@thefourtheye

View changes

Show outdated Hide outdated test/parallel/test-fs-write-buffer-without-offset-and-length.js Outdated
@thefourtheye

View changes

Show outdated Hide outdated test/parallel/test-fs-write-buffer-without-offset-and-length.js Outdated
@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Aug 1, 2016

Contributor

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

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

Contributor

papandreou commented Aug 1, 2016

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

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

@yorkie

View changes

Show outdated Hide outdated test/parallel/test-fs-write-buffer.js Outdated
@yorkie

View changes

Show outdated Hide outdated test/parallel/test-fs-write-sync.js Outdated
@cjihrig

View changes

Show outdated Hide outdated test/parallel/test-fs-write-buffer-without-length.js Outdated
@cjihrig

View changes

Show outdated Hide outdated test/parallel/test-fs-write-sync-without-length.js Outdated
@cjihrig

View changes

Show outdated Hide outdated test/parallel/test-fs-write-sync-without-length.js Outdated
@cjihrig

View changes

Show outdated Hide outdated test/parallel/test-fs-write-sync-without-length.js Outdated
@cjihrig

View changes

Show outdated Hide outdated test/parallel/test-fs-write-sync-without-length.js Outdated
@cjihrig

View changes

Show outdated Hide outdated test/parallel/test-fs-write-sync-without-offset-and-length.js Outdated
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 1, 2016

Member

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

Member

jasnell commented Aug 1, 2016

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

}
if (typeof length !== 'number') {
length = buffer.length - offset;
}

This comment has been minimized.

@trevnorris

trevnorris Aug 2, 2016

Contributor

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

@trevnorris

trevnorris Aug 2, 2016

Contributor

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

This comment has been minimized.

@papandreou

papandreou Aug 2, 2016

Contributor

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

@papandreou

papandreou Aug 2, 2016

Contributor

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

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Aug 2, 2016

Contributor

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.

Contributor

trevnorris commented Aug 2, 2016

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.

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell
Member

jasnell commented Aug 3, 2016

@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Aug 11, 2016

Contributor

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.

Contributor

papandreou commented Aug 11, 2016

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.

@trevnorris

This comment has been minimized.

Show comment
Hide comment
Contributor

trevnorris commented Aug 11, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 11, 2016

Member

LGTM if CI is green.

Member

jasnell commented Aug 11, 2016

LGTM if CI is green.

@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou Aug 12, 2016

Contributor

Fixed the lint error and rebased.

Contributor

papandreou commented Aug 12, 2016

Fixed the lint error and rebased.

@trevnorris

This comment has been minimized.

Show comment
Hide comment
@trevnorris

trevnorris Aug 16, 2016

Contributor

@jasnell i only see AIX and those all look flaky. I'm cool w/ this landing.

Contributor

trevnorris commented Aug 16, 2016

@jasnell i only see AIX and those all look flaky. I'm cool w/ this landing.

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Nov 8, 2016

Member

I will land this

Member

sam-github commented Nov 8, 2016

I will land this

sam-github added a commit to sam-github/node that referenced this pull request Nov 8, 2016

fs: Fix default params for fs.write(Sync)
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>
@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github Nov 8, 2016

Member

Landed in 0b5191f, thanks @papandreou

Member

sam-github commented Nov 8, 2016

Landed in 0b5191f, thanks @papandreou

@sam-github sam-github closed this Nov 8, 2016

Fishrock123 added a commit that referenced this pull request Nov 22, 2016

fs: Fix default params for fs.write(Sync)
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>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Dec 13, 2016

Member

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

Member

MylesBorins commented Dec 13, 2016

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

@gibfahn

This comment has been minimized.

Show comment
Hide comment
@gibfahn

gibfahn May 16, 2017

Member

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 ?

Member

gibfahn commented May 16, 2017

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 ?

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github May 16, 2017

Member
  1. This adds support for new arg combinations, shouldn't it be tagged semver-minor?
  2. @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.
Member

sam-github commented May 16, 2017

  1. This adds support for new arg combinations, shouldn't it be tagged semver-minor?
  2. @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.
@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou

papandreou May 16, 2017

Contributor

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

Contributor

papandreou commented May 16, 2017

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

@sam-github

This comment has been minimized.

Show comment
Hide comment
@sam-github

sam-github May 23, 2017

Member

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?

Member

sam-github commented May 23, 2017

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?

papandreou added a commit to assetgraph/node that referenced this pull request May 23, 2017

fs: Fix default params for fs.write(Sync)
(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.
@papandreou

This comment has been minimized.

Show comment
Hide comment
@papandreou
Contributor

papandreou commented May 23, 2017

papandreou added a commit to assetgraph/node that referenced this pull request Sep 18, 2017

fs: Fix default params for fs.write(Sync)
(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.

MylesBorins added a commit that referenced this pull request Oct 16, 2017

fs: Fix default params for fs.write(Sync)
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>

@MylesBorins MylesBorins referenced this pull request Oct 17, 2017

Merged

v6.12.0 proposal #16263

MylesBorins added a commit that referenced this pull request Oct 25, 2017

fs: Fix default params for fs.write(Sync)
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>

@MylesBorins MylesBorins referenced this pull request Nov 3, 2017

Merged

v4.8.6 proposal #16500

MylesBorins added a commit that referenced this pull request Nov 6, 2017

2017-11-07, Version 6.12.0 'Boron' (LTS)
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

MylesBorins added a commit that referenced this pull request Nov 7, 2017

2017-11-07, Version 6.12.0 'Boron' (LTS)
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

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

2017-11-07, Version 6.12.0 'Boron' (LTS)
Notable Changes:

* assert:
  - assert.fail() can now take one or two arguments (Rich Trott)
    nodejs#12293
* crypto:
  - add sign/verify support for RSASSA-PSS (Tobias Nießen)
    nodejs#11705
* deps:
  - upgrade openssl sources to 1.0.2m (Shigeki Ohtsu)
    nodejs#16691
  - upgrade libuv to 1.15.0 (cjihrig)
    nodejs#15745
  - upgrade libuv to 1.14.1 (cjihrig)
    nodejs#14866
  - upgrade libuv to 1.13.1 (cjihrig)
    nodejs#14117
  - upgrade libuv to 1.12.0 (cjihrig)
    nodejs#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) nodejs#7856
* inspector:
  - enable --inspect-brk (Refael Ackermann)
    nodejs#12615
* process:
  - add --redirect-warnings command line argument (James M Snell)
    nodejs#10116
* src:
  - allow CLI args in env with NODE_OPTIONS (Sam Roberts)
    nodejs#12028)
  - --abort-on-uncaught-exception in NODE_OPTIONS (Sam Roberts)
    nodejs#13932
  - allow --tls-cipher-list in NODE_OPTIONS (Sam Roberts)
    nodejs#13172
  - use SafeGetenv() for NODE_REDIRECT_WARNINGS (Sam Roberts)
    nodejs#12677
* test:
  - remove common.fail() (Rich Trott)
    nodejs#12293

PR-URL: nodejs#16263
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment