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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: add async_hooks migration note #45335

Closed
wants to merge 1 commit into from

Conversation

GeoffreyBooth
Copy link
Member

Implements nodejs/diagnostics#194 (comment), a docs addition informing users that the public async_hooks API will be going away at some unspecified future date; and some suggestions for other APIs to use instead. cc @Qard @mcollina @nodejs/diagnostics @nodejs/tsc

This doesn’t commit us to either a timeline or a process for async_hooks going internal, but I think merging in this PR can mark that we’ve decided on the goal we want to reach. Then we can discuss how we want to get there.

@GeoffreyBooth GeoffreyBooth added async_hooks Issues and PRs related to the async hooks subsystem. experimental Issues and PRs related to experimental features. diagnostics_channel Issues and PRs related to diagnostics channel labels Nov 6, 2022
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Nov 6, 2022
@Trott
Copy link
Member

Trott commented Nov 6, 2022

This feels like the domain deprecation. "We plan to get rid of this at some point (but actually...uh...we probably never will)." That's not a reason not to do it, but we should be aware that we have precedence for this and I'm not sure it's a precedent we don't want to break....

async_hooks was supposed to be the thing that let us get rid of domain. It's like that xkcd comic about standards.....

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Text LGTM if @nodejs/diagnostics agrees with the content.

@Qard
Copy link
Member

Qard commented Nov 6, 2022

Well, we should be able to convert domains to AsyncLocalStorage. Though making async_hooks internal doesn't mean we can't continue to use the parts of it we need for other features like domains. Just means we can stop exposing it externally and can break compatibility to make it better. Whereas right now it's extremely difficult to make any changes to async_hooks without covering a whole lot of edge cases. 😬

Should we also change the status at this point? The "legacy" status is a more accurate representation of the current state, and "deprecated" is where we want to get it.

@mcollina
Copy link
Member

mcollina commented Nov 6, 2022

Legacy is likely the correct status of this API.

@mcollina
Copy link
Member

mcollina commented Nov 6, 2022

Note that we are exposing AsyncLocalStorage under async_hooks, we might want to move it elsewhere or just flag createHook as legacy.

doc/api/async_hooks.md Outdated Show resolved Hide resolved
@GeoffreyBooth
Copy link
Member Author

Should we also change the status at this point? The "legacy" status is a more accurate representation of the current state, and "deprecated" is where we want to get it.

