Skip to content
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: don't alter user provided options object #7831

Closed
wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

@thefourtheye thefourtheye commented Jul 22, 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

This patch makes a copy of the options object before altering it.


cc @nodejs/fs

@thefourtheye thefourtheye added the fs Issues and PRs related to the fs subsystem / file system. label Jul 22, 2016
@thefourtheye
Copy link
Contributor Author

This is a fix for #7655. @micnic @thealphanerd This can be safely backported, if necessary.

@micnic
Copy link
Contributor

micnic commented Jul 22, 2016

LGTM with some comments

As I see, in some fs stream prototype constructors Object.create() is used instead of utils._extend(), it would be good to use a single object properties copying method

About tests, it would be good to have such tests for other methods that receive as parameters user defined objects, cc @nodejs/testing

);

assert.doesNotThrow(() =>
fs.appendFile(1, 'ABCD', Object.freeze({flag: 'w'}), (e) => assert.ifError(e))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add common.mustCall() to the callback.

@thefourtheye
Copy link
Contributor Author

@micnic @cjihrig Comments addressed. PTAL.

@@ -1377,11 +1377,11 @@ fs.appendFileSync = function(path, data, options) {
throwOptionsError(options);
}

if (!options.flag)
options = util._extend({ flag: 'a' }, options);
// don't make changes directly on options object
Copy link
Member

@jasnell jasnell Jul 26, 2016

Choose a reason for hiding this comment

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

minor nit... s/don't/Don't
(there are multiple comments in this doc that start with lower case that should be uppercase)

@jasnell
Copy link
Member

jasnell commented Jul 26, 2016

LGTM

@thefourtheye
Copy link
Contributor Author

cc @nodejs/collaborators

@micnic
Copy link
Contributor

micnic commented Jul 27, 2016

LGTM

@jasnell
Copy link
Member

jasnell commented Jul 29, 2016

@yorkie
Copy link
Contributor

yorkie commented Jul 30, 2016

Object.assign might be better than util._extend which would be deprecated by the former one day?

@micnic
Copy link
Contributor

micnic commented Jul 30, 2016

@yorkie from #7208 and #7255 it was decided to use the util._extend() as it has better performance than Object.assign()

@ChALkeR
Copy link
Member

ChALkeR commented Jul 31, 2016

@thefourtheye Why does this remove Object.create() from tests? Does it break with Object.create()?

@ChALkeR
Copy link
Member

ChALkeR commented Jul 31, 2016

> Object.create(Object.create({encoding: 'utf8'})).encoding
'utf8'
> util._extend({}, Object.create({encoding: 'utf8'})).encoding
undefined

Doesn't it make this semver-major?

@yorkie
Copy link
Contributor

yorkie commented Jul 31, 2016

Why does this remove Object.create() from tests? Does it break with Object.create()?

@ChALkeR because the Object.create takes its arguments into prototype chain, not the direct field, but the Object.keys that the util._extend is internally using does not return keys from prototype chain as below:

The Object.keys() method returns an array of a given object's own enumerable properties, in the same order as that provided by a for...in loop (the difference being that a for-in loop enumerates properties in the prototype chain as well).

@ChALkeR
Copy link
Member

ChALkeR commented Jul 31, 2016

@yorkie, so, this means that it actually broke those tests?

@yorkie
Copy link
Contributor

yorkie commented Jul 31, 2016

so, this means that it actually broke those tests?

Yea, we have to change those cases IMO :-(

@ChALkeR ChALkeR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 31, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Jul 31, 2016

Labeling as semver-major due to a breaking change. Note that it also changes existing tests expectations.

Feel free to remove the semver-major label if this will be changed so that all the existing tests pass.

Could we keep support for those somehow? Btw, this potentially could break some modules out there, so I'm not even sure if landing this in the current state to 7.0 without a deprecation cycle will be fine. Refs: #7912.

Note: it's not semver-major due to the actual intended change that is described in the commit title, it's semver-major because passing something like Object.create({encoding: 'utf8'}) worked before and doesn't work now.

@jasnell
Copy link
Member

jasnell commented Aug 8, 2016

@nodejs/ctc ... any further thoughts on this?
@thefourtheye ... the PR needs to be rebased and updated.

@silverwind
Copy link
Contributor

What is this options = Object.create(options) trying to achieve anyways?

@thefourtheye
Copy link
Contributor Author

@jasnell I think I am blocked by #8006. I'll rebase and update the PR once I can test this locally.

@thefourtheye
Copy link
Contributor Author

thefourtheye commented Aug 9, 2016

@silverwind We add few properties to options object passed by users, in few of the fs APIs. Object.create(options) would avoid mutating the original options object passed by user.

@claudiorodriguez
Copy link
Contributor

claudiorodriguez commented Aug 9, 2016

@thefourtheye Wouldn't something like:

const options = { a: { b: 1 } };
const newOptions = Object.create(options);
newOptions.a.b = 2;

still alter the original options object?

@mhdawson
Copy link
Member

mhdawson commented Oct 5, 2016

The status post must be wrong as there was a failure on aix even though the check above show as green. I assume this was not seen earlier as the test did not make it that far.

not ok 338 parallel/test-fs-options-immutable
# events.js:160
#       throw er; // Unhandled 'error' event
#       ^
# 
# Error: ENOENT: no such file or directory, open '/home/iojs/node-tmp/tmp.0/streams'
  ---

@mhdawson
Copy link
Member

mhdawson commented Oct 5, 2016

Hmm, see it passed in an earlier CI run so not sure why that would fail unless there could be some conflict with temp directory generation and other tests running in parallel ?

Failure on PPC was unrelated as its related to the know test-tic-processor-issues covered in: #8725

@mhdawson
Copy link
Member

mhdawson commented Oct 5, 2016

@mhdawson
Copy link
Member

mhdawson commented Oct 5, 2016

Seems to fail consistently in stress run, did the test change since the successful CI run ?

@mhdawson
Copy link
Member

mhdawson commented Oct 6, 2016

Investigating locally, believe it may be because the test assumes the write will complete before the read starts which is not guaranteed

@mhdawson
Copy link
Member

mhdawson commented Oct 6, 2016

Yes believe it is a race condition in the test. This version passes reliably:

{
  const fileName = path.resolve(common.tmpDir, 'streams');
  assert.doesNotThrow(() =>
    fs.WriteStream(fileName, options).once('open', () => {
      assert.doesNotThrow(() => fs.ReadStream(fileName, options))
    })
  );
}

@thefourtheye
Copy link
Contributor Author

Awesome. Thanks for digging deeper and providing a fix as well, @mhdawson :-)

https://ci.nodejs.org/job/node-stress-single-test/989/ is Green.

One last CI Run before landing: https://ci.nodejs.org/job/node-test-pull-request/4437/

This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: nodejs#7831
Fixes: nodejs#7655
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@thefourtheye
Copy link
Contributor Author

thefourtheye commented Oct 9, 2016

I had to rebase because of the linter rule upgrade. New CI: https://ci.nodejs.org/job/node-test-pull-request/4440/

@thefourtheye
Copy link
Contributor Author

FreeBSD failures are not related. Landing this now...

@thefourtheye
Copy link
Contributor Author

Landed in 7542bdd

@thefourtheye thefourtheye deleted the dont-alter-options branch October 9, 2016 14:09
thefourtheye added a commit that referenced this pull request Oct 9, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: #7831
Fixes: #7655
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

This does not land cleanly on v7.x-staging without the semver-major #7165 also applied.

jasnell pushed a commit that referenced this pull request Oct 10, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: #7831
Fixes: #7655
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@thefourtheye
Copy link
Contributor Author

@jasnell Oh, do you want me to PR this targeting 7.x?

@jasnell
Copy link
Member

jasnell commented Oct 10, 2016

No no, sorry i wasn't clear. Both prs landed in v7.x-staging, I was just
documenting the dependency between them.

On Monday, October 10, 2016, Sakthipriyan Vairamani <
notifications@github.com> wrote:

@jasnell https://github.com/jasnell Oh, do you want me to PR this
targeting 7.x?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7831 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAa2eSvVQzKU4UWg9r8dMba0kGOutXNmks5qymQugaJpZM4JSa-n
.

@Fishrock123
Copy link
Contributor

Backport PRs will be necessary if desired, this appears to depend on #7165 / I don't know how to resolve easily.

kevinoid pushed a commit to kevinoid/fs-file-sync-fd that referenced this pull request Dec 13, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: nodejs/node#7831
Fixes: nodejs/node#7655
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
kevinoid pushed a commit to kevinoid/fs-file-sync-fd that referenced this pull request Dec 13, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: nodejs/node#7831
Fixes: nodejs/node#7655
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
kevinoid pushed a commit to kevinoid/fs-file-sync-fd that referenced this pull request Dec 13, 2016
This patch makes a copy of the `options` object before the fs module
functions alter it.

PR-URL: nodejs/node#7831
Fixes: nodejs/node#7655
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Nicu Micleușanu <micnic90@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system.
Projects
None yet
Development

Successfully merging this pull request may close these issues.