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

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
@jasnell
Member

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.

@ChALkeR

This comment has been minimized.

Show comment
Hide comment
@ChALkeR

ChALkeR Aug 22, 2016

Member

Blocked only because

This should not land until #8217 lands.

So that won't be missed.

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 in progress label Aug 29, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 29, 2016

Member

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

Member

jasnell commented Aug 29, 2016

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

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

PR-URL: #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 removed the blocked label Aug 30, 2016

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 30, 2016

Member

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

Member

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);

This comment has been minimized.

@Fishrock123

Fishrock123 Aug 30, 2016

Member

Get inside the following if?

@Fishrock123

Fishrock123 Aug 30, 2016

Member

Get inside the following if?

@Fishrock123

This comment has been minimized.

Show comment
Hide comment
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 30, 2016

Member

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

Member

jasnell commented Aug 30, 2016

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

@benjamingr

This comment has been minimized.

Show comment
Hide comment
@benjamingr

benjamingr Aug 31, 2016

Member

LGTM

Member

benjamingr commented Aug 31, 2016

LGTM

@chrisdickinson

This comment has been minimized.

Show comment
Hide comment
@chrisdickinson

chrisdickinson Aug 31, 2016

Contributor

LGTM

Contributor

chrisdickinson commented Aug 31, 2016

LGTM

jasnell added a commit that referenced this pull request Aug 31, 2016

promise: warn on unhandled rejections
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

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Aug 31, 2016

Member

Landed in v6.x in 180867d

Member

jasnell commented Aug 31, 2016

Landed in v6.x in 180867d

@jasnell jasnell closed this Aug 31, 2016

@Fishrock123 Fishrock123 referenced this pull request Sep 9, 2016

Merged

v6.6.0 proposal #8466

@ChALkeR ChALkeR removed the in progress label Sep 9, 2016

@ChALkeR ChALkeR referenced this pull request in nodejs/promises Sep 9, 2016

Closed

Default Unhandled Rejection Behavior #26

Fishrock123 added a commit that referenced this pull request Sep 14, 2016

2016-09-14, Version 6.6.0 (Current)
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

2016-09-14, Version 6.6.0 (Current)
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

2016-09-14, Version 6.6.0 (Current)
    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

2016-09-14, Version 6.6.0 (Current)
    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 mbland referenced this pull request in 18F/hubot-slack-github-issues Nov 23, 2016

Closed

[Question] Still necessary to use a fork? #51

mbland added a commit to mbland/hubot-slack-github-issues that referenced this pull request Dec 1, 2016

scripts: Always convert result to resolved Promise
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

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 mbland referenced this pull request in mbland/hubot-slack-github-issues Dec 1, 2016

Merged

Integration test fixes in light of new Promise warnings #7

mbland added a commit to mbland/unit-testing-node that referenced this pull request Apr 28, 2017

Fix Node v6.6.0+ UnhandledPromiseRejectionWarning
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