Arguably the current status is the best, as technically it can be removed at any time. I think legacy means that it won't be removed but it also won't be improved, but we do want to remove this (from the user's perspective). We could go straight to deprecated now, to start the clock, and maybe remove in 20.0.0; but I was trying just to reach consensus about the goal here, to leave the plan for achieving it as a follow up.

@GeoffreyBooth GeoffreyBooth added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Trott
Trott previously requested changes Nov 6, 2022
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

You can test with a make doc-only but I'm pretty sure adding stuff to the stability banner line will break things.

doc/api/async_hooks.md Outdated Show resolved Hide resolved
@@ -2,7 +2,10 @@

<!--introduced_in=v8.1.0-->

> Stability: 1 - Experimental
> Stability: 1 - Experimental. We plan to remove this API
Copy link
Member

Choose a reason for hiding this comment

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

Really? I'm not aware of any plan for removal.

Copy link
Member Author

Choose a reason for hiding this comment

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

That’s what this PR is meant to declare. We’re agreeing that we want to eventually remove it, and the specifics of how will be determined.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, in that cases I don't agree.
I see no benefit in removing something from public API surface which is need internally without providing a replacement. This only creates unneeded friction.
As I see no path to get a replacement as of now I see a similar topic here as in domains.

Copy link
Member Author

Choose a reason for hiding this comment

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

I see no benefit in removing something from public API surface which is need internally without providing a replacement.

async_hooks exposes internals. That’s why it shouldn’t remain a public API; but also why it’s fine to continue existing as an internal API.

The replacements are AsyncLocalStorage and Diagnostics Channel. If there are use cases currently served by async_hooks that are unachievable with those APIs, then those use cases are ones we should try to address before async_hooks is removed, either via AsyncLocalStorage and Diagnostics Channel or some other APIs. The point of this PR is to start that process: to encourage users depending on async_hooks to start migrating off of it, and report issues that they encounter so that collaborators can address them, by adding new capabilities to AsyncLocalStorage or Diagnostics Channel or other APIs that we consider stable or on a path toward stability.

> Stability: 1 - Experimental
> Stability: 1 - Experimental. We plan to remove this API
> in the future; it will never become stable.
> Please migrate to [`AsyncLocalStorage`][] and [Diagnostics Channel][] and
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some more info here for users? In special I'm not aware of any use case where diagnostics channel is a replacement for async hooks. Diagnostics channel is a very generic API - similar to EventEmitter and it servers a different purpose. The only connection is that APMs tend to use both - but for different things.
Therefore I don't think we should mix this here as it generates likely more confusion.

AsyncLocalStore is actually the replacement for the most popular async hooks usecase. Concrete the use case to pass transactional context across async boundaries. Therefore I would put more focus on this.

Copy link
Member Author

Choose a reason for hiding this comment

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

The Diagnostics Channel is a replacement for the higher level use cases that people often use async_hooks for: tracking network requests, for example, rather than specific promises. AsyncLocalStorage is the replacement low-level API, Diagnostics Channel is a replacement high-level API for many of the use cases that both low-level APIs are used for.

Copy link
Member

Choose a reason for hiding this comment

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

Tracking active http requests is one example of a use case previously solved by async_hooks which can much more simply and performantly be solved by diagnostics_channel. My intent is to add more channels in various places to cover many of the other needs which async_hooks currently serves. If there are remaining use cases which neither AsyncLocalStorage nor diagnostics_channel with additional channels can solve, we can define new APIs which are more purpose-built and don't leak internals so much that performance optimization becomes next to impossible. One of the greatest challenges with async_hooks is that it's virtually impossible to refactor without breaking the ecosystem, which makes it extremely difficult to improve the performance. With higher-level APIs we can gain more control over the exact behaviour of the internals enabling us to rearrange things in ways that will perform much better.

> Stability: 1 - Experimental. We plan to remove this API
> in the future; it will never become stable.
> Please migrate to [`AsyncLocalStorage`][] and [Diagnostics Channel][] and
> other stable APIs.
Copy link
Member

Choose a reason for hiding this comment

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

This is quite vague. I think we should either remove this or point to concreate APIs (which I'm not aware of).

Copy link
Member Author

Choose a reason for hiding this comment

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

I am pointing to specific APIs. I don’t think we need any more detail here. This is a quick note informing people that this API will go away. It’s not our responsibility to tell them how to rewrite their code.

Copy link
Member

Choose a reason for hiding this comment

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

@Flarna Would this be better?

Suggested change
> other stable APIs.
> in the future; it will never become stable. If possible,
> please use other APIs instead such as [`AsyncLocalStorage`][] or
> [Diagnostics Channel][].

@Qard
Copy link
Member

Qard commented Nov 6, 2022

We can't just remove it at any time though. We want to remove it, but there will need to be a long deprecation cycle to do so and there's still work to be done to ensure the core use cases are adequately covered. Legacy status seems most suitable--it may be removed eventually but that will take a long time and it may have to remain but become actively discouraged, which legacy communicates well.

It is a feature that is currently depended on extensively, so the "could change at any time" definition of experimental is kind of a lie.

@GeoffreyBooth
Copy link
Member Author

It is a feature that is currently depended on extensively, so the “could change at any time” definition of experimental is kind of a lie.

This PR isn’t changing the status. We can open a separate PR for that.

doc/api/async_hooks.md Outdated Show resolved Hide resolved
@Trott Trott dismissed their stale review November 6, 2022 20:32

build-breakage fixed

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

Given how much async_hooks is used everywhere we cannot safely remove it. https://www.npmjs.com/package/cls-hooked is downloaded more than one million times per week as of today.

I agree it's not the API we want long term, but that's the API we have now. The ecosystem usage of this API ask us to treat it like any other stable API we have.

I don't see us removing it anytime soon, and we'd be sending the wrong message out. I don't see us improving this API in any way either. Legacy seems a perfect match for this situation. We should definitely point folks to AsyncLocalStorage (diagnostic_channel is yet another experimental thing, it's likely better to hold off).

@Flarna
Copy link
Member

Flarna commented Nov 6, 2022

While I agree with the general move towards AsyncLocalStore I don't agree with the wording used in this PR and I doubt this will change based on commenting only.
Therefore I will create a PR during the next days as an alternative/addition to this one. I assume some sort of combination will finally succeed.

@GeoffreyBooth
Copy link
Member Author

I will create a PR during the next days as an alternative/addition to this one. I assume some sort of combination will finally succeed.

Do you want to just propose alternative language here as a comment?

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Nov 6, 2022

Given how much async_hooks is used everywhere we cannot safely remove it. npmjs.com/package/cls-hooked is downloaded more than one million times per week as of today.

I don’t see how this is relevant. cls-hooked can migrate to AsyncLocalStorage, just as it migrated from async_listener previously, and those million downloads per week will use the new API. Ditto with the instrumentation libraries that use async_hooks; those vendors could use a firm push to prioritize refactoring their code.

I agree it's not the API we want long term, but that's the API we have now. The ecosystem usage of this API ask us to treat it like any other stable API we have.

I don’t see us removing it anytime soon, and we’d be sending the wrong message out.

This API is not stable and will never be. It’s a borderline security risk in that it leaks internals without the user needing to pass --expose-internals; at best it’s a massive bug by design. We should be doing everything we can to encourage users to migrate off of it, not providing them the reassurance that it’ll never go away. The ecosystem usage will never decrease if we do nothing; it’s circular logic to argue that we can’t warn people off of an API because people are using the API. We may never actually carry out our threat of actually removing it, but we need to put that option firmly on the table to nudge people to get migrating.

@GeoffreyBooth GeoffreyBooth added tsc-agenda Issues and PRs to discuss during the meetings of the TSC. and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 6, 2022
@Flarna
Copy link
Member

Flarna commented Nov 6, 2022

I plan to touch a bit more in the async_hooks docs to adress the exposes internals part and to be a bit more concreate regarding when to use AsyncLocalStorage.
I'm not planning to go the "we will remove this" announcement as this is the part where I don't agree.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@anonrig anonrig left a comment

Choose a reason for hiding this comment

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

lgtm 🚀

doc/api/async_hooks.md Show resolved Hide resolved
doc/api/async_hooks.md Show resolved Hide resolved
@trevnorris
Copy link
Contributor

trevnorris commented Nov 16, 2022

Fortunately it seems like this API will never be removed. Personally I use this API extensively whenever I need to debug very complex asynchronous issues which AsyncLocalStorage can't cover. I'll be happy to open issues that cover my use cases, and maybe someone else can come up with an API that would better suit the problem scope.

The Diagnostics Channel doesn't offer a substitute because it requires every location you want to observe to be tooled first. I use async hooks to figure out where I need to put those channels so it makes debugging easier in the future.

@GeoffreyBooth
Copy link
Member Author

GeoffreyBooth commented Nov 16, 2022

Fortunately it seems like this API will never be removed.

I wouldn’t count on that. The point of asking for use cases that aren’t covered by the mentioned APIs is so that they can be potentially deprecated in the future.

@Trott Trott changed the title async_hooks: add migration note doc: add async_hooks migration note Nov 18, 2022
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

I think the discussion between @trevnorris and @Qard does need a resolution of some sort, but this is a doc-only change and we can easily update the text later, so I'm smashing Approve on this one for now.

@GeoffreyBooth GeoffreyBooth added the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 21, 2022
@GeoffreyBooth GeoffreyBooth dismissed jasnell’s stale review November 21, 2022 00:26

Approved per #45335 (comment): “+1 to @Qard’s suggested alternative”

@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 21, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in ea92ca1...80dcc06

nodejs-github-bot pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45335
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@GeoffreyBooth GeoffreyBooth deleted the async-hooks-note branch November 21, 2022 00:33
ruyadorno pushed a commit that referenced this pull request Nov 21, 2022
PR-URL: #45335
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Nov 23, 2022
PR-URL: nodejs#45335
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@ruyadorno ruyadorno mentioned this pull request Nov 24, 2022
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Add hints to migrate away from async hooks.
Change docs at various places to be more clear that resources are
internals and may change at any time.

PR-URL: #45369
Refs: #45335
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45335
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
Add hints to migrate away from async hooks.
Change docs at various places to be more clear that resources are
internals and may change at any time.

PR-URL: #45369
Refs: #45335
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
danielleadams pushed a commit that referenced this pull request Dec 30, 2022
PR-URL: #45335
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
Add hints to migrate away from async hooks.
Change docs at various places to be more clear that resources are
internals and may change at any time.

PR-URL: #45369
Refs: #45335
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 3, 2023
PR-URL: #45335
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
Add hints to migrate away from async hooks.
Change docs at various places to be more clear that resources are
internals and may change at any time.

PR-URL: #45369
Refs: #45335
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Stephen Belanger <[email protected]>
Reviewed-By: Chengzhong Wu <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 4, 2023
PR-URL: #45335
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
danielleadams pushed a commit that referenced this pull request Jan 5, 2023
PR-URL: #45335
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. diagnostics_channel Issues and PRs related to diagnostics channel doc Issues and PRs related to the documentations. experimental Issues and PRs related to experimental features.
Projects
None yet
Development

Successfully merging this pull request may close these issues.