GeezerVison

@mscdex This was talked about at the diagnostics summit. I don’t think the feature has to exist in this particular form, but having it in core made sense in general, and in particular performance-wise, since a good implementation could allow us to skip calling async_hooks callbacks.

FYI @nodejs/tsc

I think this functionality belongs in core. We should offer a single way to do cls that is high performant, stable and reliable. It’s not an ecosystem concern, it’s a runtime concern (as much as a ThreadLocal implementation is for Java/.NET/.. etc).

I agree that this should be in core and there was consensus among the @nodejs/diagnostics WG members attending the diagnostics summit. Sure, we can bikeshed around the specific API, and I would like to get some of the APM vendors to review this before it lands, it would be generally better to have a single authoritative API and implementation of this than multiple userland implementations so that APM solutions built on top can be implemented consistently.

Should we create npm placeholder for this name as per our guide?

Sorry, I am not experienced in registering placeholders in npm, so anybody more confident please do it.

There is https://www.npmjs.com/package/async-storage, which makes calling this module async_storage not a great idea. @nodejs/async_storage is ok.

Does this require a separate new top level module? Could this be exposed alternatively through the existing async_hooks module? Or via util?

@jasnell I actually think it would make sense to have that in util too. I am just worried potential future features would make this API big enough for us to regret it was not a module. That said, I'd be happy to move this to another module.

I will update the PR over the week end and start to contact APM and RASP vendors to ensure thay can comment.

Thanks a lot to all of you for the comments and suggestions!

if this is going to be in core I'd want it to be more similar to https://github.com/WICG/kv-storage

@vdeturckheim ... One thing to consider is that by putting this into a new module, it automatically becomes semver-major and significantly harder to backport. My hope is that we could at least get this into 10.x also.

@jasnell good point, I'll move it to util then (this is a killer argument for me indeed ;) )

@devsnek I did not know about this proposal. I'd be interested in other opinions here but the solutions I could see would probably be a bit slow.

@vdeturckheim reading over this again it seems i was confused as to what this pr is adding to node.

i think it might lend itself better to a different name like "async_closure" or "async_context" something.

i'd be strongly in favor of making the class available as an async_hooks.Context

I think it would be better placed in async_hooks instead of util.

As a developer and co-maintainer of continuation-local-storage, I’m very happy to see this functionality coming to Node core, and am amused to see how positive the reception is this time. A lot can change in 6 years. If anyone wants to reuse the name, or join this with the existing continuation-local-storage, I’m happy to hand the name over to the Foundation, although I understand that this implementation is simpler to use than CLS.

Any chance this will be backported to v12?

Any chance this will be backported to v12?

It would be great to have AsyncLocalStorage in v12, so I'm all for backporting it.

Note: executionAsyncResource (#30959) has to be backported first.

cc @Qard @vdeturckheim

As far as I know, backporting executionAsyncResource to v12 should not be a problem, but I haven't tested it myself.

Care must be taken to include also the followup PRs fixing various bugs found afterwards.

I've created a backport PR for executionAsyncResource: #32131. Hopefully, it includes all bugfixes.

@puzpuzpuz thanks! this next release is a patch so it'll go out in the subsequent minor.

v12 backport: #32318

Providing a reference to another issue here because most interested parties are subscribed to this issue.