Node: Promise.reject() crashes repl when using --unhandled-rejections=strict

[dfabulich]

• Version: 14.11.0
• Platform: macOS Catalina 10.15.6

I have also reproduced this in Node 12.18.4.

What steps will reproduce the bug?

Launch the repl with this command:

node --unhandled-rejections=strict

(--unhandled-rejections=throw has the same issue.)

On the repl command line, type Promise.reject().

What is the expected behavior?

When an uncaught error is thrown in the repl, the repl should print "Uncaught error" without terminating the process. For example:

> throw new Error()
Uncaught Error
> void setTimeout(_=>{throw new Error()}, 0)
undefined
> Uncaught Error

What do you see instead?

The process crashes.

> Promise.reject()
Promise { <rejected> undefined }
> internal/process/promises.js:213
        triggerUncaughtException(err, true /* fromPromise */);
        ^

[UnhandledPromiseRejection: This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). The promise rejected with the reason "undefined".] {
  code: 'ERR_UNHANDLED_REJECTION'
}

Additional information

This bug will become more important in Node 15 when --unhandled-rejections=throw becomes the default, per PR #33021. At that point, the bug will repro when launching node using the default settings with no flags.

[dfabulich]

FWIW, this bug is not a regression. It happens in Node 12 as well.

I'm curious to take a crack at this, but I have no idea where to begin. Can anyone suggest a starting point? (How do repl tests work…?)

[mmarchini]

Should repl ignore the flag as use warn ~/none~ instead? That's more similar to what happens with sync errors

[BridgeAR]

Setting it to none would silence such issues in the REPL and the main issue is the programmatic REPL instances. If these are started and we just set the flag to none, rejections in the "regular code" are also silenced. Otherwise it's a good solution (if set to warn).

[dfabulich]

That would fix the crash, but it would be a little noisy.

