fs: improve argument handling for ReadStream by ryzokuken · Pull Request #19898 · nodejs/node · GitHub

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 Numbers	✘	ERR_OUT_OF_RANGE
Fractional Numbers	✘	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

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

@nodejs/fs

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.

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.

@thefourtheye I think that does not apply here.

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

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

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

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

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

@ryzokuken

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

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

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

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.

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

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.

@addaleax better now?

Running tests locally. Please run CI whenever you can.

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

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.

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

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

@addaleax How about this instead?

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.

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.

@addaleax hopefully, it should work perfectly now.

LGTM! :)

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.

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

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

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

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

Wait, the lint errors are still here.

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

Okay, tests pass.

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

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

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

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

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?

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

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

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

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

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

@BridgeAR sounds great! Thanks.

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

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 outstanding review comments, 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 outstanding review comments, 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 outstanding review comments, 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 by ryzokuken · Pull Request #19898 · nodejs/node · GitHub

Uh oh!

Uh oh!

fs: improve argument handling for ReadStream - #19898

fs: improve argument handling for ReadStream#19898ryzokuken wants to merge 26 commits intonodejs:masternodejs/node:masterfrom ryzokuken:feature/create-read-streamryzokuken/node:feature/create-read-streamCopy head branch name to clipboard

Conversation

ryzokuken commented Apr 9, 2018

Checklist

Uh oh!

ryzokuken commented Apr 9, 2018

Uh oh!

Trott commented Apr 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tniessen commented Apr 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Apr 9, 2018

Uh oh!

ryzokuken commented Apr 10, 2018

Uh oh!

ronkorving commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryzokuken commented Apr 10, 2018

Uh oh!

anliting commented Apr 10, 2018

Uh oh!

anliting commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryzokuken commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anliting commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ryzokuken commented Apr 10, 2018

Uh oh!

ryzokuken commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryzokuken commented Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryzokuken Apr 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryzokuken commented Apr 10, 2018

Uh oh!

ryzokuken commented Apr 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryzokuken commented Apr 10, 2018

fs: improve argument handling for ReadStream#19898
ryzokuken wants to merge 26 commits into
nodejs:masterfrom
ryzokuken:feature/create-read-stream

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