fs: improve argument handling for ReadStream #19898

ryzokuken · 2018-04-09T17:35:01Z

Improve handling of erratic arguments in fs.ReadStream

Refs: #19732

For the uninitiated, a summary of what I'm planning to change:

Type	Current Behavior	Proposed Behavior
`undefined`	(0, `Infinity`)	(0, `Infinity`)
Anything other than `Number`	`ERR_INVALID_ARG_TYPE`	`ERR_INVALID_ARG_TYPE`
`NaN`	`ERR_INVALID_ARG_TYPE`	`ERR_OUT_OF_RANGE`
Negative `Number`s	✘	`ERR_OUT_OF_RANGE`
Fractional `Number`s	✘	`Math.round`
Integers	Work Perfectly	Still work perfectly
Invalid Arguments	`ERR_OUT_OF_RANGE`	`ERR_OUT_OF_RANGE`

Checklist

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

cc @addaleax @anliting @nodejs/fs

ryzokuken · 2018-04-09T17:36:47Z

P.S. Don't run the tests yet, they'd probably need to be changed as this should probably be semver-major, as we're now throwing on values we previously accepted.

Just take a look at the code and let me know what you think about it (for now).

Trott · 2018-04-09T18:20:09Z

@nodejs/fs

thefourtheye · 2018-04-09T18:58:10Z

lib/fs.js

+
+  // Case 4: If either start or end is fractional, round them to the nearest
+  // whole number. (Not integer as negatives not allowed)
+  if (!Number.isInteger(this.start)) {


Now that we are doing much stricter checks, isSafeInteger would be better I guess.

@thefourtheye I think that does not apply here.

@thefourtheye I think that does not too.

tniessen · 2018-04-09T19:51:14Z

Is there a reason to make NaN be ERR_INVALID_ARG_TYPE instead of ERR_OUT_OF_RANGE? I would expect "Invalid type" if a type mismatches according to typeof or according to class / interface expectations, both don't apply here (typeof NaN === 'number'). From a practical point of view, NaN is most probably the result of an incorrect computation (e.g. Math.sqrt(-1)), and in such cases ERR_OUT_OF_RANGE might be easier to understand.

BridgeAR · 2018-04-09T21:00:23Z

lib/fs.js

+
+  // Case 4: If either start or end is fractional, round them to the nearest
+  // whole number. (Not integer as negatives not allowed)
+  if (!Number.isInteger(this.start)) {


@thefourtheye I think that does not apply here.

BridgeAR · 2018-04-09T23:33:07Z

@tniessen I think you misread the change. As far as I see it, your suggestion is already the case in this PR.

ryzokuken · 2018-04-10T00:55:43Z

@tniessen @BridgeAR exactly. ERR_INVALID_ARG_TYPE is the error currently being thrown, and this PR switches to ERR_OUT_OF_RANGE, completely incorporating your suggestion.

ronkorving · 2018-04-10T01:20:16Z

@ryzokuken What's the reasoning behind rounding floats? To me, a floating point number implies the user is not passing what they intended to pass. Rather than rounding it (ie: magic), I personally would prefer it to throw.

ryzokuken · 2018-04-10T02:07:07Z

@ronkorving I had something similar in mind, but Anna convinced me otherwise and with good reason. A lot of people do a ton of math to get the values of start and end, and things might not go exactly as planned.

Quoting @addaleax here:

When your math gives you 12.9999998, which should have turned out to 13 if it weren’t for floating-point inaccuracies, and we round down to 12, then all your offsets into the returned buffers are wrong.

For a better insight into our conversations and how we came to this final decision, read the thread at the reference PR (#19732).

anliting · 2018-04-10T02:55:21Z

@ryzokuken

I haven't reviewed the code because I am a little busy these days.

Would you like to write/quote the reason of using Math.round etc. in the code? I used to do that in my own projects to prevent good people from wasting creative mind on checking the code.

anliting · 2018-04-10T05:17:06Z

@ryzokuken

I am wondering if (Infinity,Infinity) breaks ReadStream......

ryzokuken · 2018-04-10T05:38:26Z

@anliting No idea. We could test for that, though. I'll be adding/updating unit tests for ReadStream today.

anliting · 2018-04-10T05:47:17Z

@ryzokuken

I mean, you program seems to allow passing Infinity for start, but I am not sure if the other parts will break on this.start==Infinity.

ryzokuken · 2018-04-10T10:41:20Z

Added new tests and updated existing ones, take a look when you can.

The tests might look a little too much, but better be safe than be sorry, I guess.

ryzokuken · 2018-04-10T20:24:07Z

I tried running tests locally, and none of my new tests seemed to fail (which sounds good), but a seemingly related failed. Could someone help me debug it?

=== release test-fs-read-stream ===
Path: parallel/test-fs-read-stream
events.js:167
      throw er; // Unhandled 'error' event
      ^

Error: ESPIPE: invalid seek, read
Emitted 'error' event at:
    at fs.read (fs.js:2130:12)
    at FSReqWrap.wrapper [as oncomplete] (fs.js:569:17)
Command: out/Release/node /Users/ryzokuken/Code/nodejs/node/test/parallel/test-fs-read-stream.js

addaleax · 2018-04-10T20:28:56Z

lib/fs.js

-    }
+  // Case 0: If either start or end is undefined, set them to defaults
+  // (0, Infinity) respectively.
+  this.start = this.start === undefined ? 0 : this.start;


I’m mostly sure this is the cause of the test failure – start: 0 and start: undefined aren’t equivalent, because the former means that the stream will use reads with an offset parameter (which doesn’t work on all types of files). If start: undefined was passed, this value should be undefined, too.

Alright, let me just remove the condition then.

ryzokuken · 2018-04-10T20:43:10Z

@addaleax better now?

Running tests locally. Please run CI whenever you can.

addaleax

LGTM, assuming the start-related checks are grouped in a block that ignores start === undefined (or something equivalent to that)

addaleax · 2018-04-10T20:44:01Z

lib/fs.js

-    }
+  // Case 1: If the type of either start or end is not number, throw
+  // ERR_INVALID_ARG_TYPE (TypeError).
+  if (typeof this.start !== 'number') {


I think you want to allow undefined explicitly here and for the other checks as well?

(The if (this.start !== undefined) { block from the original variant of this code seems like a pretty reasonable idea, tbh…)

Okay. So, should I enclose all the type-checking code within that statement?

@ryzokuken All that’s related to start, yes. end is another story, and my original TODO was more or less about keeping the end checks out of such a block :)

~~Exactly. That's why I wanted to keep the checks separate, so they worked either way.~~

~~I've enclosed all this code into the if block, which should work if end does not need to be transformed or checked at all if start is undefined. That probably isn't the case, is it?~~

~~P.S. How should I handle end if start is undefined? Similar to how it was handled earlier?~~

Edit: Sorry, misunderstood you. Working on it. Hopefully, it will work now.

ryzokuken · 2018-04-10T20:46:50Z

Umm, okay. So a couple of other ReadStream related tests throw now.

The arguments passed don't qualify by the newer standards, I guess.

ryzokuken · 2018-04-10T21:05:39Z

@addaleax How about this instead?

addaleax · 2018-04-10T21:09:48Z

lib/fs.js

+    }
+    if (!Number.isInteger(this.end)) {
+      this.end = Math.round(this.end);
+    }


I guess we would want to do the type checking for end before we get to this block? Otherwise end: undefined would get converted to Math.round(undefined) here, which is NaN… if I read the code correctly?

Holy moly. I missed this one and got a few failing tests for NaN. Stupid me, I guess. Thanks for pointing this out.

addaleax · 2018-04-10T21:10:07Z

lib/fs.js

    }

