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

module: always decorate thrown errors #4287

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
4 participants
@mscdex
Copy link
Contributor

commented Dec 15, 2015

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

@mscdex

This comment has been minimized.

Copy link
Contributor Author

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

This comment has been minimized.

Copy link
Member

commented Dec 15, 2015

LGTM

@cjihrig

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Dec 15, 2015

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

@mscdex

This comment has been minimized.

Copy link
Contributor Author

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 mscdex:module-decorate-errors branch 2 times, most recently Dec 15, 2015

@mscdex

This comment has been minimized.

Copy link
Contributor Author

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 mscdex:module-decorate-errors branch Dec 15, 2015

@cjihrig

View changes

test/parallel/test-util-decorate-error-stack.js Outdated
`require('${badSyntaxPath}')`
];
const result = spawnSync(process.argv[0], args, { encoding: 'utf8' });
matches = err.stack.match(/var foo bar;/g);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 16, 2015

Contributor

Should this use result?

This comment has been minimized.

Copy link
@mscdex

mscdex Dec 16, 2015

Author Contributor

Fixed.

@cjihrig

View changes

test/parallel/test-util-decorate-error-stack.js Outdated
// Verify that the stack is only decorated once
internalUtil.decorateErrorStack(err);
internalUtil.decorateErrorStack(err);
matches = err.stack.match(/var foo bar;/g);

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 16, 2015

Contributor

Could you DRY this and the following two lines?

This comment has been minimized.

Copy link
@mscdex

mscdex Dec 16, 2015

Author Contributor

Done.

@cjihrig

View changes

src/node.cc Outdated
@@ -1399,6 +1399,17 @@ ssize_t DecodeWrite(Isolate* isolate,
return StringBytes::Write(isolate, buf, buflen, val, encoding, nullptr);
}

bool ExceptionDecorated(Environment* env, Local<Value> er) {

This comment has been minimized.

Copy link
@cjihrig

cjihrig Dec 16, 2015

Contributor

Maybe call this IsExceptionDecorated.

This comment has been minimized.

Copy link
@mscdex

mscdex Dec 16, 2015

Author Contributor

Changed.

@mscdex mscdex force-pushed the mscdex:module-decorate-errors branch 3 times, most recently Dec 16, 2015

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2015

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

@cjihrig

This comment has been minimized.

Copy link
Contributor

commented Dec 16, 2015

LGTM

@bnoordhuis

View changes

lib/internal/util.js Outdated
err.stack = arrow + err.stack;
exports.setHiddenValue(err, 'decorated', true);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Dec 16, 2015

Member

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

@bnoordhuis

View changes

src/node.cc Outdated
@@ -1508,6 +1519,7 @@ static void ReportException(Environment* env,

Local<Value> trace_value;
Local<Value> arrow;
bool decorated = IsExceptionDecorated(env, er);

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Dec 16, 2015

Member

const bool?

@jasnell

This comment has been minimized.

Copy link
Member

commented Dec 16, 2015

semver-minor?

@mscdex mscdex force-pushed the mscdex:module-decorate-errors branch Dec 17, 2015

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Dec 17, 2015

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

@bnoordhuis

View changes

src/node.cc Outdated
Local<Value> decorated = err_obj->GetHiddenValue(env->decorated_string());
return (!decorated.IsEmpty() &&
decorated->IsBoolean() &&
decorated->BooleanValue());

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Dec 17, 2015

Member

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

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Dec 17, 2015

LGTM with a nit and a suggestion.

module: always decorate thrown errors
This provides more information when encountering a syntax or similar
error when executing a file with require().

@mscdex mscdex force-pushed the mscdex:module-decorate-errors branch to e7ac2fc Dec 17, 2015

@mscdex

This comment has been minimized.

Copy link
Contributor Author

commented Dec 18, 2015

@mscdex

This comment has been minimized.

Copy link
Contributor Author

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

module: always decorate thrown errors
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

This comment has been minimized.

Copy link
Contributor Author

commented Dec 19, 2015

Landed in 18490d3.

@mscdex mscdex closed this Dec 19, 2015

@mscdex mscdex deleted the mscdex:module-decorate-errors branch Dec 19, 2015

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Dec 22, 2015

module: always decorate thrown errors
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

2015-12-22, Version 5.3.1 (Stable)
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

This comment has been minimized.

Copy link
Member

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 referenced this pull request Mar 17, 2016

Closed

Planning for v6 #5766

scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016

module: always decorate thrown errors
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
You can’t perform that action at this time.