fs: simplify the error context collection in C++, migrate fs.close errors #17338

joyeecheung · 2017-11-27T04:28:03Z

fs: simplify the error context collection in C++

Simplify the SyncCall template function, only collect error
number and syscall in the C++ layer and collect the rest of context
in JS for flexibility.
Remove the stringFromPath JS helper now that the unprefixed path is
directly put into the context before the binding is invoked with the
prefixed path.
Add a errno -> [error code, uv error message] map to the uv binding
so the error message can be assembled in the JS layer

fs: throw fs.close errors in JS

Check fd in JS before invoking the binding and throw
ERR_INVALID_ARG_TYPE if it's not an integer
Add a test for checking the errors thrown from fs.close and
fs.closeSync

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, test, errors

joyeecheung · 2017-11-27T04:34:53Z

test/parallel/test-fs-close-error.js

+assert.throws(
+  () => { fs.closeSync(-1); },
+  (err) => {
+    assert.strictEqual(err.code, 'EBADF');


The current error does not include any information about the fd being closed (I think it's also the same for other fs APIs that takes fd as arguments). To improve this properly we would need to update the After callback so that async APIs can be consistent with the synchronous APIs. We could add fd as an argument to UVException but IMO that's just going to bloat the signature of that function even further, putting those data in an object in JS is much more flexible.

Since After is also used by functions not migrated yet, I would like to update this bit when all the fs errors are migrated to the JS layer.

joyeecheung · 2017-11-27T04:39:03Z

~~CI: https://ci.nodejs.org/job/node-test-pull-request/11741/~~
CI: https://ci.nodejs.org/job/node-test-pull-request/11742/

jasnell · 2017-11-27T05:09:47Z

Slight bit of overlap with this: #17334

jasnell · 2017-11-27T16:04:59Z

@joyeecheung ... in #17334 I'm going to be focusing entirely on moving the input arg validation out into javascript. To make it easier on us both, how about focusing this PR only on the other errors then we can meet in the middle :-)

joyeecheung · 2017-11-28T04:34:37Z

@jasnell ACK, thanks, I only migrated those type checks in this PR as well because since I was migrating fs.close, might as well.

joyeecheung · 2017-11-29T10:05:01Z

Removed the type checks in favor of #17334

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

joyeecheung · 2017-11-30T04:16:02Z

CI is green minus a jenkins failure. Marking this semver-major because it depends on #17160 , although I think both could be backported to v8.x if we skip the type check part..

@jasnell @fhinkel @nodejs/tsc PTAL, thanks!

joyeecheung · 2017-12-05T18:10:59Z

This is no longer semver major, but defensively adding the don't land labels until #17160 is backported in non-semver-major fashion

BridgeAR · 2017-12-12T19:27:39Z

lib/internal/errors.js


-  let message = `${ctx.code}: ${ctx.message}, ${ctx.syscall}`;
+  for (const prop of Object.keys(ctx)) {
+    if (prop === 'message' || prop === 'path' || prop === 'dest') {


~~The message will actually not show up because it should be non-enumerable. If ctx is a regular error.~~

Ignore me, I should have checked what ctx stands for. When I commented first it seemed like a error.

BridgeAR · 2017-12-12T19:36:42Z

lib/internal/errors.js

+    if (prop === 'message' || prop === 'path' || prop === 'dest') {
+      continue;
+    }
+    err[prop] = ctx;


I think you wanted to write ctx[prop] instead of ctx.

BridgeAR

JS part LGTM besides the comment. The rest is also LG as far as I can tell.

joyeecheung · 2017-12-16T09:27:50Z

Rebased now that the type checking tests have landed. Also validate the syscall and errno properties to avoid the mistakes as @BridgeAR pointed out. New CI: https://ci.nodejs.org/job/node-test-pull-request/12148/

joyeecheung · 2017-12-27T09:51:01Z

CI: https://ci.nodejs.org/job/node-test-commit/15076/

Minimized the changes to node_file.cc and split the uv binding changes to a separate commit. CI is green now minus one unrelated failure. @jasnell @BridgeAR PTAL.

jasnell · 2017-12-27T14:20:37Z

still lgtm

joyeecheung · 2017-12-27T19:14:15Z

Landed in 24c71fb...9f122e3, thanks!

Add a errno -> [error code, uv error message] map to the uv binding so the error message can be assembled in the JS layer. PR-URL: #17338 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

- Simplify the SyncCall template function, only collect error number and syscall in the C++ layer and collect the rest of context in JS for flexibility. - Remove the stringFromPath JS helper now that the unprefixed path is directly put into the context before the binding is invoked with the prefixed path. - Validate more properties in fs.access tests. PR-URL: #17338 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

* Collect the error context in both JS and C++, then throw the error in JS * Test that the errors thrown from fs.close and fs.closeSync includes the correct error code, error number and syscall properties PR-URL: #17338 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Add a errno -> [error code, uv error message] map to the uv binding so the error message can be assembled in the JS layer. PR-URL: nodejs#17338 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Add a errno -> [error code, uv error message] map to the uv binding so the error message can be assembled in the JS layer. Backport-PR-URL: #18916 PR-URL: #17338 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Add a errno -> [error code, uv error message] map to the uv binding so the error message can be assembled in the JS layer. PR-URL: nodejs#17338 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Add a errno -> [error code, uv error message] map to the uv binding so the error message can be assembled in the JS layer. Backport-PR-URL: #19191 PR-URL: #17338 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Nov 27, 2017

joyeecheung force-pushed the fs-error-close branch from 6045b01 to 00e4029 Compare November 27, 2017 04:28

joyeecheung changed the title ~~fs: simply the error context collection in C++, migrate fs.close errors~~ fs: simplify the error context collection in C++, migrate fs.close errors Nov 27, 2017

joyeecheung commented Nov 27, 2017

View reviewed changes

joyeecheung force-pushed the fs-error-close branch from 00e4029 to 7f3f9d9 Compare November 27, 2017 04:49

joyeecheung 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 Nov 27, 2017

joyeecheung force-pushed the fs-error-close branch from 7f3f9d9 to 0309a8a Compare November 29, 2017 10:04

jasnell approved these changes Dec 5, 2017

View reviewed changes

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

MylesBorins force-pushed the master branch from b7405ab to 7f086dd Compare December 8, 2017 16:37

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

BridgeAR reviewed Dec 12, 2017

View reviewed changes

BridgeAR approved these changes Dec 12, 2017

View reviewed changes

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

joyeecheung force-pushed the fs-error-close branch from 0309a8a to 4fcf4ee Compare December 16, 2017 09:25

joyeecheung removed the wip Issues and PRs that are still a work in progress. label Dec 26, 2017

joyeecheung closed this Dec 27, 2017

joyeecheung mentioned this pull request Jan 9, 2018

test: zlib-binding.deflate flaky v8/node#45

Closed

joyeecheung mentioned this pull request Feb 21, 2018

[v9.x] backport 17338, 18186, 18358, 18546 #18916

Closed

addaleax mentioned this pull request Feb 27, 2018

v9.7.0 proposal #19040

Closed

joyeecheung mentioned this pull request Mar 7, 2018

[v8.x backport] backport 17338, 18186, 18358, 18546 #19191

Closed

MylesBorins mentioned this pull request Jul 9, 2018

v8.12.0 proposal #21593

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: simplify the error context collection in C++, migrate fs.close errors #17338

fs: simplify the error context collection in C++, migrate fs.close errors #17338

joyeecheung commented Nov 27, 2017 •

edited

Loading

joyeecheung Nov 27, 2017 •

edited

Loading

joyeecheung commented Nov 27, 2017 •

edited

Loading

jasnell commented Nov 27, 2017

jasnell commented Nov 27, 2017

joyeecheung commented Nov 28, 2017

joyeecheung commented Nov 29, 2017

joyeecheung commented Nov 30, 2017

joyeecheung commented Dec 5, 2017

BridgeAR Dec 12, 2017 •

edited

Loading

BridgeAR Dec 12, 2017

BridgeAR Dec 12, 2017

BridgeAR left a comment

joyeecheung commented Dec 16, 2017

joyeecheung commented Dec 27, 2017 •

edited

Loading

jasnell commented Dec 27, 2017

joyeecheung commented Dec 27, 2017 •

edited

Loading

fs: simplify the error context collection in C++, migrate fs.close errors #17338

fs: simplify the error context collection in C++, migrate fs.close errors #17338

Conversation

joyeecheung commented Nov 27, 2017 • edited Loading

Checklist

Affected core subsystem(s)

joyeecheung Nov 27, 2017 • edited Loading

Choose a reason for hiding this comment

joyeecheung commented Nov 27, 2017 • edited Loading

jasnell commented Nov 27, 2017

jasnell commented Nov 27, 2017

joyeecheung commented Nov 28, 2017

joyeecheung commented Nov 29, 2017

joyeecheung commented Nov 30, 2017

joyeecheung commented Dec 5, 2017

BridgeAR Dec 12, 2017 • edited Loading

Choose a reason for hiding this comment

BridgeAR Dec 12, 2017

Choose a reason for hiding this comment

BridgeAR Dec 12, 2017

Choose a reason for hiding this comment

BridgeAR left a comment

Choose a reason for hiding this comment

joyeecheung commented Dec 16, 2017

joyeecheung commented Dec 27, 2017 • edited Loading

jasnell commented Dec 27, 2017

joyeecheung commented Dec 27, 2017 • edited Loading

joyeecheung commented Nov 27, 2017 •

edited

Loading

joyeecheung Nov 27, 2017 •

edited

Loading

joyeecheung commented Nov 27, 2017 •

edited

Loading

BridgeAR Dec 12, 2017 •

edited

Loading

joyeecheung commented Dec 27, 2017 •

edited

Loading

joyeecheung commented Dec 27, 2017 •

edited

Loading