util: expose promise events

[devsnek]

Refs https://github.com/nodejs/promises-debugging/issues/8 Closes https://github.com/nodejs/promises-debugging/issues/8

exposes env promise hooks to userland

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

Checklist

• [x] make -j4 test (UNIX), or vcbuild test (Windows) passes
• [x] tests and/or benchmarks are included
• [x] documentation is changed or added
• [x] commit message follows commit guidelines

[nodejs-github-bot]

@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

[benjamingr]

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]

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.

[benjamingr]

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]

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]

@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]

@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]

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]

@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]

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]

@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]

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]

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]

@benjamingr @BridgeAR unhandledRejection event remains. This PR still provides a solution to exposing the events requested in https://github.com/nodejs/promises-debugging/issues/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

[AndreasMadsen]

@devsnek I don't really understand why this belongs in async_hooks. async_hooks does as a principal not involve itself with error handling or error capture. Years ago there was discussion about this but it was decided to not be included. I suppose we could change that, but in that case it should just be done for promises. We have many more resource type than that.

In general the async_hooks concepts tries to be resource agnostic. I have indeed also though we should rename promiseResolve to just resolve as there have been some suggestions that it would be relevant for other things than promises. See the microsoft paper on this: https://www.microsoft.com/en-us/research/wp-content/uploads/2017/08/NodeAsyncContext.pdf

Don't get me wrong, I do see the benefit of this. I just don't see its relation to async_hooks because it is so promise specific.

[benjamingr]

@AndreasMadsen well, promises are different from other stuff async_hooks are used for anyway - even the existing resolve hook. This is a little footgunny - a recent example is jest listening to all async hooks and giving an error because there was a pending unresolved promise (which is totally fine unlike an open file handle for example).

As a reference - here is the V8 doc https://docs.google.com/document/d/1ZDafLmQmvP4LgQNtUyLWaSgyhXwgr5nJ624eON0ix5w/edit

[devsnek]

@AndreasMadsen do you have a better idea of where this could be exposed? it needs to be exposed somewhere.

[AndreasMadsen]

@benjamingr There are plenty of other resources, not just promises, where the resource just exists. In essence, any async resource can be implemented as a pseudo-promise without actually being a promise.

@devsnek Honestly, I don't know. /cc @nodejs/diagnostics @jasnell

[mcollina]

I'm tagging this semver-major because async_hooks is used in the wild quite a bit, and I would prefer this not to get into existing lines.

Apart from this, I'm -0 with the changes.

[devsnek]

my current thought on what to do with this is require('util').addPromiseHook(callback) (and removePromiseHook).

does anyone have a more idiomatic idea?

[benjamingr]

does anyone have a more idiomatic idea?

Going to try another ping to @nodejs/async_hooks

I think additions of a method to process would require TSC approval and personally I think this is as much async_hooks as the resolve hook.

This has no -1s and the code changes LGTM (both under async hooks and separately), I wanted Ruben's feedback on it though too.

[AndreasMadsen]

Okay, I will give it a -1 just to be clear about my opinion.

If you want to change my opinion, then please explain it's relation to async resources in general. Without defeing to argument such as "well we already have promiseResolve".

[devsnek]

@AndreasMadsen the reasoning to me is two parts

1) async_hooks doesn't have to just represent lifetime events, but async behaviour in general

2) promises are central to idiomatic async behaviour in javascript

i agree that it doesn't fit in well next to the init/destroy hooks but i don't think there is a better place for it.

if it weren't to be in async_hooks i would argue for a new promise_hooks module but that seems a bit too split up

[benjamingr]

@AndreasMadsen by all means you are welcome to object to PRs and voice your opinion. None of us feel super strongly about this hook landing in this particular way and the fact you have a formed opinion on it is already indication that you should object if you do not like the proposed API and we should have a discussion about it.

