process: Change default --unhandled-rejections=throw

[dfabulich]

This is a semver-major change that resolves DEP0018.

All users that have set an unhandledRejection hook or set a non-default
value for the --unhandled-rejections flag will see no change in behavior
after this change.

Fixes: #20392
Refs: https://nodejs.org/dist/latest/docs/api/deprecations.html#deprecations_dep0018_unhandled_promise_rejections

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[dfabulich]

While working on PR #32986, I found that I had to do an unnecessarily large amount of work updating the tests. This first led me to implement PR #33017 as a stepping stone, but @BridgeAR expressed disapproval, so I figured I'd close PR #32986 and open this PR.

This PR, I claim, is the simplest thing that could possibly work. I think it will be more appealing than my earlier PR #32986 because it provides a way for users who don't want strict mode to escape it: users can set an unhandledRejection hook and get full control over the default behavior.

At the same time, users who set a CLI flag will maintain full control over unhandled rejections, even if userland code attempts to override it.

• If you're one of the folks who think rejected Promises aren't really exceptional, that's fine! You can use this one-liner to get the behavior you want: process.on('unhandledRejection', () => {}). (You probably already have a line like that in your code today, to suppress the unsightly UnhandledPromiseRejectionWarning and the deprecation notice.) You can also use the CLI flag --unhandled-rejections=none, if that suits you better.
• If you care about ensuring your environment is consistent and that random hooks can't change the behavior of your unhandled rejections, you can keep using the CLI flags you've been using since Node 10. None of the explicit flag values (none, warn, strict) have changed their behavior, and they all continue to override whatever behavior is set in the unhandledRejections hook. (Only the old default behavior honored that hook; the new default behavior continues to honor the hook.)
• If you're just a humble cavedweller like me, and you just want to terminate the process when you throw an uncaught Error in an async function, this PR is for you!

[devsnek]

I have the same blocking concerns here as I did in the other PR.

[mcollina]

@devsnek would you mind to add a link?

[mcollina]

lgtm

[mcollina]

cc @nodejs/tsc

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/30931/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2358/

[ljharb]

This should never be on by default; the language spec does not intend Promise.reject(), or the equivalent, to inherently crash the process under any circumstances.

[dfabulich]

The language spec does not intend throw new Error() to inherently crash the process under any circumstances, even in synchronous code. But in Node, it's useful to have throw new Error() crash the process. It is just as useful to have throw new Error() crash the process in an async function. This is why crowds keep forming around this feature.

[ljharb]

I don't agree - an uncaught throw is intended to catch the process. Promise rejections are not the same as synchronous thrown exceptions, and shouldn't be treated the same.

I'm not objecting to the (existing) ways people can opt in to making the process exit, on any criteria they want. By default, however, this would be a very destructive setting.

[dfabulich]

an uncaught throw is intended to [crash] the process

