feat(core): add Deno.core.setPromiseHooks

[lbguilherme]

Fixes #5638.
Based on the previous work at #8209 by @benjamingr.
Unblocks denoland/std#2306.

This PR adds Deno.core.setPromiseHooks that will enable monitoring of async contexts. Please see previous discussion at the linked issues.

[CLAassistant]

All committers have signed the CLA.

[bartlomieju]

Thanks for looking into this!

[bartlomieju]

@cjihrig please take a look if you find a moment

[lbguilherme]

Some points for consideration:

• The API is named Deno.core.setPromiseHook, but it actually adds a new promise hook and could be better named as Deno.core.addPromiseHook. I went for keeping the same name as the underlying V8 method.
• There is no way of unregistering hooks. Maybe it could return a function that when called unregisters the just-added hook? Not sure. As this is a low level API, I wonder if there is a use case for unregistering a hook.
• Perhaps the interface could be changed to receive an object instead of four function arguments. Something like this:

  Deno.core.setPromiseHook({
    init(promise, parentPromise) {
      console.log("init", promise, parentPromise);
    },
    resolve(promise) {
      console.log("resolve", promise);
    }
  });

  Instead of:

  Deno.core.setPromiseHook(
    (promise, parentPromise) => {
      console.log("init", promise, parentPromise);
    },
    undefined,
    undefined,
    (promise) => {
      console.log("resolve", promise);
    }
  });

  Again, I went for keeping the same interface as V8.

[cjihrig]

Left a few comments. Also kicked off a CI run for you.

[bartlomieju]

Is the current API similar to Node's API?

[cjihrig]

Is the current API similar to Node's API?

Node's API is implemented in C++ here. That code calls this code.

So it seems to be different from Node's, although this is also not a public API in Node.

Dismissing my review. Looking at the test, and comparing it to #8209, it would be useful to comment the test more.

[lbguilherme]

This is intended as an internal low-level API on top of which a proper public API can be later constructed.

Node.js has the async_hooks API that is somewhat akin to this one, but Node also tracks async work scheduled from IO events. This module is marked as "experimental" and its future is uncertain.

There is also the async_context API that is stable and more high-level. It defines the AsyncLocalStorage and AsyncResource classes. Some equivalent to those could be defined on Deno's stdlib.

---

This PR provides setPromiseHooks, which is useful on itself and enables tracking promises. I believe some effort will be required to also be able to track other callbacks such as setTimeout.

[bartlomieju]

@lbguilherme please rebase and address the question from @cjihrig so we could land it for Deno v1.25 this week

[lbguilherme]

Done! This is now rebased on top of the current main and the test is properly commented and explained. Please check again.

[cjihrig]

The only thing that still isn't obvious to me is why the events in the test are different from the ones in #8209, even though the test code appears to be the same. Did something change in V8, or was the original implementation buggy?

In #8209, the first few events are:

init
init
resolve
init
init
init <-- this is a before event in this PR

[lbguilherme]

When a promise first begins to resolve it will also trigger the creation of the promise related to the then block of other promises anywhere in the code that are ready to resolve. My explanation is that there are some promises from the test runner runtime that get a chance to run at that point and are noticed by the test itself.

Previously there was some number of init events. With a more recent version of Deno something in the test utils changed and this number increased.

This is why I'm doing a bogus await at the beginning of the test now, to ensure that any unrelated promise that is waiting to run gets a chance to run before the test starts.

See this section: https://github.com/denoland/deno/pull/15475/files#diff-4ba8d96f30b74cf0a30189b1b111204e319d27a7785519bfec3941f6c5941190R26-R28

Now every event has an explainable reason to be there.

[cjihrig]

Thanks for the explanation. The code LGTM.

@bartlomieju since you've done most of the work on the test runner, I'll let you merge.

[lbguilherme]

The test failure seems unrelated. Web tests about WebSocket.

Should I do something?

[cjihrig]

I re-ran the failing CI jobs. It looks like it was a flaky test. CI is green now.

[benjamingr]

LGTM, thanks for picking this up

[benjamingr]

Note that it's probably a good idea to bring this API to APM vendors to make sure that's the API they'd like or implement a basic APM on top of it and see if there are obvious issues.

