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 7 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
23 changes: 14 additions & 9 deletions lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2009,12 +2009,15 @@ function ReadStream(path, options) {

if (this.start !== undefined) {
if (typeof this.start !== 'number') {
throw new ERR_INVALID_ARG_TYPE('start', 'number', this.start);
throw new ERR_INVALID_ARG_TYPE('options.start', 'number', options.start);
}
if (!Number.isInteger(this.start)) {
throw new ERR_OUT_OF_RANGE('options.start', 'an integer', options.start);
Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I guess I expressed myself not well enough in my last comment. What I meant is that you can move the type check inside of the Number.isInteger check. That way it will only do the type check in the negative case.

As in:

if (!Number.isInteger(this.start)) {
  if (typeof this.start !== 'number') {
    throw new ERR_INVALID_ARG_TYPE(...);
  }
  throw new ERR_OUT_OF_RANGE(...);
}

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.

No, I was wrong. @BridgeAR is faster when Number.isInteger(this.start).

----EDITED----

I think his approach is okay, maybe faster.

With

if (!Number.isInteger(this.start)) {
  if (typeof this.start !== 'number') {
    throw new ERR_INVALID_ARG_TYPE(...);
  }
  throw new ERR_OUT_OF_RANGE(...);
}

, both ERR_INVALID_ARG_TYPE and ERR_OUT_OF_RANGE cases run 2 checks to be reached; while with

if (typeof this.start !== 'number') {
  throw new ERR_INVALID_ARG_TYPE(...);
}
if (!Number.isInteger(this.start)) {
  throw new ERR_OUT_OF_RANGE(...);
}

, ERR_INVALID_ARG_TYPE runs 1 check to be reached, and ERR_INVALID_ARG_TYPE runs 2.

}
if (this.end === undefined) {
this.end = Infinity;
} else if (typeof this.end !== 'number') {
throw new ERR_INVALID_ARG_TYPE('end', 'number', this.end);
} else if (!Number.isInteger(this.end)) {
throw new ERR_INVALID_ARG_TYPE('end', 'integer', 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.

The same as above: distinguishing the type from the rest of the error would be good. You can actually copy the same part and just change what variable it applies to.

Copy link

Choose a reason for hiding this comment

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

For example, change

    if (this.end === undefined) {
      this.end = Infinity;
    } else if (!Number.isInteger(this.end)) {
      throw new ERR_INVALID_ARG_TYPE('end', 'integer', this.end);
    }

to

    if (this.end === undefined) {
      this.end = Infinity;
    }else{
      if(typeof this.end!=='number'){
        throw new ERR_INVALID_ARG_TYPE('end','number',this.end)
      }
      if (!Number.isInteger(this.end)) {
        throw new ERR_OUT_OF_RANGE('end', 'integer', this.end);
      }
    }

.

}

if (this.start > this.end) {
Expand All @@ -2023,13 +2026,15 @@ function ReadStream(path, options) {
}

this.pos = this.start;
}

// 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')
} else if (typeof this.end !== 'number') {
// 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).
this.end = Infinity;
} else if (!Number.isInteger(this.end)) {
Copy link

Choose a reason for hiding this comment

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

As we decided to support {end:Infinity}, change else if (!Number.isInteger(this.end)) to else if (!(Number.isInteger(this.end)||this.end===Infinity)).

Copy link

Choose a reason for hiding this comment

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

Also change line 2019. (Comments are not allowed there.)

throw new ERR_OUT_OF_RANGE('options.end', 'an integer', options.end);
}

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.

this.open();
Expand Down
21 changes: 15 additions & 6 deletions 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 @@ -10,18 +14,23 @@ fs.createReadStream(example, null);
fs.createReadStream(example, 'utf8');
fs.createReadStream(example, { encoding: 'utf8' });

const createReadStreamErr = (path, opt) => {
const createReadStreamErr = (path, opt, errorCode) => {
common.expectsError(
() => {
fs.createReadStream(path, opt);
},
{
code: 'ERR_INVALID_ARG_TYPE',
code: errorCode,
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);
createReadStreamErr(example, 123, 'ERR_INVALID_ARG_TYPE');
createReadStreamErr(example, 0, 'ERR_INVALID_ARG_TYPE');
createReadStreamErr(example, true, 'ERR_INVALID_ARG_TYPE');
createReadStreamErr(example, false, 'ERR_INVALID_ARG_TYPE');

// Should also throw on NaN (for https://github.com/nodejs/node/pull/19732)
createReadStreamErr(example, { start: NaN }, 'ERR_OUT_OF_RANGE');
createReadStreamErr(example, { end: NaN }, 'ERR_OUT_OF_RANGE');
createReadStreamErr(example, { start: NaN, end: NaN }, 'ERR_OUT_OF_RANGE');
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 please still add tests for Infinity? :-) That is independent of making a breaking change. Setting start to Infinity will still throw right now and setting end to infinity should not throw.