[v8.x backport] repl: remove internal frames from runtime errors by lance · Pull Request #16457 · nodejs/node

lance · 2017-10-24T20:42:51Z

When a user executes code in the REPLServer which generates an
exception, there is no need to display the REPLServer internal
stack frames.

PR-URL: #15351
Reviewed-By: Prince John Wesley [email protected]
Reviewed-By: James M Snell [email protected]
Reviewed-By: Ruben Bridgewater [email protected]

Refs: #9601

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)

repl

lance · 2017-10-24T20:46:47Z

I left make -j4 test unchecked because I am getting two completely unrelated but consistent test failures for async_hooks. Not sure what to make of that.

Local build failure stack traces

/usr/local/opt/python/bin/python2.7 tools/test.py --mode=release -J \
                async-hooks \
                default \
                addons addons-napi \
                doctool known_issues
=== release test-callback-error ===
Path: async-hooks/test-callback-error
start case 1
end case 1: 147.604ms
start case 2
end case 2: 121.808ms
start case 3
end case 3: 8.184ms
Error: test_callback_abort
    at ActivityCollector.initHooks.oninit.common.mustCall (/Users/lanceball/src/node/test/async-hooks/test-callback-error.js:36:45)
    at ActivityCollector.oninit (/Users/lanceball/src/node/test/common/index.js:531:15)
    at ActivityCollector._init (/Users/lanceball/src/node/test/async-hooks/init-hooks.js:185:10)
    at emitInitNative (async_hooks.js:472:43)
    at Object.emitInitScript [as emitInit] (async_hooks.js:388:3)
    at Object. (/Users/lanceball/src/node/test/async-hooks/test-callback-error.js:38:17)
    at Module._compile (module.js:612:30)
    at Object.Module._extensions..js (module.js:623:10)
    at Module.load (module.js:531:32)
    at tryModuleLoad (module.js:494:12)
 1: node::Abort() [/Users/lanceball/src/node/out/Release/node]
 2: node::Chdir(v8::FunctionCallbackInfo const&) [/Users/lanceball/src/node/out/Release/node]
 3: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo const&)) [/Users/lanceball/src/node/out/Release/node]
 4: v8::internal::MaybeHandle v8::internal::(anonymous namespace)::HandleApiCallHelper(v8::internal::Isolate*, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::BuiltinArguments) [/Users/lanceball/src/node/out/Release/node]
 5: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/lanceball/src/node/out/Release/node]
 6: 0x33e31af0463d
Command: out/Release/node /Users/lanceball/src/node/test/async-hooks/test-callback-error.js
=== release test-callback-error ===
Path: async-hooks/test-callback-error
start case 1
end case 1: 122.510ms
start case 2
end case 2: 124.422ms
start case 3
end case 3: 7.740ms
Error: test_callback_abort
    at ActivityCollector.initHooks.oninit.common.mustCall (/Users/lanceball/src/node/test/async-hooks/test-callback-error.js:36:45)
    at ActivityCollector.oninit (/Users/lanceball/src/node/test/common/index.js:531:15)
    at ActivityCollector._init (/Users/lanceball/src/node/test/async-hooks/init-hooks.js:185:10)
    at emitInitNative (async_hooks.js:472:43)
    at Object.emitInitScript [as emitInit] (async_hooks.js:388:3)
    at Object. (/Users/lanceball/src/node/test/async-hooks/test-callback-error.js:38:17)
    at Module._compile (module.js:612:30)
    at Object.Module._extensions..js (module.js:623:10)
    at Module.load (module.js:531:32)
    at tryModuleLoad (module.js:494:12)
 1: node::Abort() [/Users/lanceball/src/node/out/Release/node]
 2: node::Chdir(v8::FunctionCallbackInfo const&) [/Users/lanceball/src/node/out/Release/node]
 3: v8::internal::FunctionCallbackArguments::Call(void (*)(v8::FunctionCallbackInfo const&)) [/Users/lanceball/src/node/out/Release/node]
 4: v8::internal::MaybeHandle v8::internal::(anonymous namespace)::HandleApiCallHelper(v8::internal::Isolate*, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::Handle, v8::internal::BuiltinArguments) [/Users/lanceball/src/node/out/Release/node]
 5: v8::internal::Builtin_Impl_HandleApiCall(v8::internal::BuiltinArguments, v8::internal::Isolate*) [/Users/lanceball/src/node/out/Release/node]
 6: 0x948e650463d
Command: out/Release/node /Users/lanceball/src/node/test/async-hooks/test-callback-error.js
[04:01|% 100|+ 2007|-   2]: Done
make: *** [test] Error 1

Edit: This has been resolved

lance · 2017-10-25T16:19:09Z

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

Edit: several failures - all of which seem to be unrelated.

gibfahn · 2017-10-30T22:42:16Z

lib/repl.js

@lance I could be wrong, but doesn't this code come from #14223, which was semver-major?

If so I think you can just drop this bit.

lance · 2017-10-31T15:22:04Z