+    // Case 4: If start is fractional, round them to the nearest whole number.
+    // (Not integer as negatives not allowed)


I’m a bit confused by the comment here – we do round to the nearest integer, right?

We do. But because we've already thrown on negatives, we're only left with positive fractions, which will either round to zero or positive integers (whole numbers). I used this term because it's more precise (factually).

If you want, I could remove it and just say that we're rounding to the nearest Integer.

ryzokuken · 2018-04-10T21:20:25Z

@addaleax hopefully, it should work perfectly now.

addaleax

LGTM! :)

addaleax · 2018-04-10T21:28:07Z

lib/fs.js

    }

+    // Case 5: If start is a whole number, work perfectly.
+
+    // Case 6: If start is greater than end, throw ERR_OUT_OF_RANGE (RangeError)
    if (this.start > this.end) {


I think this works fine, but as far as I can tell that is because end: undefined gets coerced to NaN here (which makes this condition always false), and that the fact that we later round end does not affect this comparison

Maybe it would be a bit more obvious that this works if we swap type checking for end and start?

Umm, I couldn't understand what you meant. Should I move the end block to check first and then check for start?

end isn't being coerced to NaN before this anymore, but end does get rounded off later on in the code.

@ryzokuken What I meant was, end is being coerced to NaN in this comparison if it was passed in as undefined, because we haven’t yet gotten to the part where it’s converted to Infinity.

Oh. so, should I move the block that handles this.end === undefined first and move to a similar type-check below?

Let me make it and demonstrate.

ryzokuken · 2018-04-10T21:34:03Z

Okay, looks like the tests passed for me this time around, so that's probably a good sign.

ryzokuken · 2018-04-10T21:39:35Z

@addaleax it does feel much better now, doesn't it? Definitely more consistent (start !== undefined vs end !== undefined).

ryzokuken · 2018-05-09T14:33:13Z

@BridgeAR you're right, only two files had an hardcoded check for that test and I had also updated one. Running tests in a jiffy.

ryzokuken · 2018-05-09T14:33:50Z

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

ryzokuken · 2018-05-09T14:38:55Z

Wait, the lint errors are still here.

ryzokuken · 2018-05-09T14:42:09Z

Broke down the three lines that were failing on us, all tests should pass now. Waiting for tests to pass locally first this time.

ryzokuken · 2018-05-09T14:50:18Z

Okay, tests pass.

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

ryzokuken · 2018-05-09T19:23:37Z

Seems to be an unrelated error with https://ci.nodejs.org/job/node-test-binary-windows/17146/, rerunning.

ryzokuken · 2018-05-09T19:24:38Z

Binary Windows Test: https://ci.nodejs.org/job/node-test-binary-windows/17161/

ryzokuken · 2018-05-09T20:08:16Z

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

ryzokuken · 2018-05-09T20:45:45Z

Another Java error: https://ci.nodejs.org/job/node-test-commit-freebsd/17632/nodes=freebsd10-64/console

Aren't we having too many of these lately?

ryzokuken · 2018-05-10T10:11:30Z

Optimistic CI run: https://ci.nodejs.org/job/node-test-pull-request/14780/

ryzokuken · 2018-05-10T16:57:17Z

Rerunning: https://ci.nodejs.org/job/node-test-pull-request/14797/

BridgeAR · 2018-05-10T17:24:09Z

@ryzokuken the CI does not have to be "green green". All tests related to this PR pass and what you encounter otherwise are flakes. Some are infrastructure issues and some times some tests are not written well enough. That is normal but it does not mean this PR needs any more work. It can land as it is.

ryzokuken · 2018-05-10T17:37:34Z

@BridgeAR so, should I land it? Any CITGM or benchmark runs required?

BridgeAR · 2018-05-10T17:56:08Z

@ryzokuken running CITGM for semver-majors is always best to detect failures early that have to be fixed. I doubt that it is going to be an issue in this case but here they are nevertheless:

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1404/
CITGM master https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1405/

When they come out "green" (there will be quite a few failures and therefore it is easier to compare the output of those two. If they diverge, check if it has something to do with this PR) you should feel free to land the PR.

ryzokuken · 2018-05-10T18:02:39Z

@BridgeAR sounds great! Thanks.

ryzokuken · 2018-05-10T21:31:59Z

@BridgeAR not the exact same, but mine had a few less failures? (59 vs 66).

Also, a single less skipped test. I hope it's okay to land?

addaleax · 2018-05-14T17:07:28Z

Landed in 19374fd 🎉

Improve handling of erratic arguments in fs.ReadStream Refs: #19732 PR-URL: #19898 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Apr 9, 2018

thefourtheye reviewed Apr 9, 2018

View reviewed changes

jasnell approved these changes Apr 9, 2018

View reviewed changes

BridgeAR approved these changes Apr 9, 2018

View reviewed changes

BridgeAR added semver-major PRs that contain breaking changes and should be released in the next major version. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Apr 9, 2018

addaleax reviewed Apr 10, 2018

View reviewed changes

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 10, 2018

addaleax approved these changes Apr 10, 2018

View reviewed changes

addaleax reviewed Apr 10, 2018

View reviewed changes

addaleax approved these changes Apr 10, 2018

View reviewed changes

Update RangeError error message v2

9af225c

Fix lint errors

c76e75a

addaleax closed this May 14, 2018

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 12, 2018

targos mentioned this pull request Jul 12, 2018

test: check option start or end is not safe integer #21704

Closed

2 tasks

fs: improve argument handling for ReadStream #19898

fs: improve argument handling for ReadStream #19898

Conversation

ryzokuken commented Apr 9, 2018

Checklist

ryzokuken commented Apr 9, 2018

Trott commented Apr 9, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tniessen commented Apr 9, 2018

Choose a reason for hiding this comment

BridgeAR commented Apr 9, 2018

ryzokuken commented Apr 10, 2018

ronkorving commented Apr 10, 2018 • edited Loading

ryzokuken commented Apr 10, 2018

anliting commented Apr 10, 2018

anliting commented Apr 10, 2018 • edited Loading

ryzokuken commented Apr 10, 2018 • edited Loading

anliting commented Apr 10, 2018 • edited Loading

ryzokuken commented Apr 10, 2018

ryzokuken commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Apr 10, 2018 • edited Loading

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken Apr 10, 2018 • edited Loading

Choose a reason for hiding this comment

ryzokuken commented Apr 10, 2018

ryzokuken commented Apr 10, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Apr 10, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzokuken commented Apr 10, 2018

ryzokuken commented Apr 10, 2018

ryzokuken commented May 9, 2018

ryzokuken commented May 9, 2018

ryzokuken commented May 9, 2018

ryzokuken commented May 9, 2018

ryzokuken commented May 9, 2018

ryzokuken commented May 9, 2018

ryzokuken commented May 9, 2018

ryzokuken commented May 9, 2018

ryzokuken commented May 9, 2018

ryzokuken commented May 10, 2018

ryzokuken commented May 10, 2018

BridgeAR commented May 10, 2018

ryzokuken commented May 10, 2018

BridgeAR commented May 10, 2018

ryzokuken commented May 10, 2018

ryzokuken commented May 10, 2018

addaleax commented May 14, 2018

ronkorving commented Apr 10, 2018 •

edited

Loading

anliting commented Apr 10, 2018 •

edited

Loading

ryzokuken commented Apr 10, 2018 •

edited

Loading

anliting commented Apr 10, 2018 •

edited

Loading

ryzokuken commented Apr 10, 2018 •

edited

Loading

ryzokuken Apr 10, 2018 •

edited

Loading