fs: throw errors on invalid paths synchronously #18308

joyeecheung · 2018-01-23T02:37:27Z

Since type check errors on paths are throw synchronously, it makes more sense to throw other types of checks on the paths synchronously as well.

fs: throw errors on invalid paths synchronously

Throw getPathFromURL() and nullCheck() errors synchronously instead
of deferring them to the next tick, since we already throw
validatePath() errors synchronously.
Merge nullCheck() into validatePath()
Never throws in fs.exists(), instead, invoke the callback with
false, or emit a warning when the callback is not a function.
This is to bring it inline with fs.existsSync(), which never throws.
Updates the comment of rethrow()
Throw ERR_INVALID_ARG_VALUE for null checks

Checklist

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

Affected core subsystem(s)

fs, whatwg-url

joyeecheung · 2018-01-23T02:42:00Z

cc @jasnell
CI: https://ci.nodejs.org/job/node-test-pull-request/12679/

joyeecheung · 2018-01-23T02:44:37Z

test/parallel/test-fs-copyfile.js

@@ -101,26 +101,6 @@ common.expectsError(() => {
  );
 });

-// Throws if the source path is an invalid path.


This is removed because there are checks in test-fs-null-bytes.js

joyeecheung · 2018-01-23T02:47:10Z

lib/fs.js

@@ -204,6 +204,37 @@ function validateOffsetLengthWrite(offset, length, byteLength) {
  }
 }

+// Returns silently if path contains null bytes.


Oops, should've been Check if the path contains null types if it is a string nor Uint8Array, otherwise return silently.

gireeshpunathil · 2018-01-23T04:22:20Z

@joyeecheung - Will this cause a previously working code that received a propagated error message in the API's callback to now synchronously throw an error? I guess the decision to throw inline or async or error-flled callback may be taken based the API interface itself, rather than the type of the error we are dealing with?

joyeecheung · 2018-01-23T05:26:41Z

@gireeshpunathil But we do throw type-checking errors and range errors synchronously in almost all of our APIs at the moment, even in the async APIs, so the users should always be prepared for both types of errors even without this change. Aside from fs, it's also common in APIs like http to throw validation errors like this without passing them to callbacks - in fact fs looks like an exception in this regard.

The URL type errors and null check errors IMO are not that far from type errors and range errors, it's rather surprising that some of them are thrown asynchronously while some of them are not. Deferring them to the next tick does not seem to yield any benefit so if we are unifying the behavior, we might as well throw them synchronously.

gireeshpunathil · 2018-01-23T05:51:26Z

thanks @joyeecheung . Neither I have a full view of the existing behavior, documented evidence to support them, nor any specific concerns on this PR. But In general, I think it is a good idea for an async API to call back under all possible circumstances, except from native assertions and machine check exceptions - will you please spawn a separate thread to gather views and reach consensus?

joyeecheung · 2018-01-23T06:25:00Z

cc @nodejs/tsc since it's semver-major

mcollina

I agree with this change.

LGTM with the FIXME addressed.

mcollina · 2018-01-23T08:43:11Z

lib/fs.js

+  const err = new errors.Error('ERR_INVALID_ARG_TYPE',
+                               propName,
+                               'string without null bytes',
+                               path);


I do not like leaving FIXME in the code, can you get this sorted?

joyeecheung · 2018-01-23T10:58:34Z

@mcollina Was going to fix that later, but I am fine with fixing it now as well. Updated, PTAL.

mcollina · 2018-01-23T11:56:14Z

LGTM

joyeecheung · 2018-01-24T12:37:02Z

Rebased. New CI: https://ci.nodejs.org/job/node-test-pull-request/12706/

joyeecheung · 2018-01-24T12:53:24Z

cc @nodejs/tsc this needs at least one more approval to land, although we might want to discuss about it in a meeting.

cons:

more errors get thrown synchronously out of asynchronous APIs

pros:

