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

promise: warning on unhandled rejection (v6.x) #8223

Closed
wants to merge 1 commit into from

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Aug 22, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

promises

Description of change

A semver-minor port of #8217 for v6.x (it does not include the deprecation). This will be kept up to date along with #8217 as nits are addressed. This should not land until #8217 lands.

@jasnell jasnell added semver-minor PRs that contain new features and should be released in the next minor version. process Issues and PRs related to the process subsystem. promises Issues and PRs related to ECMAScript promises. labels Aug 22, 2016
@ChALkeR ChALkeR added the blocked PRs that are blocked by other issues or PRs. label Aug 22, 2016
@ChALkeR
Copy link
Member

ChALkeR commented Aug 22, 2016

Blocked only because

This should not land until #8217 lands.

So that won't be missed.

@jasnell jasnell added the wip Issues and PRs that are still a work in progress. label Aug 29, 2016
@jasnell
Copy link
Member Author

jasnell commented Aug 29, 2016

I need to update this before it can land...

Log unhandled promise rejections with a guid and emit
a process warning. When rejection is eventually handled,
emit a secondary warning.

PR-URL: nodejs#8217
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@jasnell jasnell force-pushed the warn-on-unhandled-rejection branch from 296fc71 to 995d504 Compare August 30, 2016 17:48
@jasnell jasnell removed the blocked PRs that are blocked by other issues or PRs. label Aug 30, 2016
@jasnell
Copy link
Member Author

jasnell commented Aug 30, 2016

#8217 landed, this PR has been updated. @ChALkeR @Fishrock123 @benjamingr @chrisdickinson ... PTAL

if (hasBeenNotifiedProperty.get(promise) === false) {
hasBeenNotifiedProperty.set(promise, true);
const uid = promiseToGuidProperty.get(promise);
Copy link
Contributor

Choose a reason for hiding this comment

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

Get inside the following if?

@Fishrock123
Copy link
Contributor

@jasnell
Copy link
Member Author

jasnell commented Aug 30, 2016

CI is green. Will land either tomorrow or Thursday if there are no objections.

@benjamingr
Copy link
Member

LGTM

1 similar comment
@chrisdickinson
Copy link
Contributor

LGTM

jasnell pushed a commit that referenced this pull request Aug 31, 2016
Log unhandled promise rejections with a guid and emit
a process warning. When rejection is eventually handled,
emit a secondary warning.

PR-URL: #8223
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Chris Dickinson <christopher.s.dickinson@gmail.com>
@jasnell
Copy link
Member Author

jasnell commented Aug 31, 2016

Landed in v6.x in 180867d

@jasnell jasnell closed this Aug 31, 2016
@Fishrock123 Fishrock123 mentioned this pull request Sep 9, 2016
@ChALkeR ChALkeR removed the wip Issues and PRs that are still a work in progress. label Sep 9, 2016
Fishrock123 added a commit that referenced this pull request Sep 14, 2016
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) #8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) #8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) #8174

Refs: #8428
Refs: #8457
PR-URL: #8466
Fishrock123 added a commit to Fishrock123/node that referenced this pull request Sep 15, 2016
Notable changes:

* crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
nodejs#8304
* events: Made the "max event listeners" memory leak warning more
accessible. (Anna Henningsen) nodejs#8298
* promises: Unhandled rejections now emit a process warning after the
first tick. (Benjamin Gruenbaum)
nodejs#8223
* repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
nodejs#8241
* util: Some functionality has been added to `util.inspect()`:
- Returning `this` from a custom inspect function now works. (Anna
Henningsen) nodejs#8174
- Added support for Symbol-based custom inspection methods. (Anna
Henningsen) nodejs#8174