@gibfahn good catch! Fixed (I think). I had to add REPLServer.prototype.parseREPLKeyword() back in to repl.js which seems like the best way to deal with this.

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

gibfahn · 2017-11-14T16:30:48Z

lib/repl.js

Isn't this function already defined here?

node/lib/repl.js

Lines 1053 to 1066 in edb03cb

/**

* Used to parse and execute the Node REPL commands.

*

* @param {keyword} keyword The command entered to check.

* @return {Boolean} If true it means don't continue parsing the command.

*/

REPLServer.prototype.parseREPLKeyword = function(keyword, rest) {

var cmd = this.commands[keyword];

if (cmd) {

cmd.action.call(this, rest);

return true;

}

return false;

};

Hmm - that's disconcerting enough for me to want to start over on this. I'll take a look..

Update: bad cherry picking on my part. I will fix and push. My apologies.

Don't think function should be declared twice.

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames. PR-URL: #15351 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Refs: #9601

lance · 2017-11-17T21:09:07Z

Another CI after latest push https://ci.nodejs.org/job/node-test-pull-request/11499/

gibfahn · 2017-12-20T01:38:00Z

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

When a user executes code in the REPLServer which generates an exception, there is no need to display the REPLServer internal stack frames. PR-URL: #15351 Backport-PR-URL: #16457 Reviewed-By: Prince John Wesley <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]> Refs: #9601

gibfahn · 2017-12-20T23:01:16Z

CI failures were from the v8.x-staging branch AFAICT.

Landed in 6deae6e

Thanks @lance !

nodejs-github-bot added repl Issues and PRs related to the REPL subsystem. v8.x labels Oct 24, 2017

lance requested a review from MylesBorins October 24, 2017 20:47

lance mentioned this pull request Oct 24, 2017

repl: remove internal frames from runtime errors #15351

Closed

3 tasks

gibfahn force-pushed the v8.x-staging branch 5 times, most recently from b183192 to fc8acc8 Compare October 30, 2017 21:42

gibfahn suggested changes Oct 30, 2017

View reviewed changes

gibfahn force-pushed the v8.x-staging branch 2 times, most recently from 97c2301 to ab0d7a6 Compare October 31, 2017 00:16

gibfahn previously approved these changes Nov 14, 2017

View reviewed changes

gibfahn reviewed Nov 14, 2017

View reviewed changes

gibfahn self-assigned this Nov 22, 2017

MylesBorins force-pushed the v8.x-staging branch from 468562c to b05ef97 Compare December 7, 2017 18:20

gibfahn force-pushed the v8.x-staging branch from 2233a64 to 053eba0 Compare December 19, 2017 23:21

gibfahn force-pushed the v8.x-staging branch 2 times, most recently from 563f1f6 to 6fa51d6 Compare December 20, 2017 22:46

gibfahn approved these changes Dec 20, 2017

View reviewed changes

gibfahn closed this Dec 20, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[v8.x backport] repl: remove internal frames from runtime errors#16457

[v8.x backport] repl: remove internal frames from runtime errors#16457
lance wants to merge 1 commit intonodejs:v8.x-stagingfrom
lance:backport-15351-to-8.x

lance commented Oct 24, 2017 •

edited

Loading

Uh oh!

lance commented Oct 24, 2017 •

edited

Loading

Uh oh!

lance commented Oct 25, 2017 •

edited

Loading

Uh oh!

gibfahn Oct 30, 2017

Uh oh!

lance commented Oct 31, 2017

Uh oh!

gibfahn Nov 14, 2017

Uh oh!

lance Nov 17, 2017 •

edited

Loading

Uh oh!

lance commented Nov 17, 2017

Uh oh!

gibfahn commented Dec 20, 2017

Uh oh!

gibfahn commented Dec 20, 2017 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

	/**
	* Used to parse and execute the Node REPL commands.
	*
	* @param {keyword} keyword The command entered to check.
	* @return {Boolean} If true it means don't continue parsing the command.
	*/
	REPLServer.prototype.parseREPLKeyword = function(keyword, rest) {
	var cmd = this.commands[keyword];
	if (cmd) {
	cmd.action.call(this, rest);
	return true;
	}
	return false;
	};

Uh oh!

Conversation

lance commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

lance commented Oct 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lance commented Oct 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gibfahn Oct 30, 2017

Choose a reason for hiding this comment

Uh oh!

lance commented Oct 31, 2017

Uh oh!

gibfahn Nov 14, 2017

Choose a reason for hiding this comment

Uh oh!

lance Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lance commented Nov 17, 2017

Uh oh!

gibfahn commented Dec 20, 2017

Uh oh!

gibfahn commented Dec 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

lance commented Oct 24, 2017 •

edited

Loading

lance commented Oct 24, 2017 •

edited

Loading

lance commented Oct 25, 2017 •

edited

Loading

lance Nov 17, 2017 •

edited

Loading

gibfahn commented Dec 20, 2017 •

edited

Loading