This is already the case for the most ERR_INVALID_ARG_TYPE and ERR_OUT_OF_RANGE errors in fs as well as many validations in other modules. This patch just adds ERR_INVALID_FILE_URL_PATH, ERR_INVALID_URL_SCHEME , and a ERR_INVALID_ARG_VALUE to the list.
AFAICT throwing this type of errors is more prevalent than passing them down asynchronously (searching for ERR_INVALID_* and ERR_OUT_OF_RANGE and the like in the codebase would show that)
These errors are synchronous in nature, they usually happen before any asynchronous operations can even be started because the operations cannot really be initiated if the arguments are invalid.
They are kind of like ReferenceError in callbacks, TypeError in null.something, and JSON parse errors, .etc which are all thrown synchronously by the JS engine.

Also see #18308 (comment) on the special case of fs.exists and fs.existsSync.

joyeecheung · 2018-01-24T12:54:49Z

CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1228/
CITGM against this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1229/

second-guessing myself here...

joyeecheung · 2018-01-24T22:46:21Z

Updated:

Spin off the ERR_INVALID_ARG_VALUE changes in errors: improve the description of ERR_INVALID_ARG_VALUE #18358 , the first commit is taken from that.
Updated the fs.exists and fs.existsSync behavior, because unlike other sync APIs, the users have already expected fs.existSync to NEVER throw, even when it's invoked like fs.existSync(), therefore we cannot throw in the async counterpart either.
Updated the description of rethrow because we didn't throw the error in v7 (reverted), and we probably won't in the near future (fs: Revert throw on invalid callbacks #12976).

joyeecheung · 2018-01-24T22:51:17Z

cc @jasnell @mcollina Please take another look since the behavior of fs.exists and fs.existsSync are changed significantly because of https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1229/:

fs.existsSync never throws at the moment. I updated it to stay that way.
fs.exists currently throws on invalid paths, but any other errors in callbacks are coerced to false. This updates that to never throw etiher, instead it invokes the callback with false when there are invalid arguments, to be inline with fs.existsSync. Mind that the callback fs.exists does not even take an error parameter, it only takes a boolean. IMO if we want to throw from fs.exist and fs.existSync, we would need a proper deprecation cycle. I would want to defer that to future PRs.

joyeecheung · 2018-01-24T22:58:58Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12720/
CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1231/
CITGM against this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1232/

BridgeAR · 2018-02-01T14:23:17Z

lib/internal/errors.js

+  }
+  return util_;
+}
+


Using the other lazy require is faster. It is probably not much of a difference in the real world but I would still prefer to keep it as is.

BridgeAR · 2018-02-01T14:26:38Z

lib/internal/errors.js

