Skip to content

fs: make ReadStream throw TypeError on NaN - #19775

Closed
ryzokuken wants to merge 1 commit into
nodejs:masterfrom
ryzokuken:fix-isnan
Closed

ryzokuken wants to merge 1 commit into
nodejs:masterfrom
ryzokuken:fix-isnan

Conversation

@ryzokuken

@ryzokuken ryzokuken commented Apr 3, 2018 •

Copy link
Copy Markdown
Contributor

Make ReadStream (and thus createReadStream) throw a TypeError signalling
towards an invalid argument type when either options.start or
options.end (or obviously, both) are set to NaN.
Also add regression tests for the same.

Fixes: #19715

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 @lpinca @BridgeAR @addaleax @anliting

Make ReadStream (and thus createReadStream) throw a TypeError signalling
towards an invalid argument type when either options.start or
options.end (or obviously, both) are set to NaN.
Also add regression tests for the same.

Fixes: nodejs#19715
@nodejs-github-bot nodejs-github-bot added the fs Issues and PRs related to file-system APIs and the fs module. label Apr 3, 2018
@ryzokuken

Copy link
Copy Markdown
Contributor Author

@addaleax @anliting please take a look at this, I think this gets all the basics covered.

@addaleax addaleax added lts-watch-v6.x author ready PRs with CI started, the required approvals, and no outstanding review comments. labels Apr 3, 2018
@addaleax

addaleax commented Apr 3, 2018

Copy link
Copy Markdown
Member

@lpinca lpinca left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thank you for bearing with us :)

@ryzokuken

Copy link
Copy Markdown
Contributor Author

The failure seems unrelated.

@ryzokuken

Copy link
Copy Markdown
Contributor Author

@addaleax is this a flaky test? I think it seems completely unrelated.

@lpinca

lpinca commented Apr 3, 2018

Copy link
Copy Markdown
Member

@ryzokuken yes it is unrelated.

@ryzokuken

Copy link
Copy Markdown
Contributor Author

@lpinca if that's the case, please land this whenever you deem fit.

Thanks.

@anliting

anliting commented Apr 3, 2018

Copy link
Copy Markdown

@ryzokuken

LGTM.

Thank you for being patient. Good bug fix.

@ryzokuken

Copy link
Copy Markdown
Contributor Author

https://ci.nodejs.org/job/node-test-commit-linux-containered/nodes=ubuntu1604_sharedlibs_fips20_x64/3377/

shows that build was successful. Why does GitHub still show it as "pending" though?

@ryzokuken

Copy link
Copy Markdown
Contributor Author

Can we land this now?

@addaleax

addaleax commented Apr 4, 2018

Copy link
Copy Markdown
Member

@ryzokuken We have a policy that we typically wait 48 hours for PRs to land, and 72 hours over weekends, so that everybody who wants to has a chance to look at it.

@ryzokuken

Copy link
Copy Markdown
Contributor Author

Oh, sorry. I already know about the policy. I thought that because this is just a continuation of #19732, it didn't need to be up for 48 hours by itself.

Cool, let's wait for a day or two then.

@addaleax

addaleax commented Apr 4, 2018

Copy link
Copy Markdown
Member

@ryzokuken I don’t think we need to hurry on this

Also, this is adding an error throw with the intention of landing it on LTS releases, which is something that people might feel uncomfortable with (I don’t in this case)

@ryzokuken

Copy link
Copy Markdown
Contributor Author

@addaleax I understand. Thanks for being patient with me 😅

@Trott

Trott commented Apr 6, 2018

Copy link
Copy Markdown
Member

Re-running sole CI failed task node-test-commit-plinux: https://ci.nodejs.org/job/node-test-commit-plinux/16650/

@lpinca

lpinca commented Apr 6, 2018

Copy link
Copy Markdown
Member

Landed in 38a6929.

lpinca pushed a commit that referenced this pull request Apr 6, 2018
Make ReadStream (and thus createReadStream) throw a TypeError signalling
towards an invalid argument type when either options.start or
options.end (or obviously, both) are set to NaN.
Also add regression tests for the same.

PR-URL: #19775
Fixes: #19715
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@lpinca lpinca closed this Apr 6, 2018
@targos targos removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Apr 6, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request May 1, 2018
Make ReadStream (and thus createReadStream) throw a TypeError signalling
towards an invalid argument type when either options.start or
options.end (or obviously, both) are set to NaN.
Also add regression tests for the same.

PR-URL: nodejs#19775
Fixes: nodejs#19715
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 file-system APIs and the fs module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fs.createReadStream doesn't throw with options={start:NaN,end:NaN}