That said: I'd just land it as is (it looks reasonable) and worst case change it based on future APM feedback.

[hastebrot]

@bartlomieju are there chances this will be included in deno v1.25.1?

[bartlomieju]

@bartlomieju are there chances this will be included in deno v1.25.1?

No, we'll ship it in v1.26

[benjamingr]

@hastebrot just wondering, would you be using this for AsyncLocalStorage or for APMs/context tracking?

[hastebrot]

would you be using this for AsyncLocalStorage or for APMs/context tracking?

I'd use it for an APM solution.

AbstractAsyncHooksContextManager could be extended to have the Deno version of https://github.com/open-telemetry/opentelemetry-js/blob/v1.6.0/packages/opentelemetry-context-async-hooks/src/AsyncHooksContextManager.ts#L28-L34.

import { context, trace } from "npm:@opentelemetry/[email protected]"; does not work right now, but "https://cdn.skypack.dev/@opentelemetry/[email protected]" works. "npm:@opentelemetry/[email protected]" works as of Deno 1.25.1.

If it could work with an async handler for std/http/server.ts it would be a first step.

[lbguilherme]

I believe this would be an equivalent implementation for Deno:

import { Context, ROOT_CONTEXT } from '@opentelemetry/api';
import { AbstractAsyncHooksContextManager } from './AbstractAsyncHooksContextManager';

interface HookCallbacks {
  init: (promise: Promise<unknown>) => void 
  before: (promise: Promise<unknown>) => void 
  after: (promise: Promise<unknown>) => void 
  resolve: (promise: Promise<unknown>) => void 
}

const enabledCallbacks = new Set<HookCallbacks>();

Deno.core.setPromiseHooks(
  (promise: Promise<unknown>) => {
    for (const { init } of enabledCallbacks) {
      init(promise);
    }
  },
  (promise: Promise<unknown>) => {
    for (const { before } of enabledCallbacks) {
      before(promise);
    }
  },
  (promise: Promise<unknown>) => {
    for (const { after } of enabledCallbacks) {
      after(promise);
    }
  },
  (promise: Promise<unknown>) => {
    for (const { resolve } of enabledCallbacks) {
      resolve(promise);
    }
  },
);

export class AsyncHooksContextManager extends AbstractAsyncHooksContextManager {
  private _contexts: Map<Promise<unknown>, Context> = new Map();
  private _stack: Array<Context | undefined> = [];
  private _callbacks: HookCallbacks = {
    init: (promise) => {
      const context = this._stack[this._stack.length - 1];
      if (context !== undefined) {
        this._contexts.set(promise, context);
      }
    },
    before: (promise) => { 
      const context = this._contexts.get(promise);
      if (context !== undefined) {
        this._enterContext(context);
      }
    },
    after: () => {
      this._exitContext();
    },
    resolve: (promise) => {
      this._contexts.delete(promise);
    }
  }

  active(): Context {
    return this._stack[this._stack.length - 1] ?? ROOT_CONTEXT;
  }

  with<A extends unknown[], F extends (...args: A) => ReturnType<F>>(
    context: Context,
    fn: F,
    thisArg?: ThisParameterType<F>,
    ...args: A
  ): ReturnType<F> {
    this._enterContext(context);
    try {
      return fn.call(thisArg!, ...args);
    } finally {
      this._exitContext();
    }
  }

  enable(): this {
    enabledCallbacks.add(this._callbacks);
    return this;
  }

  disable(): this {
    enabledCallbacks.delete(this._callbacks);
    this._contexts.clear();
    this._stack = [];
    return this;
  }

  private _enterContext(context: Context) {
    this._stack.push(context);
  }

  private _exitContext() {
    this._stack.pop();
  }
}

As pretty much all async operation in Deno is implemented on top of Promises, tracking them should be enough to keep track of all async contexts. It should work with Deno.serve out of the box, but I didn't test it.

Again, it's fairly low level and the interface doesn't help. It would make sense to introduce a more friendly wrapper on the stdlib later on.

[bartlomieju]

Again, it's fairly low level and the interface doesn't help. It would make sense to introduce a more friendly wrapper on the stdlib later on.

That's definitely a good idea

[bartlomieju]

LGTM