util: expose promise events #21857

devsnek · 2018-07-17T18:44:25Z

Refs nodejs/promises-debugging#8
Closes nodejs/promises-debugging#8

exposes env promise hooks to userland

/cc @nodejs/promises-debugging @nodejs/async_hooks @BridgeAR @ljharb

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-07-17T18:44:27Z

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

addaleax · 2018-07-17T18:48:01Z

src/async_wrap.cc

+
+  Local<Function> fn = env->async_hooks_promise_event_function();
+
+  v8::HandleScope handle_scope(env->isolate());


This would technically need to be swapped with the line above (it’s only working because we’re doing an evil hack and look into V8’s object representations to load persistent references faster)

addaleax · 2018-07-17T18:49:45Z

src/node.h

@@ -612,9 +612,20 @@ NODE_EXTERN void AtExit(void (*cb)(void* arg), void* arg = 0);
 */
 NODE_EXTERN void AtExit(Environment* env, void (*cb)(void* arg), void* arg = 0);

-typedef void (*promise_hook_func) (v8::PromiseHookType type,
+NODE_EXTERN typedef enum {


I don’t think it really makes sense to add NODE_EXTERN to a type definition; also, the typedef is redundant, just using enum PromiseHookType { … }; should work the same way

this doesn't compile without the typedef

benjamingr

This is backwards incompatible and will break code for a worse off default.

This also goes against the ongoing work we've been doing at @nodejs/promises-debugging and the work.

I am strictly -1 on this change before the survey we've been working on with the foundation is sent out and we've gathered sufficient data.

benjamingr · 2018-07-17T19:31:07Z

This allows the default behaviour in node to follow the intention of js

This is also not true and was not the intention - browsers error on unhandled rejections too. One of the things we've been getting since adding the unhandledRejection hook is constant positive feedback for logging as well as negative one for not crashing.

BridgeAR · 2018-07-17T19:44:00Z

It would have been great to discuss things like that in the promises working group. Since this is completely backwards incompatible I am strongly -1. It is not an option to remove the regular way how people deal with this.

AFAIK it is also not possible to mirror the current implementation in userland even with the events exposed.

I believe it is best to keep these things in core so everyone takes advantage of it by default. Opt-out instead of opt-in.

BridgeAR

-1 due to the mentioned reasons.

benjamingr · 2018-07-17T19:55:10Z

By hte way: By the same logic we should deprecate uncaughtException, stop exiting or logging on errors and expose an async_hook for exiting.

devsnek · 2018-07-17T20:21:03Z

i can add the promiseResolve back as deprecated if you want? i assumed it wasn't needed because async_hooks is experimental still...

as to the argument about how promises are supposed to work: they are explicitly intended to be able to silently fail. what browser consoles do is completely irrelevant because that's a debug facility. (we could probably do the same thing with our inspector, which would be also fine)

devsnek · 2018-07-17T20:28:33Z

@BridgeAR

AFAIK it is also not possible to mirror the current implementation in userland even with the events exposed.

i tried to design this with that in mind (rejectWithNoHandler includes the rejection reason, etc) and using process.nextTick will make it identical (except for storing asyncId instead of promise)

ljharb · 2018-07-17T20:31:47Z

@benjamingr browsers provide a console warning on unhandled rejections, which they then retroactively erase if it becomes handled - node doesn't have that capability.

benjamingr · 2018-07-17T20:34:17Z

they are explicitly intended to be able to silently fail.

I do not believe this is the case nor does the spec make any such requirement. That said: Under all proposed implementations promises are able to silently fail - just like you can ignore synchronous errors:

process.on('uncaughtException', e => { /* console.log('https://i.imgur.com/c4jt321.png') */ });

You can ignore unhandled rejections:

process.on('uncaughtException', e => { /* console.log('https://i.imgur.com/c4jt321.png') */ });

Which will continue to work forever under all proposed changes. The question is about what the default behaviour should be.

We have a consensus at this point in Node.js that ignoring uncaught errors should not be the default behaviour.

what browser consoles do is completely irrelevant because that's a debug facility. this is all for debugging

Again, the same argument could be made for Node.js not exiting on uncaught errors. Since a significant part of our user-base use async/await it means that in practice they don't distinguish between unhandled rejections and uncaught errors in terms of how they handle errors anyway.

benjamingr · 2018-07-17T20:38:11Z

@ljharb

@benjamingr browsers provide a console warning on unhandled rejections, which they then retroactively erase if it becomes handled - node doesn't have that capability.

The browser error handling model is completely different though - Node.js crashes on uncaught errors (by default) whereas browsers "chug on". Both behaviours have been this way for well over 8 years.

I was just pointing out browsers consider unhandled rejections as an error and log based on it. Node.js provides the "erasing" by providing the "rejectionHandled" hook which I haven't actually seen ever used but it's possible and was added in the same PR.

devsnek · 2018-07-17T20:46:10Z

I do not believe this is the case nor does the spec make any such requirement.

the spec says An implementation of HostPromiseRejectionTracker must complete normally in all cases.. i would argue that killing the process is not a normal completion, but i don't really want to get into those semantics.

aside from that, unhandledRejection (with the emit on gc proposal) can't guarantee that it will be fired for the things it should be unless someone is using global.gc, which kind of undermines the debugging potential of it. By not making these (arguably confusing for that reason) apis, we can let people find the best behaviour for what they're trying to do, similar to how people often use tools like winston even though node has console.

these changes also mean that in the common case, nothing is slowed down by promise state tracking.

benjamingr · 2018-07-17T20:56:54Z

@devsnek I encourage you to read all the discussion above as well as the nodejs/promises repo discussions about this issue.

The gist is:

We can't really know when a promise rejection will never be handled (halting problem)
We can warn when a promise is handled asynchronously (and not within a microtask) since that has been very rare in practice. We've added this and got tremendous positive community feedback.
GC is the only tool we have for detecting most true positives but no false positives at all. Note this has false negatives like a unhandled rejection the app keeps a global reference for. Also note this cannot be implemented in user-land since it relies on core GC behaviour.
Since GC isn't perfect - when a promise isn't handled synchronously there is a high true positive chance - so it makes sense to still log these.
We let users choose the behaviour they want in all cases. This is only about the default behaviour.

In a gist the correct behaviour we settled on was:

If you can prove (GC) an exception is unhandled do the same thing we do for uncaught exceptions - exit the process.
If you think with a high probability that a rejection is unhandled - show that as a warning since that's what most users find beneficial (we have a recent Netflix survey that also shows this). This is easy to opt out of.

It's not perfect - but sadly I think it's the best we might be able to do. You are encouraged to read the background discussions and participate in the promises-debugging team which still has an open "call for members". We'd love for more people to get involved.

benjamingr · 2018-07-17T20:59:47Z

the spec says An implementation of HostPromiseRejectionTracker must complete normally in all cases.. i would argue that killing the process is not a normal completion, but i don't really want to get into those semantics.

That's not HostPromiseRejectionTracker (which just tracks any rejections) - we are strictly talking about code that happens after that when the promise gets GCd. At this point control has been yielded to the platform and only platform code is running. An implementation may do as it pleases. This is entirely Node.js's domain and not ECMAScript's.

devsnek · 2018-07-17T21:18:46Z

Also note this cannot be implemented in user-land since it relies on core GC behaviour.

the destroy hook lets us do that right?

in any case, wasn't the whole point here to catch all the bad things happening? if we don't catch them we aren't solving the problem (with regard to gc) and having that as a default is imo confusing and misleading (not to mention that having all these promise state trackers by default is annoying)

devsnek · 2018-07-17T23:42:21Z

@benjamingr @BridgeAR unhandledRejection event remains. This PR still provides a solution to exposing the events requested in nodejs/promises-debugging#8 (and other fun stuff)

i still strongly recommend against node defining what an unhandled rejection is in any case but that's a conversation for another issue/pr

jasnell · 2018-08-09T20:56:13Z

doc/api/util.md

+  * `enable` {Function} Enables the hook
+  * `disable` {Function} Disables the hook
+
+> Stability: 1 - Experimental


nit: This should likely be moved above the arguments list

devsnek · 2018-08-09T23:26:10Z

CI https://ci.nodejs.org/job/node-test-pull-request/16319/

mcollina · 2018-08-10T07:05:03Z

I've signed off both PR (this and #22218), but we should really reconcile them before landing either one. See @jasnell comment #22218 (comment).

Trott · 2018-08-13T23:16:24Z

@BridgeAR Same question as in the other PR: Can you explain what you'd like the TSC to decide or do with this at the meeting? (Or should we remove it from the agenda?)

EDIT: Are you asking the TSC to decide whether we should go with this approach or #22218? Is it unlikely that you and @devsnek or others will come to an agreement on that?

Trott · 2018-08-25T04:46:51Z

If I've understood the conversation above, there are currently 3 objecting Collaborators on this PR: @benjamingr, @BridgeAR, and @AndreasMadsen. If I've misunderstood and any of you don't object to this at this time, please comment! Thanks!

Trott · 2018-08-25T05:07:43Z

Adding blocked here and in #22218 pending decision about which approach to go with.

devsnek · 2018-08-29T12:24:41Z

@nodejs/tsc PTAL.

@benjamingr your review doesn't show up anymore but i assume it is outdated.

Like Gus said, my review is outdated.Both this and the other PR LGTM.

fhinkel · 2018-08-29T13:34:04Z

Thanks everybody for putting so much work into this! Would somebody be willing to summarize #22218 and #21857 so it's easier to understand the pros and cons of the two approaches?

addaleax · 2018-09-17T15:33:18Z

@devsnek This would at least need a rebase?

devsnek · 2018-09-17T15:44:48Z

that's more than I'm willing to do at this point.

devsnek added semver-major PRs that contain breaking changes and should be released in the next major version. async_wrap promises Issues and PRs related to ECMAScript promises. async_hooks Issues and PRs related to the async hooks subsystem. labels Jul 17, 2018

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 17, 2018

devsnek added wip Issues and PRs that are still a work in progress. and removed lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 17, 2018

addaleax reviewed Jul 17, 2018

View reviewed changes

AndreasMadsen self-requested a review July 17, 2018 19:25

benjamingr previously requested changes Jul 17, 2018

View reviewed changes

jasnell requested a review from BridgeAR July 17, 2018 19:49

BridgeAR previously requested changes Jul 17, 2018

View reviewed changes

devsnek force-pushed the feature/async_hooks-promise-event branch 2 times, most recently from f83beed to ff94f4c Compare July 17, 2018 22:39

devsnek removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Jul 17, 2018

devsnek removed the wip Issues and PRs that are still a work in progress. label Jul 17, 2018

jasnell reviewed Aug 9, 2018

View reviewed changes

jasnell approved these changes Aug 9, 2018

View reviewed changes

util: expose promise events

3dec7bb

devsnek force-pushed the feature/async_hooks-promise-event branch from 7ff1627 to 3dec7bb Compare August 9, 2018 23:25

joyeecheung mentioned this pull request Aug 10, 2018

Investigate flaky test-module-loading-globalpaths with --worker on #22228

Closed

BridgeAR added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 10, 2018

mhdawson mentioned this pull request Aug 13, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-08-15 nodejs/TSC#585

Closed

mhdawson mentioned this pull request Aug 20, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-08-22 nodejs/TSC#587

Closed

Trott added the blocked PRs that are blocked by other issues or PRs. label Aug 25, 2018

mhdawson mentioned this pull request Aug 27, 2018

Node.js Foundation Technical Steering Committee (TSC) Meeting 2018-08-29 nodejs/TSC#590

Closed

cjihrig approved these changes Aug 29, 2018

View reviewed changes

Trott removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Aug 29, 2018

Trott mentioned this pull request Aug 29, 2018

opt-out or opt-in for warnings etc. #22587

Closed

devsnek requested a review from a team September 5, 2018 01:40

devsnek added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 5, 2018

Trott removed the request for review from a team September 8, 2018 01:48

jasnell mentioned this pull request Sep 12, 2018

[Discuss] Resolving the unhandled rejections issue #22822

Closed

addaleax removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Sep 17, 2018

devsnek closed this Sep 17, 2018

devsnek deleted the feature/async_hooks-promise-event branch September 17, 2018 15:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

util: expose promise events #21857

util: expose promise events #21857

devsnek commented Jul 17, 2018 •

edited

Loading

nodejs-github-bot commented Jul 17, 2018

addaleax Jul 17, 2018

addaleax Jul 17, 2018

devsnek Jul 17, 2018

benjamingr left a comment

benjamingr commented Jul 17, 2018

BridgeAR commented Jul 17, 2018

BridgeAR left a comment

benjamingr commented Jul 17, 2018

devsnek commented Jul 17, 2018 •

edited

Loading

devsnek commented Jul 17, 2018

ljharb commented Jul 17, 2018

benjamingr commented Jul 17, 2018

benjamingr commented Jul 17, 2018

devsnek commented Jul 17, 2018 •

edited

Loading

benjamingr commented Jul 17, 2018

benjamingr commented Jul 17, 2018 •

edited

Loading

devsnek commented Jul 17, 2018 •

edited

Loading

devsnek commented Jul 17, 2018 •

edited

Loading

jasnell Aug 9, 2018

devsnek commented Aug 9, 2018

mcollina commented Aug 10, 2018 •

edited

Loading

Trott commented Aug 13, 2018 •

edited

Loading

Trott commented Aug 25, 2018

Trott commented Aug 25, 2018

devsnek commented Aug 29, 2018

fhinkel commented Aug 29, 2018

addaleax commented Sep 17, 2018

devsnek commented Sep 17, 2018


		Local<Function> fn = env->async_hooks_promise_event_function();

		v8::HandleScope handle_scope(env->isolate());

util: expose promise events #21857

util: expose promise events #21857

Conversation

devsnek commented Jul 17, 2018 • edited Loading

Checklist

nodejs-github-bot commented Jul 17, 2018

addaleax Jul 17, 2018

Choose a reason for hiding this comment

addaleax Jul 17, 2018

Choose a reason for hiding this comment

devsnek Jul 17, 2018

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

benjamingr commented Jul 17, 2018

BridgeAR commented Jul 17, 2018

BridgeAR left a comment

Choose a reason for hiding this comment

benjamingr commented Jul 17, 2018

devsnek commented Jul 17, 2018 • edited Loading

devsnek commented Jul 17, 2018

ljharb commented Jul 17, 2018

benjamingr commented Jul 17, 2018

benjamingr commented Jul 17, 2018

devsnek commented Jul 17, 2018 • edited Loading

benjamingr commented Jul 17, 2018

benjamingr commented Jul 17, 2018 • edited Loading

devsnek commented Jul 17, 2018 • edited Loading

devsnek commented Jul 17, 2018 • edited Loading

jasnell Aug 9, 2018

Choose a reason for hiding this comment

devsnek commented Aug 9, 2018

mcollina commented Aug 10, 2018 • edited Loading

Trott commented Aug 13, 2018 • edited Loading

Trott commented Aug 25, 2018

Trott commented Aug 25, 2018

devsnek commented Aug 29, 2018

fhinkel commented Aug 29, 2018

addaleax commented Sep 17, 2018

devsnek commented Sep 17, 2018

devsnek commented Jul 17, 2018 •

edited

Loading

devsnek commented Jul 17, 2018 •

edited

Loading

devsnek commented Jul 17, 2018 •

edited

Loading

benjamingr commented Jul 17, 2018 •

edited

Loading

devsnek commented Jul 17, 2018 •

edited

Loading

devsnek commented Jul 17, 2018 •

edited

Loading

mcollina commented Aug 10, 2018 •

edited

Loading

Trott commented Aug 13, 2018 •

edited

Loading