Proposal: mark AsyncResource and AsyncLocalStorage as stable

[Qard]

The AsyncResource API has been largely unchanged since its creation many majors ago, and AsyncLocalStorage has been out in the wild for awhile now and has proven its utility. I think both should be considered for stable status at this point. The challenge I see is that they live in the async_hooks module which has been marked as "experimental" at the top-level because the async_hooks feature itself exposes internals thus many (myself included) were unwilling to accept it as a "stable" API.

What I suggest is that AsyncResource and AsyncLocalStorage be considered as stable in the next major. Due to the module being named after the one not stable feature, it may also be worth moving the two APIs out to new top-level modules, but I'll leave that last point for discussion.

How should we proceed in getting these two APIs to stable status?

cc @nodejs/diagnostics

[mmarchini]

cc @nodejs/tsc for awareness

[bmeck]

I'm extremely not comfortable with enterWith or exit being considered non-experimental since they are an action at a distance, but the rest seems possible to me at least. If concrete and necessary use cases in the wild exist for those methods beyond being easier to consume than run I'd be open to conversation about why they are needed but I'm still not seeing wide adoption that forces them to exist. Those APIs are within the realm of what caused problems with domains in the post mortem and I'd be very hard pressed to understand why they are not the same danger.

[Qard]

Domains were global, so enter and exit applied globally. That meant calling either method could totally mess up some other components sense of what the context was. However, each instance of AsyncLocalStorage is separate, so you'd only ever get enterWith or exit called by the owner of that storage, thus they'd have more of a sense of what the "correct" thing to do is in their specific case. Messing with the context of an AsyncLocalStorage instance only impacts that instance, so I don't see how any of the issues with domains would emerge.

If people are really so concerned about those specific parts of the API though, I'm happy to consider marking the rest of the API stable and just leaving those specific parts as experimental. Also, I'm less sure about the stability of AsyncLocalStorage than AsyncResource, given its relative newness in comparison, though I'm fairly confident at this point there are no significant issues remaining.

[bmeck]

@Qard The problem is just like how domain allows non-local and non-scoped manipulation of context in non-async boundaries. Your argument that only a "owner" will manipulate assumes that no-shared async storage would require that consumers have a specific coding style (in particular, not exposing a direct reference to an AsyncLocalStorage instance) which I don't find compelling. The docs show a local storage that is shared across multiple locations for example.

[Qard]

The docs show an AsyncLocalStorage instance shared within the same user code, which is the intent. Sharing a context object with other modules or other peoples code in general is a bad idea and not encouraged.

[bmeck]

@Qard

Sharing a context object with other modules or other peoples code in general is a bad idea and not encouraged.

Agreed, but the concerns of sharing it with only .run are much different from those 2 methods, hence me not wanting to move those methods out of experimental. The .run method cannot alter non-locally. We can wait on more usage before coming back to those.

[Qard]

The enterWith() method is needed for APM vendors to be able to achieve patch-free instrumentation with the combination of AsyncLocalStorage and the work I'm currently doing on diagnostics_channel.

With diagnostics_channel, http servers can publish a message indicating the start of a connection, which a subscriber can then do als.enterWith(context) to bind the entire request to the context. If there's only als.run(...) then they would need to patch http servers to intercept the request handler, which is fragile and can negatively impact performance. There's also many ways a request handler can be added, and they could be removed and added again any time in the future, due to servers just being an event emitter. This makes the code to correctly patch an http server actually fairly complicated, whereas with the diagnostics_channel message the handler itself becomes irrelevant.

Personally, I disagree that there is any issue with enterWith() or exit(). If you're still concerned about it, perhaps we should add more explicit warnings to the docs about sharing AsyncLocalStorage contexts? Regardless of those specific functions, sharing a context is just a bad idea because it gives you a single object space that can be overridden at any time. The danger of some downstream code calling enterWith() is no more dangerous than something calling run() and messing up the context you expect to have in a callback. Sharing a context object between consumers is just universally a bad idea and this specific problem with domains is exactly why AsyncLocalStorage was created as a non-global system, allowing each user that wants to propagate context to have their own isolated object to do it with.

I entirely disagree that this is a repeat of the domains issues. Domains were problematic because they all shared one context globally--you can't be in more than one domain at a time. That's not the case here, each context is isolated.

[bmeck]

@Qard I do not agree with that last comment, hence me not wanting to move those methods out of experimental. We can have this discussion here if necessary but it likely would be more useful to have it elsewhere. Perhaps an explanation of why that specific API design which does have non-local affects is necessary would be more helpful. I'm still unclear on why APM is unable to use .run. I will repeat that I am extremely uncomfortable with those APIs and having convenience in usage is not my priority in allowing those to move forward to stable. The rest of the APIs seem fine to me.

[AndreasMadsen]

+1 on marking AsyncResource as stable. It being experimental is the primary reason module authors don't want to use it. Other parts of async_hooks still needs work, but I don't see it interact with the AsyncResource API.

I would suggest adding a documentation section, regarding what node.js would in the future, when the other parts are stable, consider a major/minor/patch change to the async context graph. This would serve as a default guideline for module authors when they change the async context graph.

PS: Be sure to also mark the C++ AsyncResource API stable as well.

[mcollina]

I would prefer to split those two things:

1. mark AsyncResource stable
2. keep AsyncLocalStorage experimental for some more time

[mhdawson]

My initial thinking is along the same lines as @mcollina with leaving AsyncLocalStorage in experimental for a bit longer.