-E('ERR_INVALID_ARG_VALUE', (name, value) =>
-  `The value "${String(value)}" is invalid for argument "${name}"`);
+E('ERR_INVALID_ARG_VALUE', (name, value, reason = 'is invalid') => {
+  const util = lazyUtil();


This is not necessary in case you assign util to a "global" (module) variable. In that case util will always be loaded before the error here can be called. That is another reason why I would prefer to keep the old lazy loading.

@BridgeAR This has already landed in master as a separate PR #18358, this PR just needs a rebase. I don't personally feel very strongly about this approach, just following the convention of lazyErrors() in other modules.

joyeecheung · 2018-02-02T03:44:27Z

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

@BridgeAR See #18308 (comment) about the lazyUtils change, can you open a separate PR if you want to restore the way we lazy load utils?

joyeecheung · 2018-02-02T10:15:16Z

CI is unstable today...launched two builds and the combination look green...

https://ci.nodejs.org/job/node-test-pull-request/12903/ (ubuntu1604-arm64_odroid_c2 and osx1010 infra issue)
https://ci.nodejs.org/job/node-test-pull-request/12904/ (ubuntu1604-arm64_odroid_c2 and ppcbe-ubuntu1404 infra issue, two known flake test-tls-server-verify and test-child-process-exec-kill-throws on windows)

joyeecheung · 2018-02-02T10:16:40Z

CITGM against master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1253/
CITGM against this PR: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1254/

joyeecheung · 2018-02-03T03:57:33Z

There don't seem to be new errors from the CITGM run. The errors that did not appear on master appear on other PRs' CITGM runs.

cc @mcollina @jasnell @gireeshpunathil @BridgeAR @Trott any more thoughts on this PR? The only significant change is that we don't throw in fs.existSync/fs.exists under any circumstances now for backwards compatibility. See #18308 (comment)

joyeecheung · 2018-02-03T03:59:20Z

Also @mcollina @jasnell @gireeshpunathil To people who have already signed-off on this, does this still LGTY? I would like to land this soon so I can continue to work on improving the async errors' stack traces.

mcollina · 2018-02-03T11:04:34Z

Still LGTM.

gireeshpunathil · 2018-02-05T05:03:12Z

still LGTM

- Throw getPathFromURL() and nullCheck() errors synchronously instead of deferring them to the next tick, since we already throw validatePath() errors synchronously. - Merge nullCheck() into validatePath() - Never throws in `fs.exists()`, instead, invoke the callback with false, or emit a warning when the callback is not a function. This is to bring it inline with fs.existsSync(), which never throws. - Updates the comment of rethrow() - Throw ERR_INVALID_ARG_VALUE for null checks

joyeecheung · 2018-02-07T10:57:54Z

CI before landing: ~~https://ci.nodejs.org/job/node-test-pull-request/13003/~~ https://ci.nodejs.org/job/node-test-pull-request/13005/

joyeecheung · 2018-02-08T10:12:54Z

CI is clean minus one infra issue. Landing..

joyeecheung · 2018-02-08T10:27:52Z

Landed in d8f7338, thanks!

- Throw getPathFromURL() and nullCheck() errors synchronously instead of deferring them to the next tick, since we already throw validatePath() errors synchronously. - Merge nullCheck() into validatePath() - Never throws in `fs.exists()`, instead, invoke the callback with false, or emit a warning when the callback is not a function. This is to bring it inline with fs.existsSync(), which never throws. - Updates the comment of rethrow() - Throw ERR_INVALID_ARG_VALUE for null checks PR-URL: #18308 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

- Throw getPathFromURL() and nullCheck() errors synchronously instead of deferring them to the next tick, since we already throw validatePath() errors synchronously. - Merge nullCheck() into validatePath() - Never throws in `fs.exists()`, instead, invoke the callback with false, or emit a warning when the callback is not a function. This is to bring it inline with fs.existsSync(), which never throws. - Updates the comment of rethrow() - Throw ERR_INVALID_ARG_VALUE for null checks PR-URL: nodejs#18308 Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

nodejs-github-bot added dont-land-on-v4.x fs Issues and PRs related to the fs subsystem / file system. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 23, 2018

joyeecheung added semver-major PRs that contain breaking changes and should be released in the next major version. and removed dont-land-on-v4.x labels Jan 23, 2018

joyeecheung requested a review from jasnell January 23, 2018 02:42

joyeecheung commented Jan 23, 2018

View reviewed changes

gireeshpunathil approved these changes Jan 23, 2018

View reviewed changes

mcollina approved these changes Jan 23, 2018

View reviewed changes

jasnell added this to the 10.0.0 milestone Jan 24, 2018

joyeecheung force-pushed the fs-sync-error-path branch from ab32b24 to 03b0f73 Compare January 24, 2018 12:35

joyeecheung added the tsc-review label Jan 24, 2018

This was referenced Jan 24, 2018

test: make sure destroy() deletes leveldb-only dir Level/leveldown#428

Merged

[[FIX]] Skip undefined env in getHomeDir() jshint/jshint#3245

Open

Trott previously approved these changes Jan 24, 2018

View reviewed changes

joyeecheung mentioned this pull request Jan 24, 2018

errors: improve the description of ERR_INVALID_ARG_VALUE #18358

Closed

3 tasks

jasnell approved these changes Jan 24, 2018

View reviewed changes

joyeecheung force-pushed the fs-sync-error-path branch 2 times, most recently from 2cf2fdb to 354c61d Compare January 24, 2018 22:43

maclover7 force-pushed the master branch from bb5575a to 993b716 Compare January 26, 2018 22:02

cjihrig force-pushed the master branch from 993b716 to 082f952 Compare January 26, 2018 22:36

BridgeAR reviewed Feb 1, 2018

View reviewed changes

joyeecheung force-pushed the fs-sync-error-path branch from 354c61d to c1702d9 Compare February 2, 2018 03:27

joyeecheung mentioned this pull request Feb 3, 2018

fs: create consistent behavior for fs module across platforms #17927

Closed

4 tasks

joyeecheung force-pushed the fs-sync-error-path branch from bcf05af to 758edd3 Compare February 7, 2018 10:57

squash! update error handling in promise APIs

76bad33

joyeecheung closed this Feb 8, 2018

Zarel mentioned this pull request Jan 14, 2019

Prevent DoS attack cloudhead/node-static#213

Closed

bnoordhuis mentioned this pull request Oct 11, 2019

fs validatePath throws instead of calling error callback #29926

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: throw errors on invalid paths synchronously #18308

fs: throw errors on invalid paths synchronously #18308

joyeecheung commented Jan 23, 2018 •

edited

Loading

joyeecheung commented Jan 23, 2018

joyeecheung Jan 23, 2018

joyeecheung Jan 23, 2018

gireeshpunathil commented Jan 23, 2018

joyeecheung commented Jan 23, 2018 •

edited

Loading

gireeshpunathil commented Jan 23, 2018

joyeecheung commented Jan 23, 2018

mcollina left a comment

mcollina Jan 23, 2018

joyeecheung commented Jan 23, 2018

mcollina commented Jan 23, 2018

joyeecheung commented Jan 24, 2018

joyeecheung commented Jan 24, 2018 •

edited

Loading

joyeecheung commented Jan 24, 2018

joyeecheung commented Jan 24, 2018

joyeecheung commented Jan 24, 2018 •

edited

Loading

joyeecheung commented Jan 24, 2018

BridgeAR Feb 1, 2018

BridgeAR Feb 1, 2018

joyeecheung Feb 2, 2018

joyeecheung commented Feb 2, 2018

joyeecheung commented Feb 2, 2018

joyeecheung commented Feb 2, 2018

joyeecheung commented Feb 3, 2018

joyeecheung commented Feb 3, 2018

mcollina commented Feb 3, 2018

gireeshpunathil commented Feb 5, 2018

joyeecheung commented Feb 7, 2018 •

edited

Loading

joyeecheung commented Feb 8, 2018

joyeecheung commented Feb 8, 2018

fs: throw errors on invalid paths synchronously #18308

fs: throw errors on invalid paths synchronously #18308

Conversation

joyeecheung commented Jan 23, 2018 • edited Loading

Checklist

Affected core subsystem(s)

joyeecheung commented Jan 23, 2018

joyeecheung Jan 23, 2018

Choose a reason for hiding this comment

joyeecheung Jan 23, 2018

Choose a reason for hiding this comment

gireeshpunathil commented Jan 23, 2018

joyeecheung commented Jan 23, 2018 • edited Loading

gireeshpunathil commented Jan 23, 2018

joyeecheung commented Jan 23, 2018

mcollina left a comment

Choose a reason for hiding this comment

mcollina Jan 23, 2018

Choose a reason for hiding this comment

joyeecheung commented Jan 23, 2018

mcollina commented Jan 23, 2018

joyeecheung commented Jan 24, 2018

joyeecheung commented Jan 24, 2018 • edited Loading

joyeecheung commented Jan 24, 2018

joyeecheung commented Jan 24, 2018

joyeecheung commented Jan 24, 2018 • edited Loading

joyeecheung commented Jan 24, 2018

BridgeAR Feb 1, 2018

Choose a reason for hiding this comment

BridgeAR Feb 1, 2018

Choose a reason for hiding this comment

joyeecheung Feb 2, 2018

Choose a reason for hiding this comment

joyeecheung commented Feb 2, 2018

joyeecheung commented Feb 2, 2018

joyeecheung commented Feb 2, 2018

joyeecheung commented Feb 3, 2018

joyeecheung commented Feb 3, 2018

mcollina commented Feb 3, 2018

gireeshpunathil commented Feb 5, 2018

joyeecheung commented Feb 7, 2018 • edited Loading

joyeecheung commented Feb 8, 2018

joyeecheung commented Feb 8, 2018

joyeecheung commented Jan 23, 2018 •

edited

Loading

joyeecheung commented Jan 23, 2018 •

edited

Loading

joyeecheung commented Jan 24, 2018 •

edited

Loading

joyeecheung commented Jan 24, 2018 •

edited

Loading

joyeecheung commented Feb 7, 2018 •

edited

Loading