Skip to content

Commit

Permalink
repl: handle uncaughtException properly
Browse files Browse the repository at this point in the history
When running the REPL as standalone program it's now possible to use
`process.on('uncaughtException', listener)`. It is going to use those
listeners from now on and the regular error output is suppressed.

It also fixes the issue that REPL instances started inside of an
application would silence all application errors. It is now prohibited
to add the exception listener in such REPL instances. Trying to add
such listeners throws an `ERR_INVALID_REPL_INPUT` error.

Fixes: #19998

PR-URL: #27151
Fixes: #19998
Reviewed-By: Lance Ball <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
  • Loading branch information
BridgeAR committed May 8, 2019
1 parent 9a174db commit 422e8f7
Show file tree
Hide file tree
Showing 8 changed files with 244 additions and 17 deletions.
11 changes: 9 additions & 2 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1310,8 +1310,14 @@ An invalid `options.protocol` was passed to `http.request()`.
<a id="ERR_INVALID_REPL_EVAL_CONFIG"></a>
### ERR_INVALID_REPL_EVAL_CONFIG

Both `breakEvalOnSigint` and `eval` options were set in the REPL config, which
is not supported.
Both `breakEvalOnSigint` and `eval` options were set in the [`REPL`][] config,
which is not supported.

<a id="ERR_INVALID_REPL_INPUT"></a>
### ERR_INVALID_REPL_INPUT

The input may not be used in the [`REPL`][]. All prohibited inputs are
documented in the [`REPL`][]'s documentation.

<a id="ERR_INVALID_RETURN_PROPERTY"></a>
### ERR_INVALID_RETURN_PROPERTY
Expand Down Expand Up @@ -2307,6 +2313,7 @@ such as `process.stdout.on('data')`.
[`Class: assert.AssertionError`]: assert.html#assert_class_assert_assertionerror
[`ERR_INVALID_ARG_TYPE`]: #ERR_INVALID_ARG_TYPE
[`EventEmitter`]: events.html#events_class_eventemitter
[`REPL`]: repl.html
[`Writable`]: stream.html#stream_class_stream_writable
[`child_process`]: child_process.html
[`cipher.getAuthTag()`]: crypto.html#crypto_cipher_getauthtag
Expand Down
34 changes: 33 additions & 1 deletion doc/api/repl.md
Original file line number Diff line number Diff line change
Expand Up @@ -138,16 +138,47 @@ global or scoped variable, the input `fs` will be evaluated on-demand as
```

#### Global Uncaught Exceptions
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/27151
description: The `'uncaughtException'` event is from now on triggered if the
repl is used as standalone program.
-->

The REPL uses the [`domain`][] module to catch all uncaught exceptions for that
REPL session.

This use of the [`domain`][] module in the REPL has these side effects:

* Uncaught exceptions do not emit the [`'uncaughtException'`][] event.
* Uncaught exceptions only emit the [`'uncaughtException'`][] event if the
`repl` is used as standalone program. If the `repl` is included anywhere in
another application, adding a listener for this event will throw an
[`ERR_INVALID_REPL_INPUT`][] exception.
* Trying to use [`process.setUncaughtExceptionCaptureCallback()`][] throws
an [`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`][] error.

As standalone program:

```js
process.on('uncaughtException', () => console.log('Uncaught'));

throw new Error('foobar');
// Uncaught
```

When used in another application:

```js
process.on('uncaughtException', () => console.log('Uncaught'));
// TypeError [ERR_INVALID_REPL_INPUT]: Listeners for `uncaughtException`
// cannot be used in the REPL

