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: change default --unhandled-rejections=strict #32986

Closed
wants to merge 1 commit into from

Conversation

dfabulich
Copy link
Contributor

This is a semver-major change that resolves DEP0018.

This commit also adds a new --unhandled-rejections=warn-unhandled
option, which mimics the previous default behavior. When using
warn-unhandled, a warning is emitted only if the unhandledRejection
hook fails to handle the event. The behavior of warn, unchanged in
this commit, is to omit a warning no matter whether the
unhandledRejection hook is set.

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

@nodejs-github-bot 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 Apr 22, 2020
@mscdex mscdex added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 22, 2020
@dfabulich
Copy link
Contributor Author

The PR build failed in the "Build from tarball" step, but I see no logs or error messages there, and it passed the "build-windows" "test-linux" and "test-macOS" steps.

Is there a way for me to reproduce this error? Or perhaps the failure is ephemeral? Could I re-run the build somehow?

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We still don't have consensus in #20392

@mmarchini
Copy link
Contributor

If we decide to land this, we definitely want it on a odd version, so we can revert it if needed before an LTS release, so the timing right now is good.

The PR build failed in the "Build from tarball" step, but I see no logs or error messages there, and it passed the "build-windows" "test-linux" and "test-macOS" steps.

It looks like something failed on the action, not sure why. Don't worry though, the other tests passed, and either way GitHub Actions is not our main CI, I'll start a Jenkins job for you.

This will need a CITGM run as well.

We still don't have consensus in #20392

@devsnek are you objecting to the PR direction or to landing this before further discussion? I remember you suggested going in the other direction in #25747.

@dfabulich just want to give you a heads up that folks are very passionate about this topic, and it'll probably take some time for this PR to land or be rejected (if consensus is reached at all).

@devsnek
Copy link
Member

devsnek commented Apr 22, 2020

@mmarchini I'm against it, and I also don't think we came to a consensus in the linked issue.

@addaleax
Copy link
Member

A smaller first step might be to not change behaviour to throwing an exception, but adding process.exitCode = 1; in the case of an unhandled rejection. That fulfills the wording of DEP0018 and is probably more likely to reach consensus?

This is a semver-major change that resolves DEP0018.

This PR defines a new mode for the --unhandled-rejections flag, called
"default". The "default" mode first emits unhandledRejection. If this
hook is not set, the "default" mode will raise the unhandled rejection
as an uncaught exception.

This mirrors the behavior of the current (unnamed) default mode for
--unhandled-rejections, which behaves nearly like "warn" mode. The
current default behavior is to emit unhandledRejection; if this hook is
not set, the unnamed default mode logs a warning and a deprecation
notice. (In comparison, "warn" mode always logs a warning, regardless of
whether the hook is set.)

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: nodejs#20392
Refs: https://nodejs.org/dist/latest/docs/api/deprecations.html#deprecations_dep0018_unhandled_promise_rejections
@dfabulich dfabulich changed the title process: change default --unhandled-rejections=strict process: Throw exception on --unhandled-rejections=default Apr 23, 2020
@dfabulich dfabulich changed the title process: Throw exception on --unhandled-rejections=default process: change default --unhandled-rejections=strict Apr 23, 2020
@dfabulich dfabulich closed this Apr 23, 2020
@mmarchini
Copy link
Contributor

TSC will be voting on the intended default behavior for unhandled rejections on v15, where we also intend to remove the deprecation warning. We want to listen to Node.js users experiences with unhandled promises and what you think should be the default before voting, so we prepared a survey: https://www.surveymonkey.com/r/FTJM7YD

We also wrote an accompanying blog post for extra context: https://medium.com/@nodejs/node-js-promise-reject-use-case-survey-98e3328340c9

The survey will run for at least two weeks, at which point we'll evaluate if the number of replies is enough for us to move forward, otherwise we might extend it for a week or two. Please fill out the survey as it will help us decide the future of unhandled promise rejections on Node.js!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. process Issues and PRs related to the process subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terminate process on unhandled promise rejection
6 participants