Repository navigation
Promise.reject() crashes repl when using --unhandled-rejections=strict #35232
Description
Activity
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…?)
Should repl ignore the flag as use warn
/noneinstead? That's more similar to what happens with sync errorsSetting 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).
Reacted by mary marchiniThat 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
unhandledRejectionshook to log a warning that wasn't so noisy.Is there a good place I could add code like that?
with throw mode, it should bubble up as an uncaught exception, the same way throwing does.
The reason why rejections behave differently is that we use domains to track uncaught exceptions in the REPL.
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.
With the new defaults added in v15, this issue is especially pertinent.
Reacted by Vse Mozhe Buty and moghwan- pinned this issue
on Nov 1, 2020 This is quite the blast from the past, I haven't touched domains in several years ^^ I will take a look.
So the issue appears to be that
process.domainis not set to the domain of the repl insideunhandledRejectionso 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 🤷
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)
setPromiseRejectCallbackand 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
- When we set the promise to pendingUnhandledRejections its domain is set correctly (i.e.
Talking to Anna, she suggested emitting the ereor on the domain if
unhandledRejectionhappens inside a domain. I think it's probably the better idea so I'll amend to that.This should be fixed in the next version.
Reacted by Dan Fabulich, mary marchini, Jason An and Ruben Bridgewater- unpinned this issue
on Nov 20, 2020 is this fixed? I'm still seeing this on windows 10 with node v15.6.0
behavior is the same with and without flag
node --unhandled-rejections=strict index.js
and
node index.jsnode:internal/process/promises:208 triggerUncaughtException(err, true /* fromPromise */); ^ Error: read ECONNRESET at TLSWrap.onStreamRead (node:internal/stream_base_commons:211:20) { errno: -4077, code: 'ECONNRESET', syscall: 'read' }Reacted by danforejtek, faizanqubaz and Himanshu DabhiRunning a file isn't an issue since it's supposed to error and stop (that's the whole point of the flag). What might be considered an issue (although I'm not sure if this is very important) is that
--unhandled-rejections=strictstill exits out of the REPL on a promise rejection.This should be fixed in the next version.
I'm getting this in current lts 16.14.0 what would be the 'next version' your referring too ?
Reacted by Dmitry Gorelenkov, Eugene, Murat Çimen, maciek_grzybek and Maytee Sánchez(i do not get this in v14.17.0)
I have also reproduced this in Node 12.18.4.
What steps will reproduce the bug?
Launch the repl with this command:
(
--unhandled-rejections=throwhas 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:
What do you see instead?
The process crashes.
Additional information
This bug will become more important in Node 15 when
--unhandled-rejections=throwbecomes the default, per PR #33021. At that point, the bug will repro when launchingnodeusing the default settings with no flags.