Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Nov 27, 2017

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

@nodejs-github-bot 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 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
assert.throws(
() => { fs.closeSync(-1); },
(err) => {
assert.strictEqual(err.code, 'EBADF');
Copy link
Member Author

@joyeecheung joyeecheung Nov 27, 2017

Choose a reason for hiding this comment

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

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
Copy link
Member Author

joyeecheung commented Nov 27, 2017

@joyeecheung 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
@jasnell
Copy link
Member

jasnell commented Nov 27, 2017

Slight bit of overlap with this: #17334

@jasnell
Copy link
Member

jasnell commented Nov 27, 2017

@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
Copy link
Member Author

@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
Copy link
Member Author

Removed the type checks in favor of #17334

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

@joyeecheung
Copy link
Member Author

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 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
@joyeecheung
Copy link
Member Author

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

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

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

@BridgeAR BridgeAR Dec 12, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

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

@BridgeAR 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
Copy link
Member Author

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 joyeecheung removed the wip Issues and PRs that are still a work in progress. label Dec 26, 2017
@joyeecheung
Copy link
Member Author

joyeecheung commented Dec 27, 2017

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
Copy link
Member

jasnell commented Dec 27, 2017

still lgtm

@joyeecheung
Copy link
Member Author

joyeecheung commented Dec 27, 2017

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

joyeecheung added a commit that referenced this pull request Dec 27, 2017
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit that referenced this pull request Dec 27, 2017
- 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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit that referenced this pull request Dec 27, 2017
* 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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
joyeecheung added a commit to joyeecheung/node that referenced this pull request Feb 22, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Feb 26, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@addaleax addaleax mentioned this pull request Feb 27, 2018
joyeecheung added a commit to joyeecheung/node that referenced this pull request May 2, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request May 22, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
MylesBorins pushed a commit that referenced this pull request Jun 14, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@MylesBorins MylesBorins mentioned this pull request Jul 9, 2018
rvagg pushed a commit that referenced this pull request Aug 16, 2018
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 <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. fs Issues and PRs related to the fs subsystem / file system. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants