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.

Sign up for GitHub

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: make ReadStream throw error on NaN #19732

Closed
wants to merge 8 commits into from
Closed

fs: make ReadStream throw error on NaN #19732

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
473dd3d
fs: make ReadStream throw error on NaN
ryzokuken Apr 1, 2018
83f478e
fs, test: add tests for #19732
ryzokuken Apr 1, 2018
a4077ea
Resolve nits and fix failures (hopefully)
ryzokuken Apr 2, 2018
1ff2f51
Remove redundant condition
ryzokuken Apr 2, 2018
23cc8c0
Add an else block
ryzokuken Apr 2, 2018
0ae768d
Address concerns
ryzokuken Apr 2, 2018
163f911
Improve errors and tests
ryzokuken Apr 2, 2018
c49c543
Incorporate required changes
ryzokuken Apr 3, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2008,12 +2008,12 @@ function ReadStream(path, options) {
this.closed = false;

if (this.start !== undefined) {
if (typeof this.start !== 'number') {
if (typeof this.start !== 'number' || Number.isNaN(this.start)) {
Copy link
Member

Choose a reason for hiding this comment

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

As suggested by @anliting we can probably use just Number.isInteger().

throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start);
Copy link

Choose a reason for hiding this comment

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

Would throw new ERR_INVALID_ARG_TYPE('start', 'integer', this.start); be more appropriate? Since typeof NaN=='number'.

}
if (this.end === undefined) {
this.end = Infinity;
} else if (typeof this.end !== 'number') {
} else if (typeof this.end !== 'number' || Number.isNaN(this.end)) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you add this for the other typeof this.end !== 'number' check below as well? The one that is supposed to be merged with this one? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax Sure! On it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax done.

throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end);
}

Expand All @@ -2028,7 +2028,7 @@ function ReadStream(path, options) {
// Backwards compatibility: Make sure `end` is a number regardless of `start`.
// TODO(addaleax): Make the above typecheck not depend on `start` instead.
// (That is a semver-major change).
if (typeof this.end !== 'number')
if (typeof this.end !== 'number' || Number.isNaN(this.end))
this.end = Infinity;

if (typeof this.fd !== 'number')
Copy link
Member

Choose a reason for hiding this comment

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

This should not be included in the else block. The if / else is only about this.start and this.end and nothing else.

Copy link

Choose a reason for hiding this comment

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

Yes, it should not.

Expand Down
12 changes: 11 additions & 1 deletion test/parallel/test-fs-read-stream-throw-type-error.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
'use strict';
const common = require('../common');
const fixtures = require('../common/fixtures');

// This test ensures that fs.createReadStream throws a TypeError when invalid
// arguments are passed to it.

const fs = require('fs');

const example = fixtures.path('x.txt');
Expand All @@ -18,10 +22,16 @@ const createReadStreamErr = (path, opt) => {
{
code: 'ERR_INVALID_ARG_TYPE',
type: TypeError
});
Copy link
Member

Choose a reason for hiding this comment

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

Nit: unnecessary change.

}
);
};

createReadStreamErr(example, 123);
createReadStreamErr(example, 0);
createReadStreamErr(example, true);
createReadStreamErr(example, false);

// Should also throw on NaN (for https://github.com/nodejs/node/pull/19732)
createReadStreamErr(example, { start: NaN });
Copy link

Choose a reason for hiding this comment

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

I think we can leave only createReadStreamErr(example, { start: NaN }), because if start is not passed, end is not defined (https://github.com/ryzokuken/node/blob/473dd3d03fe4d08445518f9ab8e2b78892c22e27/lib/fs.js#L2010).

And at least we need to delete createReadStreamErr(example, { end: NaN }) to pass node-test-commit-linux.

Copy link
Member

Choose a reason for hiding this comment

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

@anliting If start is not passed but end is, then the value is still set and type-checked: https://github.com/ryzokuken/node/blob/473dd3d03fe4d08445518f9ab8e2b78892c22e27/lib/fs.js#L2031-L2032

Copy link

Choose a reason for hiding this comment

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

If we are going to test the end part, we need have start to pass the range test first: createReadStreamErr(example, { start: 0, end: NaN })

Copy link
Member

Choose a reason for hiding this comment

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

@anliting That would take a different branch, though?

Copy link

@anliting anliting Apr 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax

Oh, I see that line (which makes end is still set and type-checked) now, my fault.

"If we are going to test the end part ..." means if we are going to test if this line work:
https://github.com/ryzokuken/node/blob/473dd3d03fe4d08445518f9ab8e2b78892c22e27/lib/fs.js#L2016

Copy link

@anliting anliting Apr 1, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

We might need to change

  if (typeof this.end !== 'number' || Number.isNaN(this.end))
    this.end = Infinity;

to

  if (typeof this.end !== 'number')
    this.end = Infinity;
  else if (Number.isNaN(this.end))
    throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end);

to pass node-test-commit-linux.

Copy link
Member

Choose a reason for hiding this comment

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

@anliting Ah, right! Yes, we should probably do that.

createReadStreamErr(example, { end: NaN });
createReadStreamErr(example, { start: NaN, end: NaN });
Copy link
Member

Choose a reason for hiding this comment

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

Please add some tests with Infinity as value.

Copy link

@anliting anliting Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax Should the case {start:0,end:Infinity} throw (according to doc), or not (maybe for backward compatibility)?

Copy link
Member

Choose a reason for hiding this comment

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

@anliting I think Infinity should be a valid value for end, yes… It should o the same as passing in end: undefined (or no end: value at all), but removing support for Infinity as a value is probably too much of a breaking change

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly what I had been thinking. Doesn't throwing an error on Infinity make this semver-major?

Copy link

Choose a reason for hiding this comment

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

My appointment on that {start:0,end:Infinity} should throw is that the documentation does not allow passing Infinity as end (doc says end is an integer); no support is being removed, since it is never supported. In this point of view, "not throwing on {start:0,end:Infinity}" is a bug, and the old programs that worked are just lucky.

But passing Infinity as end is not harmful at all; it is even a good feature. Maybe we can change the doc, allowing it from now, making this a semver-minor?

Copy link
Member

Choose a reason for hiding this comment

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

But passing Infinity as end is not harmful at all; it is even a good feature.

👍

Maybe we can change the doc, allowing it from now, making this a semver-minor?

It’s probably a good idea to let the docs reflect this, but it might not even have to be semver-minor, if it’s just documenting existing behaviour. (After all, this is supposed to be a bug fix PR.)

Like, you’re right that technically we’d be adding support for something that is not documented as part of the API, but with Node we have to be conservative about these things and can’t just say “X is unsupported because it’s undocumented”.

Copy link

@anliting anliting Apr 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@addaleax Okay, now I understand your position for this; I'll get used to that. XD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@addaleax so, should I also make changes to docs in this PR?

Copy link
Member

Choose a reason for hiding this comment

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

@ryzokuken You can do that, or you can do it in another PR, or you can leave it up for somebody else to document this. But preferably one of the first two options :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

option 2 sounds perfect. Let me address all the concerns regarding the code in this PR and I'd make another one for the documentation changes.