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.

Sign up for GitHub

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

Jump to bottom

module: always decorate thrown errors #4287

Closed
wants to merge 1 commit into from
Closed

module: always decorate thrown errors #4287

wants to merge 1 commit into from

Conversation

mscdex
Copy link
Contributor

@mscdex mscdex commented Dec 15, 2015

This provides more information when encountering a syntax or similar error when executing a file with require().

@mscdex mscdex added the module Issues and PRs related to the module subsystem. label Dec 15, 2015
@mscdex mscdex mentioned this pull request Dec 15, 2015
Closed
@mscdex
Copy link
Contributor Author

mscdex commented Dec 15, 2015

CI is all green except flaky tests and CI issues: https://ci.nodejs.org/job/node-test-pull-request/1001/

@bnoordhuis
Copy link
Member

LGTM

@cjihrig
Copy link
Contributor

cjihrig commented Dec 15, 2015

Using the normal workflow, this will cause two arrow messages to be printed. For example:

$ node foo.js 
/Users/cjihrig/iojs/node/foo.js:1
 ^
 ^

/Users/cjihrig/iojs/node/foo.js:1
 ^
 ^
SyntaxError: Unexpected token ^
...

Where foo.js just contains a ^.

@cjihrig
Copy link
Contributor

cjihrig commented Dec 15, 2015

From a quick test, it looks like removing the arrow information from ReportException() in node.cc fixes the problem.

@mscdex
Copy link
Contributor Author

mscdex commented Dec 15, 2015

Hrmm... what about adding/checking a "decorated" flag as another hidden value on the error object?

@mscdex mscdex force-pushed the module-decorate-errors branch 2 times, most recently from 57a9169 to 490c4b8 Compare December 15, 2015 20:39
@mscdex
Copy link
Contributor Author

mscdex commented Dec 15, 2015

@cjihrig Alright, I added a 'decorated' flag and a new test for uncaught errors.

CI is all green except flaky tests/CI issues: https://ci.nodejs.org/job/node-test-pull-request/1006/

@mscdex mscdex force-pushed the module-decorate-errors branch from 490c4b8 to fbcb95a Compare December 15, 2015 21:10
cjihrig
cjihrig reviewed Dec 16, 2015
View reviewed changes
test/parallel/test-util-decorate-error-stack.js
`require('${badSyntaxPath}')`
];
const result = spawnSync(process.argv[0], args, { encoding: 'utf8' });
matches = err.stack.match(/var foo bar;/g);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this use result?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@mscdex mscdex force-pushed the module-decorate-errors branch 3 times, most recently from 9dc88d1 to ed203d0 Compare December 16, 2015 03:56
@mscdex
Copy link
Contributor Author

mscdex commented Dec 16, 2015

Incorporated suggestions and CI is green except flaky tests: https://ci.nodejs.org/job/node-test-pull-request/1010/

@cjihrig
Copy link
Contributor

cjihrig commented Dec 16, 2015

LGTM

bnoordhuis
bnoordhuis reviewed Dec 16, 2015
View reviewed changes
lib/internal/util.js
err.stack = arrow + err.stack;
exports.setHiddenValue(err, 'decorated', true);
Copy link
Member

Choose a reason for hiding this comment

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

I'd probably give this a slightly more unique name, e.g. 'node:decorated'. (Ditto for 'arrowMessage' although this PR didn't introduce that.)

@jasnell
Copy link
Member

jasnell commented Dec 16, 2015

semver-minor?

@mscdex mscdex force-pushed the module-decorate-errors branch from ed203d0 to ac1633b Compare December 17, 2015 22:49
@mscdex
Copy link
Contributor Author

mscdex commented Dec 17, 2015

@bnoordhuis I've made your suggested changes. LGTY now?

bnoordhuis
bnoordhuis reviewed Dec 17, 2015
View reviewed changes
src/node.cc
Local<Value> decorated = err_obj->GetHiddenValue(env->decorated_string());
return (!decorated.IsEmpty() &&
decorated->IsBoolean() &&
decorated->BooleanValue());
Copy link
Member

Choose a reason for hiding this comment

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

Superfluous parentheses and you can shorten it to return !decorated.IsEmpty() && decorated->IsTrue(), which should also be infinitesimally faster.

@bnoordhuis
Copy link
Member

LGTM with a nit and a suggestion.

@mscdex
module: always decorate thrown errors
e7ac2fc 
This provides more information when encountering a syntax or similar
error when executing a file with require().
@mscdex mscdex force-pushed the module-decorate-errors branch from ac1633b to e7ac2fc Compare December 17, 2015 23:54
@mscdex
Copy link
Contributor Author

mscdex commented Dec 18, 2015

Updated commit. Last CI run: https://ci.nodejs.org/job/node-test-pull-request/1022/

@mscdex
Copy link
Contributor Author

mscdex commented Dec 18, 2015

@jasnell I'm not sure how this fits in (if at all) with whatever was agreed upon on with regard to changing errors and semver-ness, but my guess would be semver-major since it's changing the formatting of the stack trace?

mscdex added a commit that referenced this pull request Dec 19, 2015
@mscdex
module: always decorate thrown errors
18490d3 
This provides more information when encountering a syntax or similar
error when executing a file with require().

Fixes: #4286
PR-URL: #4287
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@mscdex
Copy link
Contributor Author

mscdex commented Dec 19, 2015

Landed in 18490d3.

@mscdex mscdex closed this Dec 19, 2015
@mscdex mscdex deleted the module-decorate-errors branch December 19, 2015 19:20
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
@mscdex @Fishrock123
module: always decorate thrown errors
72b6b4f 
This provides more information when encountering a syntax or similar
error when executing a file with require().

Fixes: nodejs#4286
PR-URL: nodejs#4287
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Dec 22, 2015
@Fishrock123
2015-12-22, Version 5.3.1 (Stable)
b7fd395 
Notable changes:

* general: Several minor performance improvements:
  - lib: Use arrow functions instead of bind where possible (Minwoo
Jung) nodejs#3622
  - node: Improved accessor perf of process.env (Trevor Norris)
nodejs#3780
  - **node**: Improved performance of hrtime() (Trevor Norris)
nodejs#3780
  - node: Improved GetActiveHandles performance (Trevor Norris)
nodejs#3780
* module: Errors during require() now provide more information (Brian
White) nodejs#4287
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 24, 2015
@jasnell
Copy link
Member

jasnell commented Dec 24, 2015

Marking as semver-major due to the error handling change. It's not clear if this is major or minor tho. @nodejs/ctc

@jasnell jasnell mentioned this pull request Mar 17, 2016
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
@mscdex
module: always decorate thrown errors
65c424f 
This provides more information when encountering a syntax or similar
error when executing a file with require().

Fixes: nodejs#4286
PR-URL: nodejs#4287
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
module Issues and PRs related to the module subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@mscdex @bnoordhuis @cjihrig @jasnell