throw new Error('foobar');
// Thrown:
// Error: foobar
```

#### Assignment of the `_` (underscore) variable
<!-- YAML
changes:
Expand Down Expand Up @@ -661,6 +692,7 @@ For an example of running a REPL instance over [curl(1)][], see:
[`'uncaughtException'`]: process.html#process_event_uncaughtexception
[`--experimental-repl-await`]: cli.html#cli_experimental_repl_await
[`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`]: errors.html#errors_err_domain_cannot_set_uncaught_exception_capture
[`ERR_INVALID_REPL_INPUT`]: errors.html#errors_err_invalid_repl_input
[`domain`]: domain.html
[`process.setUncaughtExceptionCaptureCallback()`]: process.html#process_process_setuncaughtexceptioncapturecallback_fn
[`readline.InterfaceCompleter`]: readline.html#readline_use_of_the_completer_function
Expand Down
1 change: 1 addition & 0 deletions lib/internal/errors.js
Original file line number Diff line number Diff line change
Expand Up @@ -895,6 +895,7 @@ E('ERR_INVALID_PROTOCOL',
TypeError);
E('ERR_INVALID_REPL_EVAL_CONFIG',
'Cannot specify both "breakEvalOnSigint" and "eval" for REPL', TypeError);
E('ERR_INVALID_REPL_INPUT', '%s', TypeError);
E('ERR_INVALID_RETURN_PROPERTY', (input, name, prop, value) => {
return `Expected a valid ${input} to be returned for the "${prop}" from the` +
` "${name}" function but got ${value}.`;
Expand Down
59 changes: 47 additions & 12 deletions lib/repl.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ const {
ERR_CANNOT_WATCH_SIGINT,
ERR_INVALID_ARG_TYPE,
ERR_INVALID_REPL_EVAL_CONFIG,
ERR_INVALID_REPL_INPUT,
ERR_SCRIPT_EXECUTION_INTERRUPTED
} = require('internal/errors').codes;
const { sendInspectorCommand } = require('internal/util/inspector');
Expand Down Expand Up @@ -101,10 +102,13 @@ let processTopLevelAwait;

const parentModule = module;
const replMap = new WeakMap();
const domainSet = new WeakSet();

const kBufferedCommandSymbol = Symbol('bufferedCommand');
const kContextId = Symbol('contextId');

let addedNewListener = false;

try {
// Hack for require.resolve("./relative") to work properly.
module.filename = path.resolve('repl');
Expand Down Expand Up @@ -204,6 +208,28 @@ function REPLServer(prompt,
throw new ERR_INVALID_REPL_EVAL_CONFIG();
}

// Add this listener only once and use a WeakSet that contains the REPLs
// domains. Otherwise we'd have to add a single listener to each REPL instance
// and that could trigger the `MaxListenersExceededWarning`.
if (!options[kStandaloneREPL] && !addedNewListener) {
process.prependListener('newListener', (event, listener) => {
if (event === 'uncaughtException' &&
process.domain &&
listener.name !== 'domainUncaughtExceptionClear' &&
domainSet.has(process.domain)) {
// Throw an error so that the event will not be added and the current
// domain takes over. That way the user is notified about the error
// and the current code evaluation is stopped, just as any other code
// that contains an error.
throw new ERR_INVALID_REPL_INPUT(
'Listeners for `uncaughtException` cannot be used in the REPL');
}
});
addedNewListener = true;
}

domainSet.add(this._domain);

let rli = this;
Object.defineProperty(this, 'rli', {
get: deprecate(() => rli,
Expand Down Expand Up @@ -264,7 +290,7 @@ function REPLServer(prompt,
// statement rather than an object literal. So, we first try
// to wrap it in parentheses, so that it will be interpreted as
// an expression. Note that if the above condition changes,
// lib/internal/repl/recoverable.js needs to be changed to match.
// lib/internal/repl/utils.js needs to be changed to match.
code = `(${code.trim()})\n`;
wrappedCmd = true;
}
Expand Down Expand Up @@ -461,22 +487,31 @@ function REPLServer(prompt,
}
}

if (errStack === '') {
errStack = `Thrown: ${self.writer(e)}\n`;
} else {
const ln = errStack.endsWith('\n') ? '' : '\n';
errStack = `Thrown:\n${errStack}${ln}`;
}

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

const top = replMap.get(self);
top.outputStream.write(errStack);
top.clearBufferedCommand();
top.lines.level = [];
top.displayPrompt();
if (options[kStandaloneREPL] &&
process.listenerCount('uncaughtException') !== 0) {
process.nextTick(() => {
process.emit('uncaughtException', e);
top.clearBufferedCommand();
top.lines.level = [];
top.displayPrompt();
});
} else {
if (errStack === '') {
errStack = `Thrown: ${self.writer(e)}\n`;
} else {
const ln = errStack.endsWith('\n') ? '' : '\n';
errStack = `Thrown:\n${errStack}${ln}`;
}
top.outputStream.write(errStack);
top.clearBufferedCommand();
top.lines.level = [];
top.displayPrompt();
}
});

self.resetContext();
Expand Down
2 changes: 0 additions & 2 deletions test/parallel/test-repl-pretty-custom-stack.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ process.on('uncaughtException', (e) => {
throw e;
});

process.on('exit', () => (Error.prepareStackTrace = origPrepareStackTrace));

const tests = [
{
// test .load for a file that throws
Expand Down
44 changes: 44 additions & 0 deletions test/parallel/test-repl-uncaught-exception-async.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict';

// This verifies that adding an `uncaughtException` listener in an REPL instance
// does not suppress errors in the whole application. Adding such listener
// should throw.

require('../common');
const ArrayStream = require('../common/arraystream');
const repl = require('repl');
const assert = require('assert');

let accum = '';

const output = new ArrayStream();
output.write = (data) => accum += data.replace('\r', '');

const r = repl.start({
prompt: '',
input: new ArrayStream(),
output,
terminal: false,
useColors: false,
global: false
});

r.write(
'process.nextTick(() => {\n' +
' process.on("uncaughtException", () => console.log("Foo"));\n' +
' throw new TypeError("foobar");\n' +
'});\n'
);
r.write(
'setTimeout(() => {\n' +
' throw new RangeError("abc");\n' +
'}, 1);console.log()\n'
);
r.close();

setTimeout(() => {
const len = process.listenerCount('uncaughtException');
process.removeAllListeners('uncaughtException');
assert.strictEqual(len, 0);
assert(/ERR_INVALID_REPL_INPUT.*(?!Type)RangeError: abc/s.test(accum));
}, 2);
38 changes: 38 additions & 0 deletions test/parallel/test-repl-uncaught-exception-standalone.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
'use strict';
const common = require('../common');
const assert = require('assert');
const cp = require('child_process');
const child = cp.spawn(process.execPath, ['-i']);
let output = '';

child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
output += data;
});

child.on('exit', common.mustCall(() => {
const results = output.split('\n');
results.shift();
assert.deepStrictEqual(
results,
[
'Type ".help" for more information.',
// x\n
'> Thrown:',
'ReferenceError: x is not defined',
// Added `uncaughtException` listener.
'> short',
'undefined',
// x\n
'> Foobar',
'> '
]
);
}));

child.stdin.write('x\n');
child.stdin.write(
'process.on("uncaughtException", () => console.log("Foobar"));' +
'console.log("short")\n');
child.stdin.write('x\n');
child.stdin.end();
72 changes: 72 additions & 0 deletions test/parallel/test-repl-uncaught-exception.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
'use strict';
require('../common');
const ArrayStream = require('../common/arraystream');
const assert = require('assert');
const repl = require('repl');

let count = 0;

function run({ command, expected }) {
let accum = '';

const output = new ArrayStream();
output.write = (data) => accum += data.replace('\r', '');

const r = repl.start({
prompt: '',
input: new ArrayStream(),
output,
terminal: false,
useColors: false
});

r.write(`${command}\n`);
if (typeof expected === 'string') {
assert.strictEqual(accum, expected);
} else {
assert(expected.test(accum), accum);
}

// Verify that the repl is still working as expected.
accum = '';
r.write('1 + 1\n');
assert.strictEqual(accum, '2\n');
r.close();
count++;
}

const tests = [
{
command: 'x',
expected: 'Thrown:\n' +
'ReferenceError: x is not defined\n'
},
{
command: 'process.on("uncaughtException", () => console.log("Foobar"));\n',
expected: /^Thrown:\nTypeError \[ERR_INVALID_REPL_INPUT]: Listeners for `/
},
{
command: 'x;\n',
expected: 'Thrown:\n' +
'ReferenceError: x is not defined\n'
},
{
command: 'process.on("uncaughtException", () => console.log("Foobar"));' +
'console.log("Baz");\n',
expected: /^Thrown:\nTypeError \[ERR_INVALID_REPL_INPUT]: Listeners for `/
},
{
command: 'console.log("Baz");' +
'process.on("uncaughtException", () => console.log("Foobar"));\n',
expected: /^Baz\nThrown:\nTypeError \[ERR_INVALID_REPL_INPUT]:.*uncaughtException/
}
];

process.on('exit', () => {
// To actually verify that the test passed we have to make sure no
// `uncaughtException` listeners exist anymore.
process.removeAllListeners('uncaughtException');
assert.strictEqual(count, tests.length);
});

tests.forEach(run);

0 comments on commit 422e8f7

Please sign in to comment.