fs: migrate more errors to internal/errors by jasnell · Pull Request #17667 · nodejs/node

jasnell · 2017-12-14T01:01:07Z

Next batch of fs type checking migrations to use internal/errors

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

Affected core subsystem(s)

fs

jasnell · 2017-12-14T01:03:13Z

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

Trott · 2017-12-14T06:08:01Z

I suspect this principle has been decided previously, but maybe not?:
Is it desirable to have such specific codes? Why not just ERR_INVALID_ARG_VALUE?

it's been debated and I'm good with it either way.

I'd be in favor of erring on the side of more general and you can always move to a more specific code later if it turns out it's really needed. But yeah, if there's fierce opinions on both sides, then ¯\(ツ)/¯

starkwang · 2017-12-14T07:37:26Z

ERR_OUT_OF_RANGE is reduced into ERR_VALUE_OUT_OF_RANGE in #17648. So here would be better to use ERR_VALUE_OUT_OF_RANGE.

yep, was holding off a bit to see if/when #17648 was going to land.

oh.. right, and the latest updates remove the out of range errors anyway so this is no longer relevant ;-)

joyeecheung · 2017-12-15T03:56:34Z

It would justify this new error a bit more if the error message includes a list of valid symlink types

jasnell · 2017-12-18T18:27:59Z

ping @nodejs/tsc

jasnell · 2017-12-18T18:55:10Z

Chose to go with @joyeecheung's suggestion of a more specific error message. PTAL

joyeecheung · 2017-12-19T17:36:28Z

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

jasnell · 2017-12-21T00:33:53Z

Thank you @joyeecheung :-) ...

@nodejs/tsc ... need one more TSC sign off please.

joyeecheung · 2017-12-21T06:11:32Z

Looks like there was something wrong with linux one, new linux one CI: https://ci.nodejs.org/job/node-test-commit-linuxone/11371/

targos · 2017-12-21T06:37:38Z

+ throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'path',
+ ['string', 'Buffer', 'URL']);
+ }
+ if (!Number.isInteger(uid))


Should we also check for uid >= 0? Same question for the other checks in this commit

I do believe that uid and gid both can be negative in some edge cases.

targos · 2017-12-21T06:53:01Z


- var req = new FSReqWrap();
+ if (!Number.isInteger(fd))
+ throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number');


s/number/integer/

targos · 2017-12-21T06:53:30Z

 // fs.writeSync(fd, string[, position[, encoding]]);
 fs.writeSync = function(fd, buffer, offset, length, position) {
+ if (!Number.isInteger(fd))
+ throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'fd', 'number');


s/number/integer/

targos · 2017-12-21T06:54:02Z

targos · 2017-12-21T06:54:34Z

 [`ERR_INVALID_ARG_TYPE`]: #ERR_INVALID_ARG_TYPE
 [`EventEmitter`]: events.html#events_class_eventemitter
+[`fs.symlink()`]: fs.html#fs_symlink
+[`fs.symlinkSync()`]: fs.html#fs_symlinksymc


typo: symlinksync

targos · 2017-12-21T06:59:14Z

General question about Number.isInteger checks: the check passes for values > 2^31-1 in JS land but not in C++ land. Should we guard for this?

mcollina

LGTM with the nits already noted addresses

bnoordhuis · 2017-12-21T10:45:38Z

the check passes for values > 2^31-1 in JS land but not in C++ land

Replacing IsInt32() and IsUint32() checks with Number.isInteger() is wrong. Their JS equivalents are x === x | 0 and x === x >>> 0 respectively.

Another issue with Number.isInteger() is that users can monkey-patch it.

jasnell · 2017-12-21T18:28:35Z

Updated to address @bnoordhuis' feedback.

@mcollina and @joyeecheung ... please take another look.

jasnell · 2017-12-21T21:21:12Z

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

mcollina

LGTM with the nit above adressed.

mcollina · 2017-12-22T09:44:59Z

