async_hooks: multi-tenant promise hook api by Qard · Pull Request #39283 · nodejs/node

Qard · 2021-07-06T00:32:08Z

I've introduced a new promise_hook module which exposes the PromiseHook API in V8 more directly to userland. I've also updated async_hooks to consume this new API for its own promise lifecycle events. This is part of my ongoing effort to break down async_hooks into more purpose-specific components that can be used directly rather than using async_hooks itself which conflates many different data sources and can be awkward to use at times. Feedback is welcome. 😸

targos · 2021-07-07T10:13:59Z

@nodejs/async_hooks

test/parallel/test-promise-hook-create-hook.js

doc/api/promise_hooks.md

AndreasMadsen

I like this direction. In my opinion tracking promises should not be a part of async_hooks. except for .then() because that is the only async operation.

However, this seems to be some wired middle ground. Where promise_hooks lives as its own documentation entry but is still merged with async_hooks, while also having a different API than AsyncHook when it doesn't need to.

I would like to either separate the modules or keep the API similar and the documentation together.

doc/api/promise_hooks.md

doc/api/index.md

AndreasMadsen · 2021-07-07T12:37:15Z

lib/internal/promise_hook.js

+  if (after) ArrayPrototypePush(hooks, onAfter(after));
+  if (resolve) ArrayPrototypePush(hooks, onResolve(resolve));
+
+  return () => {


I see it as API fragmentation to deviate from the enable and disable methods in async_hooks.

What do you think of createHook returning an object with enable/disable while the on* functions continue to just return stop functions?

To me, the API of async_hooks is more complicated than it needs to be, especially here where a lot of users are likely only going to care about the init event. I'm not a fan of explicitly needing to call enable at the start--why even create the hooks if you aren't ready to enable them yet? Personally I'd rather just supply a function again if I want to attach it again rather that wrapping it in an unnecessary tracking object that does nothing else except attach and detach it. But then I also tend to prefer more functional code in general. 🤷

Sometimes it is useful to be able to have a state in its own scope, like a WeekMap for the promises, while also delaying tracking until an event, for example server.on('listen').

I don't think I had a strong opinion on what the default was (enabled or disabled), but there is a real benefit in being able to enable hooks again after they have been disabled.

Sure, but I don't see why it needs an object to manage enabling it again when you could just pass the function in again. 🤷‍♂️

The scope where you enable again may not be the scope of the WeekMap. In general, I don't think hooks should be functional. Hooks don't return anything and inherently depend on mutating a state to do something, so they will never truly be functional.

Not sure what you are referring to about a WeakMap. As for if it's functional or not, I don't think that matters here. I just find it simpler to use functions rather than objects. It's also safer as internals can’t be tampered with. If people feel strongly that it should be object-based though, I'm happy to reconsider.

Thinking about it more, I would say making it look more like async_hooks would actually be very confusing. More than just the specific intent being different, the actual functionality is different enough that it would just be a source of confusion, especially if we opt to move the API back into the async_hooks module. The set of lifecycle events does not match what async_hooks produces as there's no destroy event, and the hook functions all receive the promise object, there is no ID mechanism like async_hooks has, so the hook functions are different enough the making the API look too similar to async_hooks would most likely just confuse people that would expect functionality to be identical when it's not.

I'm torn on the enable/disable question. I prefer the function return but, with the enable/disable pattern, I can create the hook but more precisely control when it is enabled. I like having that ability to separate the creation of the hook from the enabling of it. I can live with this API design tho.

I've dealt with all the firm feedback. Would you like me to change the form of this API to mirror that enable/disable functionality? I'm okay with it either way, I just went for this design because it was simpler.

Qard · 2021-07-08T02:26:16Z

I'm happy to make this top-level if people prefer. I just chose to put it under async_hooks because we've historically done that with some other hooks-related things like AsyncLocalStorage.

AndreasMadsen · 2021-07-08T08:17:24Z

I'm happy to make this top-level if people prefer. I just chose to put it under async_hooks because we've historically done that with some other hooks-related things like AsyncLocalStorage.

Well, I also found that a wired choice. I'm fine with it being in async_hooks, my reasoning is just that if I'm looking for documentation for { PromiseHook } = require('async_hooks') then I would look in the async_hooks documentation page. Not somewhere else.

Qard · 2021-07-09T00:55:04Z

My latest change makes it a top-level module of its own. Still need to do some stuff around error handling, but I'll be on vacation for the next week so I'll have to get back to this later. Thanks for the feedback so far! 😄

Qard · 2021-07-20T21:44:44Z

I've added a test which verifies proper handling of exceptions within hooks. The JS code doesn't actually do anything with exceptions, the exception routing is entirely within the original PromiseHook changes I made in V8, this test just confirms that they get routed to the uncaughtException handler properly and don't interfere with other code.

Flarna · 2021-10-25T12:53:34Z

Changed from semver-major to semver-minor as this no longer adds a new top level module.

Qard · 2021-10-25T15:52:26Z

Rebased. Hopefully that will help with the CI failures.

Qard · 2021-10-25T17:07:59Z

Test failures all seem to be unrelated but consistent. They appear to be causing failures in many other PRs too, according to the CI reliability reports. Not sure how to proceed other than just waiting for those tests to get fixed.

nodejs-github-bot · 2021-10-25T19:19:50Z

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

nodejs-github-bot · 2021-10-27T18:29:15Z

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

Flarna · 2021-10-27T19:40:43Z

@Qard Finally CI is green. Do you want to adapt the commit message during landing as component is no longer async_hooks?

Qard · 2021-10-27T19:43:36Z

Does our tooling support the new subsystem? Not sure if our commit linting would blow up on that.

Flarna · 2021-10-27T19:47:08Z

It's part of v8 now which is not that new.

Qard · 2021-10-27T20:33:42Z

Oh, you mean change to "V8" as the subsystem? I'm good with that. Is that how we express changes to the V8 module? Not sure if that's confusing with the distinction between V8 itself and our module for it. Whatever matches what we're doing currently is good with me though. 👍

Flarna · 2021-10-28T18:41:17Z

To my understanding that's how it's done usually.

PR-URL: nodejs#39283 Reviewed-By: Gerhard Stöbich <deb2001-github@yahoo.de> Reviewed-By: Vladimir de Turckheim <vlad2t@hotmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>

Trott · 2021-11-01T18:11:27Z

Landed in da82844

Qard added doc Issues and PRs related to the documentations. lib / src Issues and PRs related to general changes in the lib or src directory. promises Issues and PRs related to ECMAScript promises. async_hooks Issues and PRs related to the async hooks subsystem. labels Jul 6, 2021

github-actions bot added the needs-ci PRs that need a full CI run. label Jul 6, 2021

Qard force-pushed the multi-tenant-promise-hook branch 3 times, most recently from 2e7497f to e60e621 Compare July 6, 2021 00:37