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

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

lance
Copy link
Member

@lance lance commented Oct 24, 2017

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

@nodejs-github-bot nodejs-github-bot added repl Issues and PRs related to the REPL subsystem. v8.x labels Oct 24, 2017
@lance
Copy link
Member Author

lance commented Oct 24, 2017

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

lance commented Oct 25, 2017

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

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

@gibfahn gibfahn force-pushed the v8.x-staging branch 5 times, most recently from b183192 to fc8acc8 Compare October 30, 2017 21:42
lib/repl.js Outdated
self.parseREPLKeyword = util.deprecate(
_parseREPLKeyword,
'REPLServer.parseREPLKeyword() is deprecated',
'DEP0075');
Copy link
Member

Choose a reason for hiding this comment

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

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

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

lance commented Oct 31, 2017

@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
gibfahn previously approved these changes Nov 14, 2017
lib/repl.js Outdated
* @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) {
Copy link
Member

Choose a reason for hiding this comment

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

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;
};

Copy link
Member Author

@lance lance Nov 17, 2017

Choose a reason for hiding this comment

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

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.

@gibfahn gibfahn dismissed their stale review November 14, 2017 16:31

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

lance commented Nov 17, 2017

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

@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

@gibfahn gibfahn force-pushed the v8.x-staging branch 2 times, most recently from 563f1f6 to 6fa51d6 Compare December 20, 2017 22:46
gibfahn pushed a commit that referenced this pull request Dec 20, 2017
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
Copy link
Member

gibfahn commented Dec 20, 2017

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

Landed in 6deae6e

Thanks @lance !

@gibfahn gibfahn closed this Dec 20, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
repl Issues and PRs related to the REPL subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants