process: add --unhandled-rejections flag by BridgeAR · Pull Request #26599 · nodejs/node · GitHub

This adds a flag to define the default behavior for unhandled
rejections. Three modes exist: none, warn and strict. The first
is going to silence all unhandled rejection warnings. The second
behaves identical to the current default with the excetion that no
deprecation warning will be printed and the last is going to throw
an error for each unhandled rejection, just as regular exceptions do.
It is possible to intercept those with the uncaughtException hook
as with all other exceptions as well.

This PR has no influence on the existing unhandledRejection hook.
If that is used, it will continue to function as before.

Supersedes #20097

// CC @nodejs/diagnostics @nodejs/promises-debugging @nodejs/tsc

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

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/2871/pipeline

Code changes and proposed modes LGTM. If you don't mind, I want to investigate how this works with postmortem/llnode before we land. I'm not sure calling process.abort() is the best approach in this case and I think the stackTraceLimit reset trick to speed up Error creation might interfere with recent developments to make llnode more useful when investigating unhandled promise rejections (see https://chromium-review.googlesource.com/c/v8/v8/+/1405568 and nodejs/llnode#271).

Although I'd still prefer the deprecation warning be removed in the default case, I'm glad it's removed when an explicit flag is passed.

LGTM

I had to rebase due to conflicts, so PTAL.

CI https://ci.nodejs.org/job/node-test-pull-request/21457/

From a postmortem/llnode perspective (with the flags --unhandled-rejections=strict and --abort-on-uncaught-exception set):

The stack we get on the output is correct in most common cases (when we reject or throw inside a Promise/async function by passing an error to reject/throw.
The ReportException called inside TriggerFatalException will turn the JSArray stack stored in the Error object into its stringified version, and the frames stored in the JSArray will be deleted. This will render src: inspcet arguments from an Error object stack llnode#271 useless, but this is how V8 works and I think we should make changes there instead of blocking this PR because of it.
The stack we get on llnode (by using v8 bt) is not very useful, but this was expected and should be addressed by 2.

LGTM, the only thing I think we should change is on V8 and not on Node.js, so I'll open an upstream PR for that.

Edit: Upstream PR, if anyone is interested: https://chromium-review.googlesource.com/c/v8/v8/+/1518612

LGTM once the default behaviour is clarified.

I improved the documentation and the warn I reworked the warn mode a tiny bit: it will now always warn (but without the deprecation notice), even if there is an unhandled rejections on in place.

The changes are in the 18c420d and a2eb1ea.

CI https://ci.nodejs.org/job/node-test-pull-request/21501/

@BridgeAR so to clarify, warn doesn't show the deprecation warning and it ignores handlers (assuming they don't actually handle the rejection) and the default did not change?

@benjamingr

it ignores handlers

Exactly

assuming they don't actually handle the rejection

That's a good point. I forgot that they might actually add a handler. I'll add a fix for that so I verify that the promise is indeed still unhandled before printing the warning.

the default did not change

There is absolutely no behavior change if non of the flags is used. That is one of the goals with this PR.

Thinking about it again: I would rather always warn, even if the unhandled rejection hook attached a handler. Otherwise it would a) be possible to silence those warnings with the hook which I think is orthogonal to what the user wants if they use the warn mode and b) it increases the complexity for the implementation (I have to wait another tick and add extra information and then check if it got handled in the meanwhile).

Explicit "still lgtm"

CI https://ci.nodejs.org/job/node-test-pull-request/21504/

CI https://ci.nodejs.org/job/node-test-pull-request/21570/

@mmarchini @mcollina @mhdawson @benjamingr would you be so kind and confirm your LG? To me this seems ready to land.

Still seems good to me!

Perhaps could this flag be set by an environment variable? That way it would be able to affect shebangs in files, or nested node commands i might not be able to edit.

This adds a flag to define the default behavior for unhandled rejections. Three modes exist: `none`, `warn` and `strict`. The first is going to silence all unhandled rejection warnings. The second behaves identical to the current default with the excetion that no deprecation warning will be printed and the last is going to throw an error for each unhandled rejection, just as regular exceptions do. It is possible to intercept those with the `uncaughtException` hook as with all other exceptions as well. This PR has no influence on the existing `unhandledRejection` hook. If that is used, it will continue to function as before. PR-URL: nodejs#26599 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

This adds a flag to define the default behavior for unhandled rejections. Three modes exist: `none`, `warn` and `strict`. The first is going to silence all unhandled rejection warnings. The second behaves identical to the current default with the excetion that no deprecation warning will be printed and the last is going to throw an error for each unhandled rejection, just as regular exceptions do. It is possible to intercept those with the `uncaughtException` hook as with all other exceptions as well. This PR has no influence on the existing `unhandledRejection` hook. If that is used, it will continue to function as before. PR-URL: #26599 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Matheus Marchini <mat@mmarchini.me> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>

Notable changes: * crypto: * add support for chacha20-poly1305 for AEAD (chux0519) #24081 * increase maxmem range from 32 to 53 bits (Tobias Nießen) #28799 * deps: * update npm to 6.11.3 (claudiahdz) #29430 * upgrade openssl sources to 1.1.1d (Sam Roberts) #29921 * dns: * remove dns.promises experimental warning (cjihrig) #26592 * fs: * remove experimental warning for fs.promises (Anna Henningsen) #26581 * http: * makes response.writeHead return the response (Mark S. Everitt) #25974 * http2: * makes response.writeHead return the response (Mark S. Everitt) #25974 * n-api: * make func argument of napi\_create\_threadsafe\_function optional (legendecas) #27791 * mark version 5 N-APIs as stable (Gabriel Schulhof) #29401 * implement date object (Jarrod Connolly) #25917 * process: * add --unhandled-rejections flag (Ruben Bridgewater) #26599 * stream: * implement Readable.from async iterator utility (Guy Bedford) #27660 * make Symbol.asyncIterator support stable (Matteo Collina) #26989 PR-URL: #29875