Can you provide a link? There's no statement about this in any version of the ECMAScript specification that I'm aware of. (But of course there wouldn't be. Browsers never "crash the process.")

Node allows users to attach an uncaughtException event handler and resolve it in a way that doesn't crash the process. This doesn't bring Node out of compliance with the language spec, because the spec doesn't say what the runtime is supposed to do with an uncaught exception at all.

this would be a very destructive setting

"Destructive" isn't the right word to use here. This PR will have no effect on anyone using the --unhandled-rejections flag today, or on anyone who uses the unhandledRejections hook. It will literally only impact people who are currently seeing DEP0018 in their warnings.

Everyone who has seen that warning since Node 10 was released and has taken some action to suppress it, any action at all, will not be impacted by this PR.

[dfabulich]

Load two scripts, throw a synchronous exception without catching on the first, the second won't run

That's not true. In this jsbin, the second script runs.

https://jsbin.com/juxinik/1/edit?html,output

(edit: pasted wrong link)

[mmarchini]

Oh, interesting. I swear I had issues with that in the past, might be confusing with something else though.

[mcollina]

I would note that this definitely helps developers, and it is the default behavior in Deno. My opinion is that we should get this landed for v15.

[jasnell]

While I have many issues with Promises and absolutely think that crashing on unhandled rejections is good for developers, I'm less convinced that we should crash by default and I do think we should remove the existing deprecation warning. The default behavior should continue to be to warn on the unhandled rejection.

So, just to be clear, I'm -1 on this PR

[addaleax]

@jasnell How do you feel about #32986 (comment)?

[jasnell]

Specifically on setting exitCode = 1 on unhandled rejection? I think that's reasonable but without additional information it could make it difficult to reason about what caused the exitCode to be non-zero. It could become a bit of a nightmare to diagnose.

[mmarchini]

I'm in favor of raising an exception by default (so +1 on this PR). As a fallback, I'm in favor of removing the deprecation and keep the warning, as long as the process returns an error code by default when it exits if there are pending unhandled rejections. I'm -1 on just removing the deprecation and exiting successfully.

[addaleax]

@jasnell We do currently say in the deprecation warning that that’s what we’re going to do, and we could still keep it in the warning text (to be clear, I’m not suggesting to remove the warning, ever) 🤷‍♀️

[devsnek]

I'm also in favor of the default mode continuing to be warn.

[jasnell]

Thinking through this while discussing on the tsc call, I think the process that I would like to see here is:

1. Setting strict mode would take highest priority and would throw and crash immediately.
2. Otherwise, if the unhandledRejection handler is in place, pass the rejection to that with no warning emitted.
3. Otherwise, if the throw mode (what this PR calls default) is set, throw the rejection as an uncaught exception.
4. Otherwise, if the none mode is set, do nothing.
5. Otherwise, emit the unhandledRejection warning and set exitCode = 1
6. Remove the existing deprecation warning.

[ljharb]

@jasnell my position (and one which imo reflects the intention of the JS spec) is that the none mode should be the default.

[devsnek]

@ljharb I'm not so sure about that... HostPromiseRejectionTracker exists for a reason.

[benjamingr]

And... 5 years later, I feel like my biggest regret regarding promises early on in Node was finally fixed 🎉 Chapeau!

If anyone is wondering there is still a ton of interesting work to do in this space and improvement ideas/suggestions.

[Fishrock123]

Also 5 years later, I still believe this is the correct choice for Node.js. Thank you.

[luiscastro193]

For all the folks who, like me, prefer the behaviour to be more consistent with the browser ecosystem, I recommend the following line:

process.on('unhandledRejection', reason => console.trace(reason));

[Fishrock123]

I believe that is similar to setting --unhandled-rejections=warn, which can also be set via the NODE_OPTIONS environment variable.

[benjamingr]

@luiscastro193 note that in order to be consistent with browsers it is sufficient to do process.on('uncaughtException', reason => console.trace(reason)); since I believe unhandled rejections end up as uncaught exceptions.

Also note that the default changed because not terminating the process is unsafe and leads to possible cascading failures - so beware dragons ahead.

[mmarchini]

It's worth noting that console.trace will add frames to the stacktrace which are unrelated to where the rejection happened, which might be undesired behavior to some users:

$ node -e 'process.on("unhandledRejection", function iShouldntShowOnStackTraceYetHereIAm(e) { console.trace(e) }); function foo() { Promise.reject(new Error()) } foo()'
Trace: Error
    at foo ([eval]:1:137)
    at [eval]:1:152
    at Script.runInThisContext (vm.js:120:18)
    at Object.runInThisContext (vm.js:309:38)
    at Object.<anonymous> ([eval]-wrapper:10:26)
    at Module._compile (internal/modules/cjs/loader.js:1015:30)
    at evalScript (internal/process/execution.js:94:25)
    at internal/main/eval_string.js:23:3
    at process.iShouldntShowOnStackTraceYetHereIAm ([eval]:1:92)
    at process.emit (events.js:314:20)
    at processPromiseRejections (internal/process/promises.js:209:33)
    at processTicksAndRejections (internal/process/task_queues.js:98:32)

console.warn might be a better option for most users:

$ node -e 'process.on("unhandledRejection", function iShouldntShowOnStackTraceYetHereIAm(e) { console.warn(e) }); function foo() { Promise.reject(new Error()) } foo()'
Error
    at foo ([eval]:1:136)
    at [eval]:1:151
    at Script.runInThisContext (vm.js:120:18)
    at Object.runInThisContext (vm.js:309:38)
    at Object.<anonymous> ([eval]-wrapper:10:26)
    at Module._compile (internal/modules/cjs/loader.js:1015:30)
    at evalScript (internal/process/execution.js:94:25)
    at internal/main/eval_string.js:23:3