Refs: nodejs#8428
Refs: nodejs#8457
PR-URL: nodejs#8466
imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 15, 2016
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
imyller added a commit to imyller/meta-nodejs that referenced this pull request Sep 15, 2016
    Notable changes:

    * crypto: Added `crypto.timingSafeEqual()`. (not-an-aardvark)
    nodejs/node#8304
    * events: Made the "max event listeners" memory leak warning more
    accessible. (Anna Henningsen) nodejs/node#8298
    * promises: Unhandled rejections now emit a process warning after the
    first tick. (Benjamin Gruenbaum)
    nodejs/node#8223
    * repl: Added auto alignment for `.editor` mode. (Prince J Wesley)
    nodejs/node#8241
    * util: Some functionality has been added to `util.inspect()`:
    - Returning `this` from a custom inspect function now works. (Anna
    Henningsen) nodejs/node#8174
    - Added support for Symbol-based custom inspection methods. (Anna
    Henningsen) nodejs/node#8174

Signed-off-by: Ilkka Myller <ilkka.myller@nodefield.com>
mbland added a commit to mbland/hubot-slack-github-issues that referenced this pull request Dec 1, 2016
As discovered in 18F#51, the
`UnhandledPromiseRejectionWarning` and `PromiseRejectionHandledWarning`
warnings were apparently added in v6.6.0
(https://nodejs.org/en/blog/release/v6.6.0/); specifically it was added
by nodejs/node#8223. See also:

  nodejs/node#6439
  nodejs/node#8217
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_unhandledrejection
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_rejectionhandled
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_warning

Test failures from `test/integration-test.js` after upgrading to Node
v6.9.1 showed extra output such as:

```
  (node:39696) UnhandledPromiseRejectionWarning: Unhandled promise rejection (rejection id: 2): null
  (node:39696) PromiseRejectionHandledWarning: Promise rejection was handled asynchronously (rejection id: 2)
```

This was happening because `scripts/slack-github-issues.js` created a
Hubot listener that returned a `Promise` so that the integration test
could use `.should.be.rejectedWith` assertions. Rather than jump through
hoops to quash the warnings, this change now causes the listener's
`catch` handler to return the result of the rejected `Promise`,
converting it to a fulfilled `Promise` in the process.

Since Hubot never used the result anyway, the only effect it has in
production is to eliminate the warning messages. The tests now just
check that the `Promise` returned by the listener callback is fulfilled
with the expected error result, with practically no loss in clarity.
mbland added a commit to mbland/unit-testing-node that referenced this pull request Apr 28, 2017
Backported from mbland/hubot-slack-github-issues#7.

As discovered in 18F/hubot-slack-github-issues#51, the
`UnhandledPromiseRejectionWarning` and `PromiseRejectionHandledWarning`
warnings were apparently added in v6.6.0
(https://nodejs.org/en/blog/release/v6.6.0/); specifically it was added
by nodejs/node#8223. See also:

  nodejs/node#6439
  nodejs/node#8217
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_unhandledrejection
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_rejectionhandled
  https://nodejs.org/dist/latest-v6.x/docs/api/process.html#process_event_warning

A test failure from `solutions/06-integration/test/integration-test.js`
after upgrading to Node v6.9.1 showed output such as:

```
-  "(node:87412) UnhandledPromiseRejectionWarning: Unhandled
     promise rejection (rejection id: 14): Error: failed to create a
     GitHub issue in 18F/handbook: received 500 response from GitHub
     API: {\"message\":\"test failure\"}\n"
```

This was happening because `scripts/slack-github-issues.js`
ignored the return value from `Middleware.execute()`, whether it was
undefined or a `Promise`. For consistency's sake (and to provide a
clearer upgrade path to the current state of
mbland/slack-github-issues), `Middleware.execute()` always returns a
`Promise`, and `scripts/slack-github-issues.js` handles and ignores any
rejected Promises.

This fixed the `integration-test.js` error, but also required minor
updates to `solutions/{05-middleware,complete}/test/middleware-test.js`.
The next commit will update the tutorial narrative to account for this
change.
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. promises Issues and PRs related to ECMAScript promises. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants