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

repl: improve error output #22436

Closed
wants to merge 4 commits into from
Closed

repl: improve error output #22436

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0a9ad2c
repl: improve error output
BridgeAR Aug 21, 2018
9709144
repl: refactor ERR_SCRIPT_EXECUTION_INTERRUPTED stack handling
BridgeAR Aug 21, 2018
8102eb1
fixup: address comment
BridgeAR Aug 24, 2018
c347f7a
fixup: fix windows test
BridgeAR Aug 26, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 46 additions & 31 deletions lib/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,11 +339,6 @@ function REPLServer(prompt,
} catch (e) {
err = e;

if (err && err.code === 'ERR_SCRIPT_EXECUTION_INTERRUPTED') {
// The stack trace for this case is not very useful anyway.
Object.defineProperty(err, 'stack', { value: '' });
}

if (process.domain) {
debug('not recoverable, send to domain');
process.domain.emit('error', err);
Expand All @@ -359,7 +354,11 @@ function REPLServer(prompt,
if (self.breakEvalOnSigint) {
const interrupt = new Promise((resolve, reject) => {
sigintListener = () => {
reject(new ERR_SCRIPT_EXECUTION_INTERRUPTED());
const tmp = Error.stackTraceLimit;
Error.stackTraceLimit = 0;
const err = new ERR_SCRIPT_EXECUTION_INTERRUPTED();
Error.stackTraceLimit = tmp;
reject(err);
Copy link
Member

Choose a reason for hiding this comment

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

Since .stack creation is deferred until the .stack is accessed does toggling the Error.stackTraceLimit have the effect wanted here (making an empty stack)? It looks like stackTraceLimit is reset before the lazy .stack is generated.

Copy link
Member Author

Choose a reason for hiding this comment

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

The stack size is set on error creation time. Otherwise the test would fail as well.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good to know!

};
prioritizedSigintQueue.add(sigintListener);
});
Expand All @@ -378,11 +377,6 @@ function REPLServer(prompt,
// Remove prioritized SIGINT listener if it was not called.
prioritizedSigintQueue.delete(sigintListener);

if (err.code === 'ERR_SCRIPT_EXECUTION_INTERRUPTED') {
// The stack trace for this case is not very useful anyway.
Object.defineProperty(err, 'stack', { value: '' });
}

unpause();
if (err && process.domain) {
debug('not recoverable, send to domain');
Expand All @@ -404,29 +398,50 @@ function REPLServer(prompt,

self._domain.on('error', function debugDomainError(e) {
debug('domain error');
const top = replMap.get(self);
const pstrace = Error.prepareStackTrace;
Error.prepareStackTrace = prepareStackTrace(pstrace);
if (typeof e === 'object')
let errStack = '';

if (typeof e === 'object' && e !== null) {
const pstrace = Error.prepareStackTrace;
Error.prepareStackTrace = prepareStackTrace(pstrace);
internalUtil.decorateErrorStack(e);
Error.prepareStackTrace = pstrace;
const isError = internalUtil.isError(e);
if (!self.underscoreErrAssigned)
self.lastError = e;
if (e instanceof SyntaxError && e.stack) {
// remove repl:line-number and stack trace
e.stack = e.stack
.replace(/^repl:\d+\r?\n/, '')
.replace(/^\s+at\s.*\n?/gm, '');
} else if (isError && self.replMode === exports.REPL_MODE_STRICT) {
e.stack = e.stack.replace(/(\s+at\s+repl:)(\d+)/,
(_, pre, line) => pre + (line - 1));
Error.prepareStackTrace = pstrace;

if (e.domainThrown) {
delete e.domain;
delete e.domainThrown;
}

if (internalUtil.isError(e)) {
if (e.stack) {
if (e instanceof SyntaxError) {
Copy link
Member

Choose a reason for hiding this comment

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

The error could be a syntax error from another realm (so not instanceof SyntaxError). Since we've already confirmed it is an error object above with isError(e) we could just check that the name is SyntaxError.

// Remove stack trace.
e.stack = e.stack
.replace(/^repl:\d+\r?\n/, '')
.replace(/^\s+at\s.*\n?/gm, '');
} else if (self.replMode === exports.REPL_MODE_STRICT) {
e.stack = e.stack.replace(/(\s+at\s+repl:)(\d+)/,
(_, pre, line) => pre + (line - 1));
}
}
errStack = util.inspect(e);

// Remove one line error braces to keep the old style in place.
if (errStack[errStack.length - 1] === ']') {
errStack = errStack.slice(1, -1);
}
}
}
if (isError && e.stack) {
top.outputStream.write(`${e.stack}\n`);
} else {
top.outputStream.write(`Thrown: ${String(e)}\n`);

if (errStack === '') {
errStack = `Thrown: ${util.inspect(e)}`;
}

if (!self.underscoreErrAssigned) {
self.lastError = e;
}

const top = replMap.get(self);
top.outputStream.write(`${errStack}\n`);
top.clearBufferedCommand();
top.lines.level = [];
top.displayPrompt();
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-repl-top-level-await.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ async function ctrlCTest() {
{ ctrl: true, name: 'c' }
]), [
'await timeout(100000)\r',
'Thrown: Error [ERR_SCRIPT_EXECUTION_INTERRUPTED]: ' +
'Error [ERR_SCRIPT_EXECUTION_INTERRUPTED]: ' +
'Script execution was interrupted by `SIGINT`',
PROMPT
]);
Expand Down
6 changes: 5 additions & 1 deletion test/parallel/test-repl-underscore.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,8 +177,12 @@ function testError() {
'[Error: foo]',

// The sync error, with individual property echoes
/Error: ENOENT: no such file or directory, scandir '.*nonexistent.*'/,
/^{ Error: ENOENT: no such file or directory, scandir '.*nonexistent.*'/,
/Object\.readdirSync/,
' errno: -2,',
" syscall: 'scandir',",
" code: 'ENOENT',",
" path: '/nonexistent?' }",
"'ENOENT'",
"'scandir'",

Expand Down
8 changes: 6 additions & 2 deletions test/parallel/test-repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,11 @@ const errorTests = [
// Uncaught error throws and prints out
{
send: 'throw new Error(\'test error\');',
expect: /^Error: test error/
expect: 'Error: test error'
},
{
send: "throw { foo: 'bar' };",
expect: "Thrown: { foo: 'bar' }"
},
// Common syntax error is treated as multiline command
{
Expand Down Expand Up @@ -526,7 +530,7 @@ const errorTests = [
{
send: 'require("internal/repl")',
expect: [
/^Error: Cannot find module 'internal\/repl'/,
/^{ Error: Cannot find module 'internal\/repl'/,
/^ at .*/,
/^ at .*/,
/^ at .*/,
Expand Down