doc: add async_hooks migration note - #45335
Conversation
Sorry, something went wrong.
|
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..... |
Sorry, something went wrong.
Trott
left a comment
There was a problem hiding this comment.
Text LGTM if @nodejs/diagnostics agrees with the content.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Legacy is likely the correct status of this API. |
Sorry, something went wrong.
|
Note that we are exposing AsyncLocalStorage under async_hooks, we might want to move it elsewhere or just flag createHook as legacy. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Sorry, something went wrong.
Trott
left a comment
There was a problem hiding this comment.
You can test with a make doc-only but I'm pretty sure adding stuff to the stability banner line will break things.
Sorry, something went wrong.
There was a problem hiding this comment.
Really? I'm not aware of any plan for removal.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
This is quite vague. I think we should either remove this or point to concreate APIs (which I'm not aware of).
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
@Flarna Would this be better?
| > other stable APIs. | |
| > in the future; it will never become stable. If possible, | |
| > please use other APIs instead such as [`AsyncLocalStorage`][] or | |
| > [Diagnostics Channel][]. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
This PR isn’t changing the status. We can open a separate PR for that. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
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).
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
Do you want to just propose alternative language here as a comment? |
Sorry, something went wrong.
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.
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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
mcollina
left a comment
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
anonrig
left a comment
There was a problem hiding this comment.
lgtm 🚀
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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. |
Sorry, something went wrong.
Trott
left a comment
There was a problem hiding this comment.
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.
Sorry, something went wrong.
Approved per #45335 (comment): “+1 to @Qard’s suggested alternative”
PR-URL: #45335 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: #45335 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <webadmin@geoffreybooth.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: #45335 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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 <webadmin@geoffreybooth.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Stephen Belanger <admin@stephenbelanger.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
PR-URL: #45335 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
PR-URL: #45335 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com>
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.