Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

process: add --unhandled-rejections flag #26599

Open
wants to merge 10 commits into
base: master
from

Conversation

@BridgeAR
Copy link
Member

BridgeAR commented Mar 11, 2019

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 BridgeAR referenced this pull request Mar 11, 2019

Closed

src: exit on gc unhandled rejection #20097

4 of 4 tasks complete
@mmarchini

This comment has been minimized.

Copy link
Member

mmarchini commented Mar 11, 2019

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).

@ljharb

ljharb approved these changes Mar 11, 2019

Copy link
Contributor

ljharb left a comment

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.

Show resolved Hide resolved lib/internal/process/promises.js Outdated
Show resolved Hide resolved src/node_options.cc Outdated
@benjamingr
Copy link
Member

benjamingr left a comment

LGTM

Show resolved Hide resolved doc/api/cli.md Outdated
Show resolved Hide resolved doc/api/cli.md Outdated
Show resolved Hide resolved src/node_options.h
Show resolved Hide resolved src/node_options.cc

@BridgeAR BridgeAR force-pushed the BridgeAR:unhandled-take-xyz branch from 0ed8131 to c4dffa1 Mar 12, 2019

@mcollina
Copy link
Member

mcollina left a comment

LGTM

BridgeAR added some commits Mar 12, 2019

src: print error before aborting
In case of fatal errors, first print the error before aborting in
case the process should abort on uncaught exceptions.
process: add --unhandled-rejections flag
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.

@BridgeAR BridgeAR force-pushed the BridgeAR:unhandled-take-xyz branch from ac91b7e to 11022c2 Mar 12, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 12, 2019

I had to rebase due to conflicts, so PTAL.

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

@mmarchini

This comment has been minimized.

Copy link
Member

mmarchini commented Mar 12, 2019

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

  1. 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.
  2. 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 nodejs/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.
  3. The stack we get on llnode (by using v8 bt) is not very useful, but this was expected and should be addressed by 2.
@mmarchini
Copy link
Member

mmarchini left a comment

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

@mhdawson
Copy link
Member

mhdawson left a comment

LGTM once the default behaviour is clarified.

@BridgeAR BridgeAR force-pushed the BridgeAR:unhandled-take-xyz branch from 8177e7c to cb8b50b Mar 13, 2019

@BridgeAR BridgeAR force-pushed the BridgeAR:unhandled-take-xyz branch from cb8b50b to 18c420d Mar 13, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 13, 2019

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/

@benjamingr

This comment has been minimized.

Copy link
Member

benjamingr commented Mar 13, 2019

@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?

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 13, 2019

@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.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 13, 2019

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).

@benjamingr
Copy link
Member

benjamingr left a comment

Explicit "still lgtm"

@ljharb

ljharb approved these changes Mar 15, 2019

Copy link
Contributor

ljharb left a comment

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.

@mcollina
Copy link
Member

mcollina left a comment

LGTM

BridgeAR added some commits Mar 18, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@bcoe

This comment has been minimized.

Copy link
Member

bcoe commented Mar 18, 2019

@BridgeAR I really like @ljharb's suggestion of exposing this as an environment variable. One use-case brought up to me was the ability to abort more aggressively on uncaught exceptions in tests, it would be delightful if I could simply do:

{
  "script": {
    "test": "NODE_UNHANDLED_REJECTIONS=strict npm t"
  }
}
@mmarchini

This comment has been minimized.

Copy link
Member

mmarchini commented Mar 18, 2019

Can't we just use NODE_OPTIONS to set the rejection mode from an environment variable?

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@bcoe that should be possible by using NODE_OPTIONS=--unhandled-rejections=strict npm t.

// cc @ljharb

Update: Seems like @mmarchini was faster.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@bcoe

This comment has been minimized.

Copy link
Member

bcoe commented Mar 18, 2019

@mmarchini @BridgeAR works for me, I'm excited about this work.

@bcoe

This comment has been minimized.

Copy link
Member

bcoe commented Mar 18, 2019

@BridgeAR any reason we couldn't back port this to v10? (would be valuable for my team).

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@bcoe it should be possible to backport this to all active release lines.

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Mar 18, 2019

@bcoe that should be possible by using NODE_OPTIONS=--unhandled-rejections=strict npm t.

// cc @ljharb

Update: Seems like @mmarchini was faster.

Could you also update the list of allowed options in NODE_OPTIONS? Optionally test/parallel/test-cli-node-options.js too.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@richardlau will do. @Trott @vsemozhetbyt should we maybe change the list to a blacklist instead of a whitelist? I expect almost all flags to be allowed while only a few to be disallowed?

@richardlau

This comment has been minimized.

Copy link
Member

richardlau commented Mar 18, 2019

@richardlau will do. @Trott @vsemozhetbyt should we maybe change the list to a blacklist instead of a whitelist? I expect almost all flags to be allowed while only a few to be disallowed?

I can't remember the exact details, but IIRC the PR to add NODE_OPTIONS originally was proposed with a blacklist but was changed to a whitelist by the time it landed.

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Mar 18, 2019

All things being equal, I think a list of things that do work is better for the reader than a list of things that don't work. But if the consensus is that this principle does not hold in this case, I'm fine with changing the doc.

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 18, 2019

@ChALkeR
Copy link
Member

ChALkeR left a comment

Overall, LGTM, also, this is great. 👍

But I have some questions regarding state handling in js code -- see code comments.

Show resolved Hide resolved lib/internal/process/promises.js
Show resolved Hide resolved lib/internal/process/promises.js Outdated
@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 19, 2019

Using this flag allows to change what should happen when an unhandled rejection
occurs. One of three modes can be chosen:

* `strict`: Raise the unhandled rejection as an uncaught exception.

This comment has been minimized.

@Fishrock123

Fishrock123 Mar 19, 2019

Member

Can this be error?

This comment has been minimized.

@BridgeAR

BridgeAR Mar 19, 2019

Author Member

I originally planned on using error and silence instead of strict and none but others preferred these and I have no strong opinion on the name in this case.

Issues were fixed

@ChALkeR ChALkeR self-requested a review Mar 19, 2019

@BridgeAR

This comment has been minimized.

Copy link
Member Author

BridgeAR commented Mar 19, 2019

@mmarchini
Copy link
Member

mmarchini left a comment

Still LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.