targos · 2017-12-22T13:57:30Z

IsInt32 (both lines)

FChown is not changed in this PR and also gets Uint32Value(). Are you sure negative uid and gid are possible?

hmm... they are definitely theoretically possible but you're right. I'll keep these as uint32s

jasnell · 2017-12-22T18:22:46Z

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

PR-URL: #17667 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell · 2017-12-22T20:53:02Z

Landed in 6100e12

targos

Belated LGTM

apapirovski · 2017-12-24T00:37:35Z

@jasnell it looks like the new assertions in src/node_file.cc are causing some CitGM issues. Would you mind having a look? https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1165/

joyeecheung · 2017-12-24T20:57:09Z

@apapirovski The CITGM issues are caused by fs.readFile and fs.exists/fs.existsSync raising assertion when the path is undefined. I have a fix coming up.

PR-URL: #17852 Refs: #17667 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell added fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. labels Dec 14, 2017

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 2b87e7d to 0e284e1Compare December 14, 2017 01:01

jasnell added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Dec 14, 2017

jasnell requested review from addaleax and joyeecheung December 14, 2017 01:02

Trott reviewed Dec 14, 2017
View reviewed changes

starkwang reviewed Dec 14, 2017
View reviewed changes

jasnell mentioned this pull request Dec 14, 2017
Extract out fs validation functions #17682
Closed
2 tasks

joyeecheung reviewed Dec 15, 2017
View reviewed changes

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 0e284e1 to c57266dCompare December 18, 2017 18:29

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 0c2cf69 to a93e157Compare December 18, 2017 19:30

jasnell requested a review from a teamDecember 19, 2017 15:34

joyeecheung approved these changes Dec 19, 2017
View reviewed changes

targos reviewed Dec 21, 2017
View reviewed changes

mcollina approved these changes Dec 21, 2017
View reviewed changes

mcollina approved these changes Dec 22, 2017
View reviewed changes

Comment threadlib/fs.js Outdated
mcollinaDec 22, 2017
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.
@jasnell ^

targos reviewed Dec 22, 2017
View reviewed changes

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 469c61a to 4e75058Compare December 22, 2017 17:18

fs: move type checking to js
a2d9bd7

jasnell force-pushed the fs-migrate-internal-errors-2 branch from 4e75058 to a2d9bd7Compare December 22, 2017 20:48

jasnell added a commit that referenced this pull request Dec 22, 2017
fs: move type checking to js
6100e12
PR-URL: #17667 Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

jasnell closed this Dec 22, 2017

targos reviewed Dec 22, 2017
View reviewed changes

joyeecheung mentioned this pull request Dec 24, 2017
fs: fix fs.readFile, fs.exists{Sync} regression caused by assertions #17852
Closed
4 tasks

apapirovski pushed a commit that referenced this pull request Dec 27, 2017
fs: validate path in fs.readFile
9ec700b
PR-URL: #17852 Refs: #17667 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

apapirovski pushed a commit that referenced this pull request Dec 27, 2017
fs: validate path in fs.exists{Sync}
71396a2
PR-URL: #17852 Refs: #17667 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Uh oh!

Uh oh!

Conversation

jasnell commented Dec 14, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

jasnell commented Dec 14, 2017

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!

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!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 18, 2017

Uh oh!

jasnell commented Dec 18, 2017

Uh oh!

joyeecheung commented Dec 19, 2017

Uh oh!

jasnell commented Dec 21, 2017

Uh oh!

joyeecheung commented Dec 21, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

targos commented Dec 21, 2017

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Dec 21, 2017

Uh oh!

jasnell commented Dec 21, 2017

Uh oh!

jasnell commented Dec 21, 2017

Uh oh!

mcollina 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!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 22, 2017

Uh oh!

jasnell commented Dec 22, 2017

Uh oh!

targos left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell commented Dec 14, 2017•
edited
Loading

joyeecheung commented Dec 21, 2017•
edited
Loading