Proposal for reworking promise integration into async_hooks

Open
#389 11 comments 7 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Assessment

Difficulty
5/5
Estimated time
Over a week
Newbie friendliness
20/100
Issue type
Feature
Clarity
Needs clarification
Activity status
Stale
Tech stack
node.js

Research direction

Start by reviewing the existing async_hooks promise integration and the proposed promise_hooks module, then examine how [[then]] and async/await are handled. The open question about a custom MicrotaskQueue and the linked diagnostics issue must be resolved; done means a defined integration design that addresses compatibility and can be scheduled for a major release.

Written by the indexing model from the issue text.

Description

never stale

Background

The current implementation of promise integration into async_hooks is.

  1. For each new [[Promise]] object, call the init hook with a PromiseWrap referencing that Promise.
  2. At [[resolve]] or [[reject]] call the resolve hook.
  3. For each [[then]] callback, call the before hook before calling the callback.
  4. For each [[then]] callback, call the after hook after calling the callback.
  5. When the Promise object is garbage collected call the destroy hook.

I think this implementation is fundamentally wrong, as it intertwines the promise lifecycle with async_hooks. This causes a number of issues that are currently blocking us from making async_hooks stable.

  • Performance issues caused by listening for the garbage collection event.
  • Thenables are not tracked when used by a native function that creates a microtrask.
  • destroy hook is not called if async_hook is enabled after Promise creation.
  • Tracking the async boundary when multiple .then() calls are used on the same promise object, is not possible.

Proposal

My proposal is to rework the promise integration into async_hooks such that the async barrier is around the [[then]] call, not creating a new Promise object.

  1. at the call of [[then]] on a promise or thenable create a resource object (or use the promise/thenable object created by [[then]]) then call the init hook.
  2. at the call of the[[then]] callback, the before hook is called.
  3. at the end of the[[then]] callback, call the after hook, immediately after call the destroy hook.
How it solves the above-mentioned issues
Performance issues caused by listening for the garbage collection event.

Because the before and after hooks are only called once per resource, the destroy hook can be called immediately after the after hook. Thus completely eliminating the need to track promise objects in the garbage collector.

Thenables are not tracked when used by a native function that creates a microtrask.

This can now be solved because we don't need to know when the object was created or destroyed. The only knowledge that is required is when the [[then]] method of the thenable is called by the native JS APIs which invokes the microtask queue. That is actually doable, as we could hook into those APIs. Only manual calls to [[then]] on a thenable will not be tracked. But I don't see that as a concern, because that is not an async action.

destroy hook is not called if async_hook is enabled after Promise creation.

Again, because the destroy hook is called with the after hook, calling the destroy hook becomes trivial.

Tracking the async boundary when multiple .then() calls are used on the same promise object, is not possible.

This directly confronts this issue, by making the async boundary the [[then]] call.

Tracking the lifetime of promises

This proposal removes features from async_hooks that provide insight into promises. That information is still valuable.

To keep providing that information, I propose making a dedicated promise_hooks module. A user can then connect the promise lifecycle with async_hooks via a promiseId that is exposed both in promise_hook and via the resource in async_hooks.

Compatability

This does change the default async chain, as the start point of the async-barrier is now the .then() call and not the new Promise call. However, I think this is actually a preferred default. For example, lazyloading a resource with new Promise, would currently not be tracked correctly, but with the proposed change it will.

Even though this changes the async graph, the proposed async graph will still be valid and useful in most cases. And the existing async graph can be restored by integrating the proposed promise_hooks module.

implementing this proposal should be part of a major release.

Open questions

  • It is unclear to me how this would integrate with async/await. As I don't have a good grasp of how the internal [[then]] calls are used and wrapped. It has been proposed that we supply our own custom MicrotaskQueue (https://github.com/nodejs/diagnostics/issues/376#issuecomment-623682088) to solve this. I think this could also be a good approach to hook into the native promise APIs, which will be necessary to track [[then]] calls on thenables.
Dominant language
No language data
Stars
550
Forks
69
PR merge metrics
No merged PRs in 30d

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

More from nodejs/diagnostics

All issues in nodejs/diagnostics

Similar issues

More DevTools issues

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.