Personally, I don't understand way promiseResolve is an async_hook to begin with if async hooks are only for tracking resources. If async_hooks are not only for managing async resources but also for tracking asynchronous context in asynchronous constructs then both promiseResolve and promiseEvent makes sense since it allows us to keep the context for these cases.

My understanding was the latter but I am not an async_hooks team member and haven't kept up with the discussions in a while and I will gladly learn about it.

[AndreasMadsen]

@benjamingr Have you read the async context paper? It makes it pretty clear why “resolve” is necessary in terms of async context.

[devsnek]

@AndreasMadsen would you mind paraphrasing that paper? all i got from it was some formulas I don't know how to read trying to explain how event loops work.

[benjamingr]

@AndreasMadsen

It makes it pretty clear why “resolve” is necessary in terms of async context.

Bear with me since this is not my area of research so I might be making silly assumptions or mistakes:

That paper doesn't say anything about which hooks node should expose but only what minimal subset it must for the formalisation. The paper discusses a formalisation of JS semantics with how Node.js code (if we disallow all native modules) is expected to behave in terms of context ('linking' and 'causal' tracking). I don't see any discussion of what hooks Node.js should expose at all - I see a formalisation of a subset of how Node.js executes asynchronous code on various queues.

It was an interesting read (and I absolutely understand why a hook for resolving promises is required for tracking asynchronous context - which was plenty obvious even before). I am just not sure why this means we can't expose an async hook for something else developers find useful (someone trying to resolve a promise).

Do you just think it's just bad in terms of API surface?

[mike-kaufman]

I think what @AndreasMadsen is saying (plz correct me if wrong) is that adding general promise event notifications to async hooks' pollutes the API. i.e., you're starting to conflate general promise events with the events necessary to track async context; the resolve hook is in async-hooks precisely because it is necessary to construct the "causal context".

[addaleax]

If it helps to clarify things: Afaik, the promiseResolve() hook is an optimization – we could model it in terms of the other hooks, by having separate async IDs/resources for individual microtasks, with their trigger ids pointing to the Promise (and the Promise never having its own before/after callbacks called).

[AndreasMadsen]

