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=warn-with-error-code #33033

Closed
wants to merge 1 commit into from

Conversation

dfabulich
Copy link
Contributor

@dfabulich dfabulich commented Apr 23, 2020

This is a semver-major change that resolves DEP0018.

In addition to changing the default value for --unhandled-rejections,
this PR changes the exit code from 1 to 66. The warning message has
been updated to mention the new exit code, which I selected in order
to make this problem easier to search for on the web.

Users can prevent this behavior by setting a non-default value for the
--unhandled-rejections flag (throw, strict, warn, or none) or by setting
a hook for 'unhandledRejection'. This change has no effect on users who
have already set the flag or installed an 'unhandledRejection' hook; it
will only impact users who currently see a DEP0018 warning today.

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 23, 2020
@dfabulich
Copy link
Contributor Author

When I posted this PR, I hadn't yet seen @BridgeAR's comment #33021 (comment):

We just discussed these PRs in the TSC meeting and we try to move these forward fast. The issue about the deprecation notice was around for way to long that we did not act upon it so far.

The last agreement on our Collaborator Summit was to get a survey about this topic and I am going to write a blog post about it that I'll post in the promises debugging working group and the website working group so that it can be reviewed before being published.
As soon as that's done, we'll get a survey together to get a wider audience about the preferred future default value.

It is unlikely that any PR that tries to change the default before that is going to land before that point of time. I guess this PR might land as semver-minor in case it does not change the default anymore. The name could be changed to e.g., semi-strict in that case.

Well, the work is done, and I figure it's better to have the code out there while we wait for results.

@BridgeAR BridgeAR added blocked PRs that are blocked by other issues or PRs. semver-major PRs that contain breaking changes and should be released in the next major version. labels Apr 23, 2020
@devsnek
Copy link
Member

devsnek commented Apr 23, 2020

needs another 6

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:18
This is a semver-major change that resolves DEP0018.

In addition to changing the default value for --unhandled-rejections,
this PR changes the exit code from 1 to 66. The warning message has
been updated to mention the new exit code, which I selected in order
to make this problem easier to search for on the web.

Users can prevent this behavior by setting a non-default value for the
--unhandled-rejections flag (throw, strict, warn, or none) or by setting
a hook for 'unhandledRejection'. This change has no effect on users who
have already set the flag or installed an 'unhandledRejection' hook; it
will only impact users who currently see a DEP0018 warning today.
@dfabulich
Copy link
Contributor Author

For the sake of discussion, I've resolved conflicts on this PR now that PR #33475 has merged. The TSC will need to decided whether to merge this PR or PR #33021. (I would prefer to merge 33021.)

@dfabulich dfabulich changed the title process: unhandled rejections exit with code 66 by default process: Change default --unhandled-rejections=warn-with-error-code Jun 16, 2020
@nylen
Copy link

nylen commented Jun 23, 2020

issue a warning, and possibly set the exit code to non-zero when the process eventually happens to exit

That's what this PR does, right?

Why would anyone want this behavior, much less making it the default?

@dfabulich
Copy link
Contributor Author

Most of the discussion around this PR is on the thread for PR #33021, which is my preferred solution.

tl;dr: Everybody would like to get rid of DEP0018, but there's disagreement about how to do that. Some Node TSC members think the default should be to throw an exception (and thereby crash, by default), and others think the default should be to just log a warning and delete the DEP0018 warning, saying it was a mistake to deprecate unhandled rejections.

(One TC39 member in the thread argued that the default shouldn't even log a warning on unhandled rejections, though he accepted logging a warning as a compromise.)

The option adopted here, to set an exit code while logging the warning, was raised as a compromise solution between the two main groups. It doesn't crash the process, which may satisfy those who feel that rejected promises are not exceptional, but it does indicate a problem via the exit code.

TSC plans to survey node users about this in a few weeks, and then vote on a solution.

@nylen
Copy link

nylen commented Jun 24, 2020

but it does indicate a problem via the exit code

...at some point, when the process finally exits. This could be hours or even weeks later.

And that's if no other code sets process.exitCode to some other value in the meantime!

Frankly, this "compromise solution" is basically useless, and I will repeat what I said on the other PR here:

unhandled errors should abort the process. I think we (programmers in general) learned a long time ago that unhandled errors should abort, any other behavior is asking for trouble in complex systems.

Anyway thanks for making the PRs!

@mmarchini
Copy link
Contributor

Closing as decision was to move forward with throw

@mmarchini mmarchini closed this Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked PRs that are blocked by other issues or PRs. 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.

6 participants