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: refactor unhandled rejection handling #28228

Closed
wants to merge 1 commit into from

Conversation

joyeecheung
Copy link
Member

  • Use constants instead of a dictionary and add comments
    about the behavior of each mode.
  • Use switch cases to handle the unhandled rejection modes.
  • Rename the run time value of the CLI option from state
    to unhandledRejectionsMode.
  • Return in the call site of emitWarning when
    --unhandled-rejections=none instead of inside
    the function.
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

- Use constants instead of a dictionary and add comments
  about the behavior of each mode.
- Use switch cases to handle the unhandled rejection modes.
- Rename the run time value of the CLI option from `state`
  to `unhandledRejectionsMode`.
- Return in the call site of `emitWarning` when
  `--unhandled-rejections=none` instead of inside
  the function.
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jun 14, 2019

Sadly, an error occurred when I tried to trigger a build. :(
CI: https://ci.nodejs.org/job/node-test-pull-request/23886/

@nodejs-github-bot nodejs-github-bot added the process Issues and PRs related to the process subsystem. label Jun 14, 2019
// --unhandled-rejection=strict:
// Emit 'uncaughtException'. If it's not handled, print the error to stderr
// and exit the process.
// Otherwise, emit 'unhandledRejection'. If 'unhandledRejection' is not
Copy link
Member Author

@joyeecheung joyeecheung Jun 14, 2019

Choose a reason for hiding this comment

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

TBH this behavior (currently on master) looks weird to me. It contradicts with the description in test/parallel/test-promise-unhandled-error.js:

// Verify that unhandled rejections always trigger uncaught exceptions instead
// of triggering unhandled rejections.

Even though that test specifically tests:

process.on('unhandledRejection', common.mustCall(2));

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

The test description is faulty. The original implementation changed multiple times and it seems like the description was not updated appropriately.

The original intention was to replace the unhandled rejections with the exception. Later on it was decided that the current unhandled rejections hook behavior should not be influenced by the flag at all.

emitWarning(uid, reason);
switch (unhandledRejectionsMode) {
case kThrowUnhandledRejections: {
fatalException(reason);
Copy link
Member Author

Choose a reason for hiding this comment

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

Also funny enough there really isn't anything fatal about this function, or triggerFatalException, as they give the user a chance to pretend nothing has happened by listening to uncaughtException.

Copy link
Member

Choose a reason for hiding this comment

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

This is a legacy name. It does however reflect it's actual purpose. It is just unfortunate that we have an escape hatch for it with process.on('uncaughtException', fn).

// --unhandled-rejection=strict:
// Emit 'uncaughtException'. If it's not handled, print the error to stderr
// and exit the process.
// Otherwise, emit 'unhandledRejection'. If 'unhandledRejection' is not
Copy link
Member

Choose a reason for hiding this comment

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

The test description is faulty. The original implementation changed multiple times and it seems like the description was not updated appropriately.

The original intention was to replace the unhandled rejections with the exception. Later on it was decided that the current unhandled rejections hook behavior should not be influenced by the flag at all.

@joyeecheung
Copy link
Member Author

Landed in 370873c

joyeecheung added a commit that referenced this pull request Jun 17, 2019
- Use constants instead of a dictionary and add comments
  about the behavior of each mode.
- Use switch cases to handle the unhandled rejection modes.
- Rename the run time value of the CLI option from `state`
  to `unhandledRejectionsMode`.
- Return in the call site of `emitWarning` when
  `--unhandled-rejections=none` instead of inside
  the function.

PR-URL: #28228
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
BridgeAR pushed a commit that referenced this pull request Jun 17, 2019
- Use constants instead of a dictionary and add comments
  about the behavior of each mode.
- Use switch cases to handle the unhandled rejection modes.
- Rename the run time value of the CLI option from `state`
  to `unhandledRejectionsMode`.
- Return in the call site of `emitWarning` when
  `--unhandled-rejections=none` instead of inside
  the function.

PR-URL: #28228
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Jun 17, 2019
targos pushed a commit that referenced this pull request Jun 18, 2019
- Use constants instead of a dictionary and add comments
  about the behavior of each mode.
- Use switch cases to handle the unhandled rejection modes.
- Rename the run time value of the CLI option from `state`
  to `unhandledRejectionsMode`.
- Return in the call site of `emitWarning` when
  `--unhandled-rejections=none` instead of inside
  the function.

PR-URL: #28228
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants