process: add multipleResolves event by BridgeAR · Pull Request #22218 · nodejs/node

BridgeAR · 2018-08-09T20:16:11Z

This adds the ~~resolveSwallowed~~ multipleResolves event to track promises that resolve
more than once or that reject after resolving.

It is important to expose this to the user to make sure the
application runs as expected. Without such warnings it would be very
hard to debug these situations.

A single warning is triggered when such an error is triggered for the
first time during runtime without any active listeners.

This is an alternative to #21857.

Fixes: nodejs/promises-debugging#8

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 · 2018-08-09T20:16:13Z

@BridgeAR build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/482/pipeline

addaleax

/cc @devsnek @benjamingr

addaleax · 2018-08-09T20:26:43Z

doc/api/process.md

This sentence is incomplete, I think?

addaleax · 2018-08-09T20:27:47Z

src/bootstrapper.cc

Keeping return; allows V8 to add new events without breaking Node

The reason I changed it was to actually detect the new events up front. I'll change it back to return tough.

jasnell · 2018-08-09T20:29:59Z

lib/internal/process/promises.js

nit: should name this something like resolveAfterResolveWarned or something more specific just in case we need to add multiple such flags at some point

devsnek

please stop adding annoying promise warnings...

also i wouldn't consider this an alternative to my pr

jasnell · 2018-08-09T20:36:53Z

doc/api/process.md

non-blocking nit: not a fan of the name here. Not sure if I have a better suggestion tho. Perhaps just splitting these into two separate events? process.on('resolveAfterResolve') and process.on('rejectAfterResolve') or something similar?

These mistakes are all very similar and that is the reason I implemented it as a single event. I also just requested to add one more case that could also just land in this event.

I am absolutely open to a new name tough!

What about ambiguousResolve, resolveOmission or variousResolves?

multipleResolves (or redundantResolves) would be better than variousResolves, imo.

redundantResolves is a good choice, I think

I personally like multipleResolves. Redundant sounds like that case could just be removed but it is likely that there is an actual bug in the code.

resolveAfterResolve and rejectAfterResolve are pretty much as explainatory as you can get

only happens once now seems fine i guess...

jasnell · 2018-08-09T20:42:38Z

@devsnek ... the entire point behind process warnings is to be annoying. They are a not so subtle nudge away from less than ideal behavior. Avoiding the warnings here is a simple matter of either (a) fixing the code in question or (b) explicitly handling the events. In this particular case, being a bit noisy about bad code practices would definitely seem to make a lot of sense.

devsnek · 2018-08-09T20:43:44Z

lib/internal/process/promises.js

could just do nothing here

devsnek · 2018-08-09T20:46:29Z

@jasnell its not really node's job to tell me if my code is good or bad though. seeing this once is enough imo, as it reminds you and then won't get in the way of any other possibly more critical output.

addaleax · 2018-08-09T20:53:47Z

I have to agree with @devsnek, if users write spec-compliant JS code that worked up until now, then Node.js should not warn about that. Using the fact that Promises resolve only once has become a common pattern, and it’s simply not our job to tell users how to write their code.

mcollina

LGTM

addaleax

The warning should be opt-in.

Trott

I'd prefer to be extremely cautious about adding events like this, but I'm not blocking on it.

Trott · 2018-08-09T20:50:03Z

lib/internal/process/promises.js

It is recommended to listen -> Listen

Trott · 2018-08-09T20:54:16Z

lib/internal/process/promises.js

listen to the `process.on('resolveSwallowed')` event -> use `process.on('resolveSwalled', ...)` or something like that

jasnell · 2018-08-09T21:04:42Z

I'm generally fine with this or @devsnek's approach with the promise hook API, and I'm happy to sign off on both, but we should attempt to either reconcile the two or decide on a single approach because it likely does not make sense to land both.

jasnell · 2018-08-09T21:05:52Z

The warning should be opt-in.

If it's not emitted by default, we simply should not have it at all, even as opt in.

And, to be clear, I was not arguing in favor of this specific warning, but commenting generally on process warnings as a whole... specifically, they are intentionally annoying. I'm not convinced that we should have them in this specific case but that's a different question.

devsnek · 2018-08-09T21:10:01Z

lib/internal/process/promises.js

can we use the same names as the enum? also the last two are out of order

enum PromiseRejectEvent { kPromiseRejectWithNoHandler = 0, kPromiseHandlerAddedAfterReject = 1, kPromiseRejectAfterResolved = 2, kPromiseResolveAfterResolved = 3, };

benjamingr · 2018-08-09T23:12:01Z

If it's not emitted by default, we simply should not have it at all, even as opt in.

We have lots such useful features - like --trace-sync-io which are not on by default but are helpful when a user needs help troubleshooting. I believe this is one such case.

benjamingr · 2018-08-09T23:13:39Z

lib/internal/process/promises.js

Should be on with a flag or config by default I believe.

jasnell · 2018-08-09T23:16:28Z

Yes, but those are not easily hooked into by user code. If someone needs to troubleshoot this, then they can just as easily attach a handler to the events to detect as they can use a command line switch. I dunno, it's not something that I feel strongly enough about to block one way or the other on.

lib/internal/process/promises.js

mcollina

LGTM

BridgeAR · 2018-09-25T08:05:56Z

Thanks everyone.

Landed in 5605cec 🎉

This adds the `multipleResolves` event to track promises that resolve more than once or that reject after resolving. It is important to expose this to the user to make sure the application runs as expected. Without such warnings it would be very hard to debug these situations. PR-URL: nodejs#22218 Fixes: nodejs/promises-debugging#8 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

This adds the `multipleResolves` event to track promises that resolve more than once or that reject after resolving. It is important to expose this to the user to make sure the application runs as expected. Without such warnings it would be very hard to debug these situations. PR-URL: #22218 Fixes: nodejs/promises-debugging#8 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Anatoli Papirovski <[email protected]>

benjamingr · 2018-09-25T19:16:53Z

Thanks for your patience and thanks to both you and Gus for picking this up and working on this :)

Notable changes: * Build * FreeBSD 10 is no longer supported. [#22617](#22617) * `child_process` * The default value of the `windowsHide` option has been changed to `true`. [#21316](#21316) * `console` * `console.countReset()` will emit a warning if the timer being reset does not exist. [#21649](#21649) * `console.time()` will no longer reset a timer if it already exists. [#20442](#20442) * `crypto` * PEM-level encryption is now supported. [#23151](#23151) * An API for key pair generation has been added. [#22660](#22660) * Dependencies * V8 has been updated to 7.0. [#22754](#22754) * `fs` * The `fs.read()` method now requires a callback. [#22146](#22146) * The previously deprecated `fs.SyncWriteStream` utility has been removed.[#20735](#20735) * `http` * The `http`, `https`, and `tls` modules now use the WHATWG URL parser by default. [#20270](#20270) * `http2` * An event will be emitted when a `PING` frame is received. [#23009](#23009) * Support for the `ORIGIN` frame has been added. [#22956](#22956) * General * Use of `process.binding()` has been deprecated. Userland code using `process.binding()` should re-evaluate that use and begin migrating. * An experimental implementation of `queueMicrotask()` has been added. [#22951](#22951) * Internal * Windows performance-counter support has been removed. [#22485](#22485) * The `--expose-http2` command-line option has been removed. [#20887](#20887) * Promises * A new `multipleResolves` event will be emitted when a Promise is resolved (or rejected) more than once. [#22218](#22218) * Timers * Interval timers will be rescheduled even if previous interval threw an error. [#20002](#20002) * `util` * The WHATWG `TextEncoder` and `TextDecoder` are now globals. [#22281](#22281) * `util.inspect()` output size is limited to 128 MB by default. [#22756](#22756) * A runtime warning will be emitted when `NODE_DEBUG` is set for either `http` or `http2`. [#21914](#21914)

Notable changes: * assert * The diff output is now a tiny bit improved by sorting object properties when inspecting the values that are compared with each other. #22788 * cli * The options parser now normalizes `_` to `-` in all multi-word command-line flags, e.g. `--no_warnings` has the same effect as `--no-warnings`. #23020 * Added bash completion for the `node` binary. To generate a bash completion script, run `node --completion-bash`. The output can be saved to a file which can be sourced to enable completion. #20713 * crypto * Added support for PEM-level encryption. #23151 * Added an API asymmetric key pair generation. The new methods `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be used to generate public and private key pairs. The API supports RSA, DSA and EC and a variety of key encodings (both PEM and DER). #22660 * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to true, non-existing parent folders will be automatically created. #21875 * http2 * Added a `'ping'` event to `Http2Session` that is emitted whenever a non-ack `PING` is received. #23009 * Added support for the `ORIGIN` frame. #22956 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom require function that will resolve modules relative to the filename path. #19360 * process * Added a `'multipleResolves'` process event that is emitted whenever a `Promise` is attempted to be resolved multiple times, e.g. if the `resolve` and `reject` functions are both called in a `Promise` executor. #22218 * **url** * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between file: URLs and absolute paths. #22506 * **util** * Added the `sorted` option to `util.inspect()`. If set to `true`, all properties of an object and Set and Map entries will be sorted in the returned string. If set to a function, it is used as a compare function. #22788 * The `util.instpect.custom` symbol is now defined in the global symbol registry as `Symbol.for('nodejs.util.inspect.custom')`. #20857 * **Windows** * The Windows msi installer now provides an option to automatically install the tools required to build native modules. #22645 * **Added new collaborators**: * digitalinfinity - Hitesh Kanwathirtha PR-URL: #23313

Notable changes: * assert * The diff output is now a tiny bit improved by sorting object properties when inspecting the values that are compared with each other. #22788 * cli * The options parser now normalizes `_` to `-` in all multi-word command-line flags, e.g. `--no_warnings` has the same effect as `--no-warnings`. #23020 * Added bash completion for the `node` binary. To generate a bash completion script, run `node --completion-bash`. The output can be saved to a file which can be sourced to enable completion. #20713 * crypto * Added support for PEM-level encryption. #23151 * Added an API asymmetric key pair generation. The new methods `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be used to generate public and private key pairs. The API supports RSA, DSA and EC and a variety of key encodings (both PEM and DER). #22660 * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to true, non-existing parent folders will be automatically created. #21875 * http2 * Added a `'ping'` event to `Http2Session` that is emitted whenever a non-ack `PING` is received. #23009 * Added support for the `ORIGIN` frame. #22956 * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect protocol support to allow use of WebSockets over HTTP/2. #23284 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom require function that will resolve modules relative to the filename path. #19360 * process * Added a `'multipleResolves'` process event that is emitted whenever a `Promise` is attempted to be resolved multiple times, e.g. if the `resolve` and `reject` functions are both called in a `Promise` executor. #22218 * url * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between file: URLs and absolute paths. #22506 * util * Added the `sorted` option to `util.inspect()`. If set to `true`, all properties of an object and Set and Map entries will be sorted in the returned string. If set to a function, it is used as a compare function. #22788 * The `util.instpect.custom` symbol is now defined in the global symbol registry as `Symbol.for('nodejs.util.inspect.custom')`. #20857 * Added support for `BigInt` numbers in `util.format()`. #22097 * V8 API * A number of V8 C++ APIs have been marked as deprecated since they have been removed in the upstream repository. Replacement APIs are added where necessary. #23159 * Windows * The Windows msi installer now provides an option to automatically install the tools required to build native modules. #22645 * Workers * Debugging support for Workers using the DevTools protocol has been implemented. #21364 * The public `inspector` module is now enabled in Workers. #22769 * Added new collaborators: * digitalinfinity - Hitesh Kanwathirtha PR-URL: #23313

BridgeAR added the semver-minor PRs that contain new features and should be released in the next minor version. label Aug 9, 2018

BridgeAR requested review from benjamingr, bmeurer, devsnek, jasnell and mcollina August 9, 2018 20:16

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 Aug 9, 2018

addaleax reviewed Aug 9, 2018

View reviewed changes

jasnell reviewed Aug 9, 2018

View reviewed changes

devsnek previously requested changes Aug 9, 2018

View reviewed changes

jasnell reviewed Aug 9, 2018

View reviewed changes

BridgeAR requested a review from a team August 9, 2018 20:37

devsnek reviewed Aug 9, 2018

View reviewed changes

lib/internal/process/promises.js Outdated

Copy link

Member

devsnek Aug 9, 2018

Choose a reason for hiding this comment

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

could just do nothing here

mcollina approved these changes Aug 9, 2018

View reviewed changes

addaleax previously requested changes Aug 9, 2018

View reviewed changes

addaleax added semver-major PRs that contain breaking changes and should be released in the next major version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 9, 2018

Trott reviewed Aug 9, 2018

View reviewed changes

devsnek reviewed Aug 9, 2018

View reviewed changes

benjamingr reviewed Aug 9, 2018

View reviewed changes

lib/internal/process/promises.js Outdated

Copy link

Member

benjamingr Aug 9, 2018

Choose a reason for hiding this comment

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

Should be on with a flag or config by default I believe.

devsnek reviewed Sep 24, 2018

View reviewed changes

lib/internal/process/promises.js Show resolved Hide resolved

mcollina approved these changes Sep 25, 2018

View reviewed changes

BridgeAR closed this Sep 25, 2018

jasnell mentioned this pull request Oct 2, 2018

11.0.0 proposal #23230

Closed

4 tasks

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

BridgeAR mentioned this pull request Oct 9, 2018

Summit Topic: Promises openjs-foundation/summit#119

Closed

BridgeAR deleted the swallowed-promise-events branch January 20, 2020 11:38

zamnuts mentioned this pull request Mar 3, 2020

Node 8 deprecation party 🎉 googleapis/google-cloud-node#2972

Closed

Uh oh!

Conversation

BridgeAR commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nodejs-github-bot commented Aug 9, 2018

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsnek left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsnek Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devsnek commented Aug 9, 2018

Uh oh!

addaleax commented Aug 9, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Trott left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 9, 2018

Uh oh!

jasnell commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

benjamingr commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 9, 2018

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Sep 25, 2018

BridgeAR commented Aug 9, 2018 •

edited

Loading

devsnek left a comment •

edited

Loading

jasnell Aug 9, 2018 •

edited

Loading

devsnek Aug 9, 2018 •

edited

Loading

jasnell commented Aug 9, 2018 •

edited

Loading