From cbd8eece0ebac7ad0423fd5737f6695f635de163 Mon Sep 17 00:00:00 2001
From: Ruben Bridgewater <ruben@bridgewater.de>
Date: Thu, 17 May 2018 16:25:36 +0200
Subject: [PATCH] repl: properly handle uncaughtException

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: https://github.com/nodejs/node/issues/19998
---
 doc/api/errors.md                             | 11 ++-
 doc/api/repl.md                               | 34 ++++++++-
 lib/internal/errors.js                        |  1 +
 lib/repl.js                                   | 59 +++++++++++----
 .../parallel/test-repl-pretty-custom-stack.js |  2 -
 .../test-repl-uncaught-exception-async.js     | 42 +++++++++++
 ...test-repl-uncaught-exception-standalone.js | 38 ++++++++++
 test/parallel/test-repl-uncaught-exception.js | 72 +++++++++++++++++++
 8 files changed, 242 insertions(+), 17 deletions(-)
 create mode 100644 test/parallel/test-repl-uncaught-exception-async.js
 create mode 100644 test/parallel/test-repl-uncaught-exception-standalone.js
 create mode 100644 test/parallel/test-repl-uncaught-exception.js

diff --git a/doc/api/errors.md b/doc/api/errors.md
index 79570ffb510ea9..d3266c256f617b 100644
--- a/doc/api/errors.md
+++ b/doc/api/errors.md
@@ -1310,8 +1310,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 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
@@ -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
diff --git a/doc/api/repl.md b/doc/api/repl.md
index e0ec27b7e3bada..563817c632297f 100644
--- a/doc/api/repl.md
+++ b/doc/api/repl.md
@@ -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:
@@ -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
diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 2cdf40e2092933..88ab387e4a436d 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -884,6 +884,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}.`;
diff --git a/lib/repl.js b/lib/repl.js
index a7eb22e396e847..eb48eff81d8d65 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -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');
@@ -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');
@@ -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,
@@ -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;
     }
@@ -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();
diff --git a/test/parallel/test-repl-pretty-custom-stack.js b/test/parallel/test-repl-pretty-custom-stack.js
index a37cf04263433d..b65287c5f55b1f 100644
--- a/test/parallel/test-repl-pretty-custom-stack.js
+++ b/test/parallel/test-repl-pretty-custom-stack.js
@@ -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
diff --git a/test/parallel/test-repl-uncaught-exception-async.js b/test/parallel/test-repl-uncaught-exception-async.js
new file mode 100644
index 00000000000000..15abea1b3dcf53
--- /dev/null
+++ b/test/parallel/test-repl-uncaught-exception-async.js
@@ -0,0 +1,42 @@
+'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(() => {
+  assert(/ERR_INVALID_REPL_INPUT.*(?!Type)RangeError: abc/s.test(accum));
+  assert.strictEqual(process.listenerCount('uncaughtException'), 0);
+}, 2);
diff --git a/test/parallel/test-repl-uncaught-exception-standalone.js b/test/parallel/test-repl-uncaught-exception-standalone.js
new file mode 100644
index 00000000000000..ae7b214fefae9e
--- /dev/null
+++ b/test/parallel/test-repl-uncaught-exception-standalone.js
@@ -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();
diff --git a/test/parallel/test-repl-uncaught-exception.js b/test/parallel/test-repl-uncaught-exception.js
new file mode 100644
index 00000000000000..447b69b1d4a29e
--- /dev/null
+++ b/test/parallel/test-repl-uncaught-exception.js
@@ -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);