@benjamingr I agree that the hook should be exposed. But it has nothing to do with asynchronicity. It has to do with promises and error handling. Thus it should not be exposed to async_hooks. (so @mike-kaufman is right in what I'm trying to say).

@devsnek @benjamingr Basically you can express async context as two different graphs (actually there are three, but the paper only models two of them).

We can call one the resolve graph, and the other the init graph. Meaning in the resolve graph you listen for the promiseResolve event and uses the executeAsyncId for the context. And in the init graph you listen to the init event and use the executeAsyncId for context. Both of these graphs are valid viewpoints and have different use-cases. This is why promiseResolve exists in async_hooks.

The thrid graph has to do with triggerAsyncId in the init event.

@addaleax is theoretically correct, as the init graph could be turned in to a resolve graph if some extra "binding resources" where added. On a theorically level the same could be done for triggerAsyncId graph. However, it would be a minor nightmare for the user to work around thoese extra "binding resources". Resources that would only exists for semantic reasons and hence be pretty confusing for even an advanced user.

[benjamingr]

@AndreasMadsen thanks for explaining - that makes sense. I agree we should keep resolve as a hook for tracking contexts and export that as a separate hook since it has a different concern.

If it's an API concern then I understand and sympathize. Do you have a proposed alternative?

Honestly I don't feel too strongly about it other than that we should expose it somehow.

[benjamingr]

@AndreasMadsen Sorry for the offtopic - but do you have a formal proof that this specific model 'works'? (That is, a proof that shows that given the source code of a program context flows correctly in all cases - given prior formalizations of JavaScript for stuff like escape analysis and induction models - it should be doable and very interesting to see). Have you tried collaborating with Mark Miller on any of this? He has a lot of interesting (and relevant) work?

[AndreasMadsen]

If it's an API concern then I understand and sympathize. Do you have a proposed alternative?

It is an API concern, in the sense of where it is placed. How exactly the C++ API is exposed to JS I don't really care about. Right now we have these things on the process object. If that is not acceptable then I suppose we should work forwards moving all those error-handling APIs to their own module.

[AndreasMadsen]

@benjamingr I'm not sure what model you are talking about. The "unified everything can be expressed in the init-graph model"?

I haven't thought much about the resolve graph, but the redundancy of triggerAsyncId is discussed in great detail here: https://github.com/nodejs/node/issues/14238

If you have already proved that the resolve and init graphs are sufficient on their own, then it is pretty easy to extend that to a unified graph:

"Consider the init graph. Then extend this graph with the edges from the resolve graph. To attach each resolve edge in the extended graph to its parent promise, then add a binding resource that has the promise as a parent node and the orphan start-node of the resolve edge as a child. Annotate this resource to be a binding resource. The resolve graph can then be separated from the extended init graph by only following the annotated binding resources. QED"

It could properly be more exact, but I think it is pretty easy to see that two graphs with similar nodes can be merged and separated after as long as the edges can be annotated. However, since only nodes can be annotated just add a dummy binding node (resource) on the edge.

[BridgeAR]

@mike-kaufman

adding general promise event notifications to async hooks' pollutes the API. i.e., you're starting to conflate general promise events with the events necessary to track async context

I agree with that.

---

I just wonder what the current use case is for the events. The existing events are exposed through the unhandledRejection hook and it was added to provide better feedback to users. And we want to further improve the API and the user experience as well as it is still sub par. But what is the reasoning for exposing the actual event? IMHO it should not be exposed because the feedback API is something a user should actively opt-out from instead of having to opt-in by using a module. Yes, the preprocessing might not always be what the user wants but in that case we could just expose the raw information in combination with other things for any new event added (like the resolve after reject and resolve after resolve cases).

I would like to discuss this matter further in the promises working group and put this on hold until we get a conclusion there. I am happy to set up a doodle to find a meeting date if requested.

[devsnek]

The existing events are exposed through the unhandledRejection hook and it was added to provide better feedback to users.

But there is no case for the unhandledRejection hook where it is correct in all cases so lets not use that as a model.

it should not be exposed because the feedback API is something a user should actively opt-out from instead of having to opt-in by using a module.

can we please stop having debug tooling enabled by default? please?

Node got too opinionated with the unhandled rejection event and I'm trying to expose an agnostic interface to promise behaviour. I don't want to continue trying to help out in this area if the promise debugging team keeps making decisions that enforce a certain behaviour.

[benjamingr]

@devsnek the promise debugging team is working on a survey to figure out what the user expectation is. Most of the feedback so far (like the netflix survey) was that the warnings are good and people expect crashing and are confused about why Node.js doesn't crash on promise errors.

Again, you are welcome to be involved in any or all of these efforts - it's not gated and the promises debugging team has an open call for members. It is a relatively new team of people who have spent a considerable amount of time looking into the topic and we don't have a "team policy" yet.

Personally I think Node.js should "chug on" un uncaught exceptions thrown in async functions if and only if it "chugs on" on exceptions thrown in sync functions.

Also, in my opinion debugging things in production is a real and painful use case that Node.js should get much better at and we are currently not great with. One of the conclusions from the summit for example is that async stack traces for production are really important to people.

If you prefer having a call instead of back-and-forth messages and want to discuss what the behaviour should be I'm also fine going through the history and all the previous efforts with you. I realize going over all the previous work is frustrating and time consuming and I'm willing to spend the time in order to get you to engage with the team. This is not an adversarial situation, none of us have immutable opinions (but we do have strong ones) and we're trying to figure out together what's the right way to go about this.

[devsnek]

@benjamingr i'm happy to read anything you push in front of me (i feel the same way about people engaging the modules group with no context). I just really want to make sure node doesn't end up with behaviour that disables valid use, which is what i see happening now.

[benjamingr]

@devsnek

Re: prior work:

There was a ton of discussion in https://github.com/nodejs/promise-use-cases/issues and there is a summary and slides at https://github.com/nodejs/summit/issues/86 (it was joint work with V8). All the use cases in https://docs.google.com/presentation/d/16WYzlvvDm85IruD3jkH2nTwTkTUa6YSPtDF0VQ4g4Hk/edit are explain in the promise-use-cases repo with expected behaviour.

Unhandled rejection work goes back a lot further than the summit with GitHub code surveys from before we added this hook. Here is the original hook proposal https://gist.github.com/benjamingr/0237932cee84712951a2 - there are two big discussion threads on the default behaviour on the defunkt nodejs/promises repo though they are largely obsolete.

It also goes back from before promises were added to Node.js itself - I can dig a lot of behaviour discussions from various repos - we don't have everything summarized. My goal is:

• In ~1M send out the survey to the foundation so they can edit it. I have been actively seeking people with different opinions so they participate in forming the questions - suggestions are welcome.
• Based on the survey results - implement the action items regarding writing a guide and deciding what hooks we want and want to enable by default as well as work on the post-mortem situation.
• Based on the survey results - see what assumptions we can relax and what the behaviours should be on different use case.

In addition there is some ongoing work from parties such as:

• Netflix did a survey and are working on tooling and summaries of the situation as well as analysis of better postmortem debugging approaches
• V8 are working on async stack traces as well as hooks (like this one). We are not turning any warnings on by default - like the unhandledRejection hook which was added in v1.3.0 - the goal is to monitor these hooks for a year and then formulate a policy about turning them on.

(This does not block Ruben's PR which enables GC tracking but does not change the default at this point)

---

Now I think it's important to say that Node.js promises have configurable behaviour that allow users to pick what behaviours they want. I don't understand "not enabling" behaviour - our current behaviour is very similar to what browsers do and most feedback I've gotten was about it not being strict enough.

If people want to suppress uncaught errors in order to behave in a way similar to browsers they can add an empty unhandledRejection handler which they could always do - similarly to uncaughtException.

process.on('unhandledRejection', () => {})

Which people rarely do, what people do instead is usually what core itself does when we do common.crashOnUnhandledRejection - in fact it was recently made it opt out rather than opt in to reduce mistakes.

Again, I strongly believe that we should:

• Enable as many use cases as we can - this is why we allow the user to pick whatever behaviour they want in Node.js
• Supply the safest and best defaults for debugging as we can. We are not sure what this is yet - but I do feel like we have enough data at this point to not ignore uncaught errors from promises. This assumption will be validated again in an official foundation survey.

---

I am sorry if this is a lot at once and I'd like to emphasize that the promises-debugging team wants different opinions and ideas and I am personally willing to answer any questions you might have about prior work, what we tried etc as well as sit with you 1x1 in an hangout to discuss the whole thing.

We should have had a good promises usage story in 2015, it's 2018 and it's still not near where we want it to be - your participation is valuable enough for me to gladly be willing to spend that time.

[ljharb]

Is it a safe default if third-party code that runs fine in the browser (with an unhandled rejection) fails in node?

[mcollina]

Is it a safe default if third-party code that runs fine in the browser (with an unhandled rejection) fails in node?

Yes, I think so. A Browser is an environment with only one single user. A Node.js server has hundreds or thousands of concurrent users. A tiny leak on a Node.js server could cause a significant amount of problems. It is way better that error condition are always handled and if not it is a serious bug that should be resolved promptly by the developers.

[benjamingr]

@ljharb I don't think this PR is where we should be having this discussion - but Node makes no guarantee that code that runs in the browser works in Node.

setTimeout(() => { throw new Error(); }, 1);
setTimeout(() => { console.log("Hi"); }, 10); 

Which "fails" in Node but "succeeds" in the browser. This is a fundamental difference between the browser and Node. Their execution model is different on the same language - the code also behaves differently on different platforms with different APIs.

Whether or not ignoring uncaught errors is a good policy or not is another question which we should discuss - but whatever we choose to do with unhandled rejections is orthogonal to browser/node interoperability.

That said, third party code may choose to add its own unhandledRejection error and suppress errors originating inside it and forward the rest which would let it pick its own behaviour. The fact is very few if any packages do this.

Again - I am totally up for discussing our behaviour again - but I do not understand the argument that it aligns with browsers (or not).

[benjamingr]

Let's bikeshed where to put this hook.

We can:

• Put it on the global Promise
• Put it on process
• Put it on async_hooks in a way that differentiates it from other hooks used for tracking context
• Put it some place new - like a new promise_hooks module or even engine_hooks and bunch other hooks from V8 we use there.

Also, ping to @nodejs/chakracore in case they also want to implement this since IIUC their implementation is somewhat debugging-focused

[devsnek]

ftr i don't feel comfortable about putting it on Promise or process

[benjamingr]

Ok, @devsnek - I personally agree that both process and Promise are ugly places to put it and given the async_hooks team feels strongly against putting it there - I think we should bring this up to the TSC. https://github.com/nodejs/promises-debugging/issues/13

[mike-kaufman]

RE chakracore, we should be able to add in equivalent events at some point. Not complex, but I can't guarantee when it will light up in node-chakracore.

RE where to expose this data through node's API, I would suggest something where the effectively the raw "HostPromseRejectionTracker" events can propogate out to user code, process.on("HostPromiseRejectionTrackerEvent", (...)=>{...});, seems simplest, or if you don't want it there, a promise-specific thing. Users of this API will likely need to ignore the uhhandled/handled events, but I think that's OK.

RE "async context" in general, we've been working on trying to get to a crisp definition of "async context". There's a PR as part of the diag WG here: https://github.com/nodejs/diagnostics/pull/197. It attempts to "democratize" some of the concepts in the MSR paper Andreas cited above. I've also got a deck on this and we'll be doing a deep-dive in the next few weeks. All feedback on this (positive or negative), is very much appreciated - this is a complex topic that lacks agreed-upon terminology, so part of our goal is to get to shared understanding & terminology.

[devsnek]

i'm cool with exposing HostPromiseRejectionTracker on the basis that it replaces unhandledRejection+rejectionHandled

[mike-kaufman]

i'm cool with exposing HostPromiseRejectionTracker on the basis that it replaces unhandledRejection.

The challenge with this is there's a bit of complexity around going from the "HostPromiseRejectionTracker" events to a rejection that might be "unhandled". Consider

Promise.reject().catch(()=> {});

From the perspective of the JS VM, the Promise.reject() call results in a promise that is uhandled at the time of its rejection. Hosts generally have some logic around this to "give a little time" for "things to settle down" - e.g., I think node waits for the nexttick & micro-task queues to drain, before it raises the unhandledRejection event.

If you remove the unhandledRejection event, you're pushing this analysis into user code & user's don't have visibility into the internals to make an informed decision in this space.

[devsnek]

@mike-kaufman i say that on the basis that you can't really ever say that a promise is unhandled until it gets gc'd (because they aren't designed to have a concept of unhandled) and there is no guarantee (in node.js) that a promise will get gc'd so node trying to define an "unhandled rejection" is pointless and misleading.

On the other hand, exposing the hooks that define what promises are actually doing let userland make up their own opinions about how to handle promises instead of node.js exposing terrible things.

[mike-kaufman]

so node trying to define an "unhandled rejection" is pointless and misleading.

Sure, if you're being pedantic. :) Per @benjamingr's points above, users generally find the warnings useful. I personally don't think it's a bad thing. Perhaps wording can be changed to a "potentially unhandled rejection", which would then be technically accurate, but... wordy.

My opinion is, weighing the tradeoffs between a) doing nothing b) pushing all analysis into user space c) having node make some "reasonable guess" as to when to act on a "potentially unhandled rejection"

c) is the best choice. Ideally, there'd be some user dials around the "when" and the "act" - reasonable ppl may disagree here.