[async hooks] Criteria for exiting experimental

[ofrobots]

At a previous WG meeting we discussed the task (assigned to me) of figuring out the criteria for async_hooks to exit experimental. While #124 can continue to be the tracking issue keeping track of the concrete work that needs to happen, I am opening this issue as a discussion what being stable entails.

To become stable, the API must be well understood, well specified and well tested – but those are subjective.

• At previous WG meetings, in Async-context formalization and diagnostics support #107 and in Discuss blocking on Zones proposal TSC#340 (comment), @mike-kaufman and @mrkmarron have expressed the need (albeit not directly) for more formally defined semantics – perhaps at the language level – rather than the semantics being defined by implementation. I share this sentiment. We have gotten this concept wrong before, an it is a foundational concept for the language that needs some more rigorous treatment rather than being implementation defined. @mike-kaufman, @mrkmarron: LMK if I am inferring incorrectly.

• Similarly, the V8 team (e.g. @bmeurer) has expressed concerns about semantics being inadequately specified which is a barrier to VM being able to optimize safely. A recent example of this is the recent regression in Node 10 where async_hooks behavior changed in a way to break existing use-cases in a major way. The regression was a result of optimization work V8 did in order to improve async await performance. The optimizations V8 did were reasonable as per the spec. However, since Promise Hooks is neither well specified, nor well tested it is hard to know what behaviors are observable part of the Promise Hook contract and what aspects are ancillary. What all can the VM optimize? IMO, at a minimum the Promise Hooks API needs to be better specified (as if it was a language spec – even if it is not) so that we can have adequate levels of fuzz testing in V8 and a strong contract between Node and V8 on the semantics.

• Well-understood semantics: I am still making discoveries about async-hooks behavior that are surprising to me. For example, recently I learned that for certain kinds of promises the resolve hook will be called multiple times – what kind of promises this applies to left as an exercise for the reader. As a group we need to decide whether the semantics have gotten enough vetting for us to be comfortable calling it stable.

• Performance: AsyncHooks (esp. PromiseHooks), when enabled have a fair amount of performance impact and there are anecdotal reports from both extremes. We have not concluded Collect data about PromiseHooks performance #144 with data from the real world – although I know people are working on getting this data. More abstractly, what is acceptable level of performance impact? Should this even be part of the exit criteria?

• API Stability: To improve performance, we may need to change API as a result of the changes suggested in Proposal for Promise hooks to improve performance. #188. There is also a branch that @mcollina is working on that adds currentResource as a parameter to each callback. It is not clear whether we are API stable at this point.

[mike-kaufman]

I'm in strong agreement w/ first three bullet points. My gut says if that gets defined right, it obviates AsyncHooks & PromiseHooks (happy to be wrong about this...).

[bmeurer]

Thanks for kicking this off @ofrobots. On the V8 side we did revert a whole bunch of recent changes to get back to working state for Node 10. @MayaLekova already started working on adding more test coverage for async_hooks and making PromiseHooks fuzzable on the V8 side.

We had been discussing a plan with @hashseed and @mcollina two weeks ago that will allow us to close the gap performance wise, and we should definitely follow up on that during the summit (@hashseed, @MayaLekova and me will be there from V8 side).

[mcollina]

Some notes:

1. I think our current model is lacking something: a "join/randevouz" operator. This is critical for the correct modelling of async/await functions and to maintain the causality relationship. I have a couple of bubbleprof visualizations to show this. Happy to demo it at the summit.

2. my branch add async_hooks.currentResource() and it works similarly to how zones would work. It does not change any our external APIs, it is just a new semver-minor addition. I am waiting to open a PR because I would like async_hooks to be fully working before adding more functionality/changing things there. The code in the branch is ready from my point of view.

[mrkmarron]

Thanks for outlining these issues so clearly @ofrobots. The first bullet point captures my thoughts on the semantics issue very well and I am in complete agreement with all of the other bullet points as well.

[Flarna]

I think one major point is also documentation:

• I think it's needed to clearly specify which objects are passed as resource for the various async resource types - or document that relying on specifics of these objects is undefined behavior. Maybe just specify the characteristics users can rely on - or even an API to interact with the resource to e.g. store some context,... instead of forcing users to decide how to do that.
• We should consider to reduce the binding of public visible resource types to internal implementation details. e.g. for Node 11 FSREQWRAP has been renamed to FSReqCallback; TIMERWRAP has been remove - both internal changes now visible in public API. Otherwise I fear this may end up in a maintenance burden.

[github-actions]

This issue is stale because it has been open many days with no activity. It will be closed soon unless the stale label is removed or a comment is made.

[mmarchini]

I believe we need to evaluate which issues are still open for async_hooks, as we have some long and last-standing issues which are touching similar topics.

[boutell]

Hmm, time has passed... should any part of async_hooks be regarded as safe for use in production at this point?

I'm wondering what the official situation is because OpenTelemetry depends rather heavily on it (e.g. the mongodb wrapper, express wrapper, etc. are fundamentally based on it) and it is very much intended for production use.

[Qard]

AsyncResource and AsyncLocalStorage are marked stable already. But async_hooks is not likely to ever be considered stable due to too much leaking of internals and an API with too many edge cases and confusing usage. In fact, there's ongoing effort to satisfy the use cases for it in other ways so it can hopefully eventually be deprecated or marked as legacy.

[mcollina]

@Qard I think we could just mark async_hooks as "internals" but keep exposing them.