Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: multi-tenant promise hook api #39283

Merged
merged 1 commit into from Nov 1, 2021

Conversation

@Qard
Copy link
Member

@Qard Qard commented Jul 6, 2021

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. 馃樃

@Qard Qard force-pushed the multi-tenant-promise-hook branch 3 times, most recently from 2e7497f to e60e621 Jul 6, 2021
@targos
Copy link
Member

@targos targos commented Jul 7, 2021

Loading

test/parallel/test-promise-hook-create-hook.js Outdated Show resolved Hide resolved
Loading
doc/api/promise_hooks.md Outdated Show resolved Hide resolved
Loading
doc/api/promise_hooks.md Outdated Show resolved Hide resolved
Loading
doc/api/promise_hooks.md Outdated Show resolved Hide resolved
Loading
Copy link
Member

@AndreasMadsen AndreasMadsen left a comment

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.

Loading

doc/api/promise_hooks.md Outdated Show resolved Hide resolved
Loading
doc/api/index.md Outdated Show resolved Hide resolved
Loading
if (after) ArrayPrototypePush(hooks, onAfter(after));
if (resolve) ArrayPrototypePush(hooks, onResolve(resolve));

return () => {
Copy link
Member

@AndreasMadsen AndreasMadsen Jul 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Loading

Copy link
Member Author

@Qard Qard Jul 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 馃し

Loading

Copy link
Member

@AndreasMadsen AndreasMadsen Jul 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@Qard Qard Jul 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 馃し鈥嶁檪锔

Loading

Copy link
Member

@AndreasMadsen AndreasMadsen Jul 9, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@Qard Qard Jul 21, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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鈥檛 be tampered with. If people feel strongly that it should be object-based though, I'm happy to reconsider.

Loading

Copy link
Member Author

@Qard Qard Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member

@jasnell jasnell Aug 28, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

Copy link
Member Author

@Qard Qard Oct 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Loading

@Qard
Copy link
Member Author

@Qard Qard commented Jul 8, 2021

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.

Loading

@Qard Qard force-pushed the multi-tenant-promise-hook branch from e60e621 to e6d77f3 Jul 8, 2021
@AndreasMadsen
Copy link
Member

@AndreasMadsen AndreasMadsen commented Jul 8, 2021

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.

Loading

@Qard
Copy link
Member Author

@Qard Qard commented Jul 9, 2021

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! 馃槃

Loading

@Qard Qard force-pushed the multi-tenant-promise-hook branch from e6d77f3 to 54b9ce1 Jul 20, 2021
@Qard
Copy link
Member Author

@Qard Qard commented Jul 20, 2021

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.

Loading

@Qard Qard marked this pull request as ready for review Jul 20, 2021
@nodejs-github-bot

This comment has been hidden.

@Qard Qard force-pushed the multi-tenant-promise-hook branch 3 times, most recently from 4efda13 to 80855c5 Jul 21, 2021
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Oct 25, 2021

Loading

@nodejs-github-bot

This comment has been hidden.

@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Oct 27, 2021

Loading

@Flarna
Copy link
Member

@Flarna Flarna commented Oct 27, 2021

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

Loading

@Qard
Copy link
Member Author

@Qard Qard commented Oct 27, 2021

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

Loading

@Flarna
Copy link
Member

@Flarna Flarna commented Oct 27, 2021

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

Loading

@Qard
Copy link
Member Author

@Qard Qard commented Oct 27, 2021

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. 馃憤

Loading

@Flarna
Copy link
Member

@Flarna Flarna commented Oct 28, 2021

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

Loading

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 Trott force-pushed the multi-tenant-promise-hook branch from 926ea9f to da82844 Nov 1, 2021
@Trott Trott merged commit da82844 into nodejs:master Nov 1, 2021
@Trott
Copy link
Member

@Trott Trott commented Nov 1, 2021

Landed in da82844

Loading

@Qard Qard deleted the multi-tenant-promise-hook branch Nov 2, 2021
targos added a commit that referenced this issue Nov 6, 2021
PR-URL: #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>
targos added a commit that referenced this issue Nov 8, 2021
Notable changes:

doc:
  * add VoltrexMaster to collaborators (voltrexmaster) #40566
esm:
  * (SEMVER-MINOR) add support for JSON import assertion (Antoine du Hamel) #40250
lib:
  * (SEMVER-MINOR) add unsubscribe method to non-active DC channels (simon-id) #40433
  * (SEMVER-MINOR) add return value for DC channel.unsubscribe (simon-id) #40433
v8:
  * (SEMVER-MINOR) multi-tenant promise hook api (Stephen Belanger) #39283

PR-URL: TODO
targos added a commit that referenced this issue Nov 8, 2021
Notable changes:

doc:
  * add VoltrexMaster to collaborators (voltrexmaster) #40566
esm:
  * (SEMVER-MINOR) add support for JSON import assertion (Antoine du Hamel) #40250
lib:
  * (SEMVER-MINOR) add unsubscribe method to non-active DC channels (simon-id) #40433
  * (SEMVER-MINOR) add return value for DC channel.unsubscribe (simon-id) #40433
v8:
  * (SEMVER-MINOR) multi-tenant promise hook api (Stephen Belanger) #39283

PR-URL: #40758
@targos targos mentioned this pull request Nov 8, 2021
targos added a commit that referenced this issue Nov 9, 2021
Notable changes:

doc:
  * add VoltrexMaster to collaborators (voltrexmaster) #40566
esm:
  * (SEMVER-MINOR) add support for JSON import assertion (Antoine du Hamel) #40250
lib:
  * (SEMVER-MINOR) add unsubscribe method to non-active DC channels (simon-id) #40433
  * (SEMVER-MINOR) add return value for DC channel.unsubscribe (simon-id) #40433
v8:
  * (SEMVER-MINOR) multi-tenant promise hook api (Stephen Belanger) #39283

PR-URL: #40758
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet