process: Don't warn/throw on unhandled rejections when hook is set - #33017
process: Don't warn/throw on unhandled rejections when hook is set#33017dfabulich wants to merge 1 commit into
Conversation
Sorry, something went wrong.
--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
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
|
@BridgeAR Can you point me to where this was intensively discussed? I only see this comment on your PR.
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.
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".) |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
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! |
Sorry, something went wrong.
--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