doc: add async_hooks migration note #45335
Conversation
GeoffreyBooth
added
async_hooks
|
This feels like the
|
|
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. |
|
Legacy is likely the correct status of this API. |
|
Note that we are exposing AsyncLocalStorage under async_hooks, we might want to move it elsewhere or just flag createHook as legacy. |
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
added
the
author ready
doc/api/async_hooks.md
Outdated
| @@ -2,7 +2,10 @@ | |||
|
|
|||
| <!--introduced_in=v8.1.0--> | |||
|
|
|||
| > Stability: 1 - Experimental | |||
| > Stability: 1 - Experimental. We plan to remove this API | |||
There was a problem hiding this comment.
Really? I'm not aware of any plan for removal.
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.
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.
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.
doc/api/async_hooks.md
Outdated
| > 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 |
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.
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.
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.
doc/api/async_hooks.md
Outdated
| > 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. |
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).
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.
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][]. |
|
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. |
This PR isn’t changing the status. We can open a separate PR for that. |
Trott
force-pushed
the
async-hooks-note
branch
from
279632e
to
b4d0dbd
Compare
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).
|
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. |
Do you want to just propose alternative language here as a comment? |
I don’t see how this is relevant.
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 |
GeoffreyBooth
added
tsc-agenda
|
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. |
Trott
force-pushed
the
async-hooks-note
branch
from
5f961a2
to
3762422
Compare
|
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. |
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. |
GeoffreyBooth
force-pushed
the
async-hooks-note
branch
from
81b3c38
to
65a6795
Compare
Trott
changed the title
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.
GeoffreyBooth
added
the
commit-queue
GeoffreyBooth
dismissed
jasnell’s stale review
Approved per #45335 (comment): “+1 to @Qard’s suggested alternative”
nodejs-github-bot
removed
the
commit-queue
|
Landed in ea92ca1...80dcc06 |
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>
PR-URL: nodejs#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>
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_hooksAPI will be going away at some unspecified future date; and some suggestions for other APIs to use instead. cc @Qard @mcollina @nodejs/diagnostics @nodejs/tscThis doesn’t commit us to either a timeline or a process for
async_hooksgoing 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.