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

repl: emit uncaughtException #20803

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
11 changes: 9 additions & 2 deletions doc/api/errors.md
Original file line number Diff line number Diff line change
Expand Up @@ -1308,8 +1308,14 @@ An invalid `options.protocol` was passed.
<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 can or may not be used in the [`REPL`][]. All prohibited inputs are
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what is meant by "can or may not be used".

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought I'll keep it broad so that it could be used for different REPL input. So far it will only be used for uncaught exception listeners, so only the may part applies. I am happy to change it to only may or what ever you have as alternative.

documented in the [`REPL`][]'s documentation.

<a id="ERR_INVALID_RETURN_PROPERTY"></a>
### ERR_INVALID_RETURN_PROPERTY
Expand Down Expand Up @@ -2293,6 +2299,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/20803
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 this event synchronous will throw an
Copy link
Member

Choose a reason for hiding this comment

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

What is meant by "adding this event synchronous"?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is actually outdated in the other PR. In this version I was not able to actually handle listeners that were added asynchronously. E.g., setTimeout(() => process.on("uncaughtException", () => {})) was not detected here. This is fixed in the latest implementation.

[`ERR_INVALID_REPL_INPUT`][] error!
* Trying to use [`process.setUncaughtExceptionCaptureCallback()`][] throws
an [`ERR_DOMAIN_CANNOT_SET_UNCAUGHT_EXCEPTION_CAPTURE`][] error.

As standalone program:
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved

```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]: Unhandled exception listeners can not be
Copy link
Member

Choose a reason for hiding this comment

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

Nit: perhaps "Listeners for uncaughtException cannot be used in the REPL" is a bit clearer.

// 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 @@ -892,6 +892,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
71 changes: 58 additions & 13 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 @@ -102,6 +103,20 @@ const replMap = new WeakMap();
const kBufferedCommandSymbol = Symbol('bufferedCommand');
const kContextId = Symbol('contextId');

let tmpListeners = [];
let checkUncaught = false;
let currentListeners = [];
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved

process.on('newListener', (event, listener) => {
if (event === 'uncaughtException' &&
checkUncaught &&
!currentListeners.includes(listener)) {
// Add listener to a temporary list of listeners which should be removed
// again.
tmpListeners.push(listener);
}
});

try {
// Hack for require.resolve("./relative") to work properly.
module.filename = path.resolve('repl');
Expand Down Expand Up @@ -268,7 +283,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 @@ -428,7 +443,6 @@ function REPLServer(prompt,
}

self.eval = self._domain.bind(eval_);

self._domain.on('error', function debugDomainError(e) {
debug('domain error');
let errStack = '';
Expand Down Expand Up @@ -465,22 +479,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(() => {
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved
process.emit('uncaughtException', e);
top.clearBufferedCommand();
BridgeAR marked this conversation as resolved.
Show resolved Hide resolved
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 Expand Up @@ -666,10 +689,32 @@ function REPLServer(prompt,
const evalCmd = self[kBufferedCommandSymbol] + cmd + '\n';

debug('eval %j', evalCmd);
if (!options[kStandaloneREPL]) {
checkUncaught = true;
currentListeners = process.listeners('uncaughtException');
}
self.eval(evalCmd, self.context, 'repl', finish);

function finish(e, ret) {
debug('finish', e, ret);

if (tmpListeners.length !== 0) {
tmpListeners.forEach((tmp) => {
process.removeListener('uncaughtException', tmp);
});
tmpListeners = [];
const err = new ERR_INVALID_REPL_INPUT(
'Unhandled exception listeners can not be used in the REPL');
self._domain.emit('error', err);
if (ret === process) {
self.clearBufferedCommand();
self.displayPrompt();
return;
}
}
currentListeners = [];
checkUncaught = false;

_memory.call(self, cmd);

if (e && !self[kBufferedCommandSymbol] && cmd.trim().startsWith('npm ')) {
Expand Down
61 changes: 61 additions & 0 deletions test/known_issues/test-repl-uncaught-exception-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
'use strict';

require('../common');

// Adding an `uncaughtException` listener in an REPL instance suppresses errors
// in the whole application.
// Closing the instance won't remove those listeners either.

if (process.argv[2] === 'child') {

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

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(
'setTimeout(() => {\n' +
' process.on("uncaughtException", () => console.log("Foo"));\n' +
'}, 1);\n'
);

// Event listeners added to the global `process` won't be removed after
// closing the REPL instance! Those should be removed again, especially since
// the REPL's `global` option is set to `false`.
r.close();

setTimeout(() => {
// This should definitely not be silenced!
throw new Error('HU');
}, 2);

setTimeout(() => {
console.error('FAILED');
process.exit(1);
}, 10);

return;
}

const cp = require('child_process');
const assert = require('assert');

const res = cp.spawnSync(
process.execPath,
[__filename, 'child'],
{ encoding: 'utf8' }
);

assert.notStrictEqual(res.stderr, 'FAILED\n');
3 changes: 0 additions & 3 deletions test/parallel/test-repl-pretty-custom-stack.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ const fixtures = require('../common/fixtures');
const assert = require('assert');
const repl = require('repl');


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

Expand Down Expand Up @@ -40,8 +39,6 @@ process.on('uncaughtException', (e) => {
throw e;
});

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

Choose a reason for hiding this comment

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

This seems unrelated to the PR changes. What am I missing?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is somewhat unrelated. In a former PR this test was impacted as well and I noticed that this is actually not necessary, so I just went ahead and removed it.


const tests = [
{
// test .load for a file that throws
Expand Down
33 changes: 33 additions & 0 deletions test/parallel/test-repl-uncaught-exception-standalone.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
'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.replace(/^> /mg, '').split('\n');
assert.deepStrictEqual(
results,
[
'Thrown:',
'ReferenceError: x is not defined',
'short',
'undefined',
'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();
60 changes: 60 additions & 0 deletions test/parallel/test-repl-uncaught-exception.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
'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);
}
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]: Unhandled exception/
},
{
command: 'x;\n',
expected: 'Thrown:\n' +
'ReferenceError: x is not defined\n'
},
{
command: 'process.on("uncaughtException", () => console.log("Foobar"));' +
'console.log("Baz");\n',
// eslint-disable-next-line node-core/no-unescaped-regexp-dot
expected: /^Baz(.|\n)*ERR_INVALID_REPL_INPUT/
}
];

process.on('exit', () => {
assert.strictEqual(count, tests.length);
});

tests.forEach(run);