$ node --unhandled-rejections=warn
Welcome to Node.js v14.11.0.
Type ".help" for more information.
> Promise.reject()
Promise { <rejected> undefined }
> (node:66834) UnhandledPromiseRejectionWarning: undefined
(Use `node --trace-warnings ...` to show where the warning was created)
(node:66834) UnhandledPromiseRejectionWarning: Unhandled promise rejection. This error originated either by throwing inside of an async function without a catch block, or by rejecting a promise which was not handled with .catch(). To terminate the node process on unhandled promise rejection, use the CLI flag `--unhandled-rejections=strict` (see https://nodejs.org/api/cli.html#cli_unhandled_rejections_mode). (rejection id: 1)

I feel like I'd want to make the repl set an unhandledRejections hook to log a warning that wasn't so noisy.

Is there a good place I could add code like that?

[devsnek]

with throw mode, it should bubble up as an uncaught exception, the same way throwing does.

[mmarchini]

@devsnek yeah I'm a bit surprised with the behavior here honestly. I won't have time to look into it during the week but might have some time during the weekend.

[BridgeAR]

The reason why rejections behave differently is that we use domains to track uncaught exceptions in the REPL.

[mmarchini]

Didn't had time to look into it over the weekend, and I'm not sure if I'll have time to look into it during the week, so if anyone else wants to take a look go ahead. As a workaround we should probably let it to warn, and then keep this issue open to investigate making it work without changing the default.

[Aplet123]

With the new defaults added in v15, this issue is especially pertinent.

[benjamingr]

This is quite the blast from the past, I haven't touched domains in several years ^^ I will take a look.

[benjamingr]

So the issue appears to be that process.domain is not set to the domain of the repl inside unhandledRejection so a fix like:

process.on('unhandledRejection', (e, p) => {
    p.catch((e) => {
      if (process.domain === self._domain) {
        self._domain.emit('error', e);
      }
    });
  });

Wouldn't work. I think we'd have to parse the stack trace which sucks but I don't really see an alternative, that wouldn't work with Promise.reject('foo') which has no stack.

Also we should probablyt refactor the REPL to not use domains 🤷

[benjamingr]

Ok so the issue is:

• When we set the promise to pendingUnhandledRejections its domain is set correctly (i.e. promise.catch(e => console.log(process.domain)) is the REPL domain.
• However, we defer a microtick - so when we run the microticks and process unhandled rejections the domain context is lost.

Rather than fix domains: I think the REPL needs to set a (separate) setPromiseRejectCallback and catch the promise "earlier" when the domain is still configured rather than using our "unhandled rejection" microtick heuristic.

Edit: Actually I'll attempt a more general fix first

[benjamingr]

@BridgeAR @addaleax how would you feel about emitting unhandledRejection on the domain the promise belongs to, i.e. this patch:

diff --git a/lib/internal/process/promises.js b/lib/internal/process/promises.js
index 2b806e7e8e..7c7ce4bba5 100644
--- a/lib/internal/process/promises.js
+++ b/lib/internal/process/promises.js
@@ -117,7 +117,8 @@ function unhandledRejection(promise, reason) {
   maybeUnhandledPromises.set(promise, {
     reason,
     uid: ++lastPromiseId,
-    warned: false
+    warned: false,
+    domain: process.domain
   });
   // This causes the promise to be referenced at least for one tick.
   pendingUnhandledRejections.push(promise);
@@ -192,26 +193,32 @@ function processPromiseRejections() {
     }
     promiseInfo.warned = true;
     const { reason, uid } = promiseInfo;
+    function emit(reason, promise, promiseInfo) {
+      if (promiseInfo.domain) {
+        return promiseInfo.domain.run(() => process.emit('unhandledRejection', reason, promise));
+      }
+      return process.emit('unhandledRejection', reason, promise);
+    }
     switch (unhandledRejectionsMode) {
       case kStrictUnhandledRejections: {
         const err = reason instanceof Error ?
           reason : generateUnhandledRejectionError(reason);
         triggerUncaughtException(err, true /* fromPromise */);
-        const handled = process.emit('unhandledRejection', reason, promise);
+        const handled = emit(reason, promise, promiseInfo);
         if (!handled) emitUnhandledRejectionWarning(uid, reason);
         break;
       }
       case kIgnoreUnhandledRejections: {
-        process.emit('unhandledRejection', reason, promise);
+        emit(reason, promise, promiseInfo);
         break;
       }
       case kAlwaysWarnUnhandledRejections: {
-        process.emit('unhandledRejection', reason, promise);
+        emit(reason, promise, promiseInfo);
         emitUnhandledRejectionWarning(uid, reason);
         break;
       }
       case kThrowUnhandledRejections: {
-        const handled = process.emit('unhandledRejection', reason, promise);
+        const handled = emit(reason, promise, promiseInfo);
         if (!handled) {
           const err = reason instanceof Error ?
             reason : generateUnhandledRejectionError(reason);
@@ -220,7 +227,7 @@ function processPromiseRejections() {
         break;
       }
       case kWarnWithErrorCodeUnhandledRejections: {
-        const handled = process.emit('unhandledRejection', reason, promise);
+        const handled = emit(reason, promise, promiseInfo);
         if (!handled) {
           emitUnhandledRejectionWarning(uid, reason);
           process.exitCode = 1;
@@ -266,10 +273,9 @@ function generateUnhandledRejectionError(reason) {
 function listenForRejections() {
   setPromiseRejectCallback(promiseRejectHandler);
 }
-
 module.exports = {
   hasRejectionToWarn,
   setHasRejectionToWarn,
   listenForRejections,
-  processPromiseRejections
+  processPromiseRejections,
 };
diff --git a/lib/repl.js b/lib/repl.js
index 78c256d60b..a18fe7727a 100644
--- a/lib/repl.js
+++ b/lib/repl.js
@@ -541,6 +541,13 @@ function REPLServer(prompt,

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

+  process.on('unhandledRejection', (e, p) => {
+    if (process.domain === self._domain) {
+      self._domain.emit('error', e);
+    } else {
+      throw e;
+    }
+  });
   self._domain.on('error', function debugDomainError(e) {
     debug('domain error');
     let errStack = '';

[benjamingr]

Talking to Anna, she suggested emitting the ereor on the domain if unhandledRejection happens inside a domain. I think it's probably the better idea so I'll amend to that.

[benjamingr]

This should be fixed in the next version.