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: Don't warn/throw on unhandled rejections when hook is set #33017

Closed
wants to merge 1 commit into from

Conversation

dfabulich
Copy link
Contributor

--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: #26599

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

--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
@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Apr 23, 2020
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

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

The flag as it was implemented intentionally did not check for the hook being present or not and the behavior was intensively discussed before it was agreed upon to behave exactly as it does right now.

I am strongly against checking for the hook's presence and therefore against this change. Any module might set such a hook (and many do) so that it would impact the user.

@himself65 himself65 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 23, 2020
@dfabulich
Copy link
Contributor Author

@BridgeAR Can you point me to where this was intensively discussed? I only see this comment on your 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).

No one replied to your "I would rather" remark, except to say "still LGTM." It's a fairly subtle point that hasn't really been discussed anywhere I can find.

Any module might set such a hook (and many do) so that it would impact the user.

That is already true today. That's how --unhandled-rejections behaves by default, and it has behaved this way since Node 10. If any module sets the hook, then the warning disappears. We then do just whatever the hook says to do: crash, log, nothing, or whatever.

This PR only changes the behavior of --unhandled-rejections=strict mode when a module would attempt to set a "no-op hook" like process.on('unhandledRejection', () => {});. But users who need a guarantee of strictness can set their own strict hook like process.on('unhandledRejection', err => {throw err}), overriding their own modules.

If you think it would help, we could compromise by having a new mode for --unhandled-rejections, e.g. --unhandled-rejections=always-strict that would disregard the unhandledRejection hook, allowing --unhandled-rejections=strict to throw an exception only if the hook is missing.

(I'd have to think harder about the name always-strict … maybe we'd leave strict alone and come up with another semi-strict name like default-strict or hard or throwing or something.)

The reason I care about this is that I hope to change the default --unhandled-rejections mode to finally fix #20392. I think defaulting to strict mode (always-strict mode) today would be too strict, because there would be no way for userland code to override that default, and this is a big part of why we see such controversy around changing it.

So yet another option is to skip this PR entirely and just do a PR that changes the default mode to semi-strict. Userland code could override the semi-strict behavior with a hook. If we did that, I'd almost certainly want to give the default mode an actual named value and add it to the documentation.

(I think a lot of people wrongly think that the default mode is warn, but warn always warns, regardless of the hook. Today's default is "semi-warn".)

@dfabulich
Copy link
Contributor Author

Instead of this PR, we merged PR #33475 which adds two new modes to --unhandled-rejections; in those modes, setting a hook will override the default behavior.

@dfabulich dfabulich closed this Jun 16, 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
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.

6 participants