--unhandled-rejections has three explicit modes (strict, warn, none) plus one implicit "default" mode, which logs an additional deprecation warning (DEP0018). Prior to this commit, the default mode was subtly different from warn mode. If the unhandledRejections hook is set, default mode suppresses all warnings. In warn mode, unhandledRejections would always fire a warning, regardless of whether the hook was set. In addition, prior to this commit, strict mode would always throw an exception, regardless of whether the hook was set. In this commit, all modes honor the unhandledRejections hook. If the user has set the hook, then the user has taken full responsibility over the behavior of unhandled rejections. In that case, no additional warnings or thrown exceptions will be fired, even in warn mode or strict mode. This commit is a stepping stone towards resolving DEP0018. After this commit, any code that includes an unhandledRejection hook will behave unchanged when we change the default mode. Refs: nodejs#26599

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 11, 2019

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. labels Mar 11, 2019

BridgeAR mentioned this pull request Mar 11, 2019

src: exit on gc unhandled rejection #20097

Closed

4 tasks

devsnek reviewed Mar 11, 2019

View reviewed changes

Comment thread lib/internal/process/promises.js Outdated

ljharb approved these changes Mar 11, 2019

View reviewed changes

Comment thread lib/internal/process/promises.js Outdated

antsmartian reviewed Mar 12, 2019

View reviewed changes

Comment thread src/node_options.cc Outdated

benjamingr approved these changes Mar 12, 2019

View reviewed changes

addaleax reviewed Mar 12, 2019

View reviewed changes

Comment thread doc/api/cli.md Outdated

Comment thread doc/api/cli.md Outdated

Comment thread src/node_options.h Outdated

Comment thread src/node_options.cc Outdated

BridgeAR force-pushed the unhandled-take-xyz branch from 0ed8131 to c4dffa1 Compare March 12, 2019 13:08

mcollina approved these changes Mar 12, 2019

View reviewed changes

BridgeAR force-pushed the unhandled-take-xyz branch from ac91b7e to 11022c2 Compare March 12, 2019 14:34

mmarchini approved these changes Mar 12, 2019

View reviewed changes

mhdawson approved these changes Mar 12, 2019

View reviewed changes

BridgeAR force-pushed the unhandled-take-xyz branch 2 times, most recently from cb8b50b to 18c420d Compare March 13, 2019 13:42

benjamingr approved these changes Mar 13, 2019

View reviewed changes

vsemozhetbyt reviewed Mar 14, 2019

View reviewed changes

Comment thread doc/api/cli.md Outdated

Comment thread doc/api/process.md Outdated

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Mar 15, 2019

ljharb approved these changes Mar 15, 2019

View reviewed changes

BridgeAR mentioned this pull request May 7, 2019

v10.16.0 proposal #27514

Merged

BridgeAR added backport-requested-v10.x labels Aug 6, 2019

BridgeAR mentioned this pull request Aug 7, 2019

[v10.x] backport #26599 (process: add --unhandled-rejections flag) #29036

Closed

4 tasks

BethGriggs removed the lts-watch-v8.x label Aug 20, 2019

BethGriggs added land-on-v10.x and removed backport-requested-v10.x labels Oct 18, 2019

BethGriggs mentioned this pull request Oct 18, 2019

v10.17.0 proposal #29875

Merged

BridgeAR deleted the unhandled-take-xyz branch January 20, 2020 12:02

himself65 mentioned this pull request Apr 13, 2020

misleading command line help docs for "unhandled-rejections=mode" in "node --help server" #32796

Closed

dfabulich mentioned this pull request Apr 23, 2020

process: Don't warn/throw on unhandled rejections when hook is set #33017

Closed

4 tasks

process: add --unhandled-rejections flag by BridgeAR · Pull Request #26599 · nodejs/node · GitHub

Uh oh!

Uh oh!

process: add --unhandled-rejections flag - #26599

process: add --unhandled-rejections flag#26599BridgeAR wants to merge 2 commits intonodejs:masternodejs/node:masterfrom BridgeAR:unhandled-take-xyzBridgeAR/node:unhandled-take-xyzCopy head branch name to clipboard

Conversation

BridgeAR commented Mar 11, 2019

Checklist

Uh oh!

nodejs-github-bot commented Mar 11, 2019

Uh oh!

mmarchini commented Mar 11, 2019

Uh oh!

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 12, 2019

Uh oh!

mmarchini commented Mar 12, 2019

Uh oh!

mmarchini left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 13, 2019

Uh oh!

benjamingr commented Mar 13, 2019

Uh oh!

BridgeAR commented Mar 13, 2019

Uh oh!

BridgeAR commented Mar 13, 2019

Uh oh!

benjamingr left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Mar 13, 2019

Uh oh!

Uh oh!

Uh oh!

BridgeAR commented Mar 15, 2019

Uh oh!

ljharb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

20 participants

process: add --unhandled-rejections flag#26599
BridgeAR wants to merge 2 commits into
nodejs:masterfrom
BridgeAR:unhandled-take-xyz

mmarchini left a comment •
edited
Loading