async_hooks: add using scopes to AsyncLocalStorage

[Qard]

Adds support for using scope = storage.withScope(data) to do the equivalent of a storage.run(data, fn) with using syntax. This enables avoiding unnecessary closures.

cc @nodejs/diagnostics

[Flarna]

I guess this replaces #58104 right?

[Qard]

Ah, yes. Forgot that one existed. 😅

[github-actions]

Failed to start CI

   ⚠  No approving reviews found
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/21678728231

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.76%. Comparing base (5e818c9) to head (de68dc5).
⚠️ Report is 73 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61674      +/-   ##
==========================================
+ Coverage   89.74%   89.76%   +0.02%     
==========================================
  Files         674      676       +2     
  Lines      204389   204681     +292     
  Branches    39280    39327      +47     
==========================================
+ Hits       183424   183742     +318     
+ Misses      13264    13239      -25     
+ Partials     7701     7700       -1     

Files with missing lines	Coverage Δ
...nternal/async_local_storage/async_context_frame.js	100.00% <100.00%> (ø)
lib/internal/async_local_storage/async_hooks.js	98.03% <100.00%> (+0.08%)	⬆️
lib/internal/async_local_storage/run_scope.js	100.00% <100.00%> (ø)

... and 61 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71216/

[bengl]

I'll happily close #58104 in favour of this one.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/71326/

[jasnell]

This makes me quite nervous as it seems very easy for users to get incorrect. In cases where the async context is propagating session or tracing details specific to a particular scope, this feels like a signficant footgun

[Qard]

I'm a bit less worried about it as it operates per-store rather than globally across all stores, so you're likely only ever going to run into it with your own code. Agreed that it has non-zero risk though.

I'm mainly adding this to enable optimizations to TracingChannel and runStores in diagnostics_channel to eliminate a bunch of closures (#61680) so could also consider just not documenting it if we're concerned about users using it wrong.

Could also pursue adding some more hooks to V8 to mark the boundaries of that segment of code and give us something to reset the context around. I've already had a bunch of prior conversations about this problem in the AsyncContext proposal channels--this part of the spec is just packed full of these landmines and it'd be really nice to have a way around the strange behaviour there. 🙈

[jasnell]

Yeah, it's super tricky and there might not be anything we can reasonably do about the footguns except to document the hell out of them and hope for the best. At the very least, I'd say at the very least let's be sure to keep this experimental for a bit.

[Qard]

Yep, would definitely need to remain experimental until we're sure about it. And my feeling is that if we're not sure about it I would probably want to just go make a V8 change to make it possible to fix that async function prelude issue at least--that'd also unblock a more sync-feeling set/get model for ALS that I have been wanting to try for a while.

[jasnell]

What happens if someone forgets to use using with this? That also feels like a bit of a footgun here.

[Qard]

Yes, that's a definite weakness of the resource management spec. I really wish they had some method of detecting if the expression being evaluated is targeting a using declaration and be able to throw an error if not. In my opinion the ability to create a using-based resource without actually using it with the syntax was a mistake. 😐

[jasnell]

Yep. I'll be sure to raise this in the committee.

[rbuckton]

This has been discussed several times in committee. In general, we do not force you to use syntax in other cases, such as using await with a Promise-returning function, as there are perfectly reasonable situations where you don't want to use await (or using). Even if we were to introduce a mechanism to enforce using, ideally there would be some way to opt out.

There are two proposed solutions to this. One uses the current proposal as-is and suggests that [Symbol.dispose] be a getter that sets a flag on read before returning the disposer. That flag is then checked when performing any operation against the resource. In this scenario, the expectation is that a resource can be used with using or with DisposableStack.prototype.use as both will immediately read the [Symbol.dispose] property. In addition, a developer could also imperatively read the [Symbol.dispose] property themselves for any kind of manual resource tracking.

The second proposed solution would be to introduce a [Symbol.enter] method that must be invoked to unwrap the underlying resource to be disposed. In this proposal the object returned from something like storage.withScope might not be directly usable by the consumer and instead contains a [Symbol.enter]() method that is invoked by a statement like using to acquire the actual resource. As such, developers would still be able to imperatively invoke [Symbol.enter]() if they so choose as well as leverage composition mechanisms like DisposableStack. The intent is that the added complication would be to guide users towards using as the simplest path, without preventing advanced use cases.

I'm generally not in favor of a magical behavior that enforces a syntactic usage at the callee site, as it makes it much more complicated for users to reason over whether a resource is composable and makes the whole mechanism seem very fragile.

[rbuckton]

The main upside to the get [Symbol.dispose]() (getter) approach is that it can be leveraged immediately without depending on the advancement of a second proposal, with the obvious downside that it requires more internal plumbing.

Users could also leverage a type-aware linter to detect and flag untracked resources.

[Qard]

Yes, I suggested a using.target so the creator of the usable type could opt to throw when it's not set, but it would not be mandatory to use it. I can understand why folks might be averse to syntax like that though.

I'd be happy with a [Symbol.enter] too, even if that is technically exposed to users, as it looks obvious enough that one should not be using it unless they know what they're doing. As it is presently, it's too easy to misuse ERM between forgetting the using and getting half-applied logic or its odd interactions with async function preludes.

[jasnell]

Per the recommendations in https://github.com/nodejs/node/blob/main/doc/contributing/erm-guidelines.md#guidelines-for-disposable-objects, "Disposable objects should expose named disposal methods in addition to the Symbol.dispose..."

Adding a close() method in here that SymbolDispose defers to would be good.

[Qard]

Ah, hadn't seen that. I actually explicit went for not having an explicit close method as that felt like its own sort of footgun. For example:

const thing = shouldHaveBeenAUsing()

if (someCondition) {
  thing.close()
}

// Whoops, forgot to properly dispose of the resource.

I can add the explicit close method though if that is the preferred design.

[jasnell]

Feel free also to push back on the recommendation if you feel there's a strong enough motivation for doing so. It's a guideline not a hard rule.

[Qard]

I wouldn't say I'm against it, but I definitely feel it's a problem of having no good options.

Like I mentioned in the other thread, if we had a way to throw if constructing a usable-conforming type when not in a using-targeted expression that'd basically resolve all the safety issues, something like new.target but for detecting if it was in a using declaration.

[jasnell]

I'll leave this up to your judgement then and won't block on it.

[Qard]

I opened tc39/proposal-explicit-resource-management#271 to suggest the idea.

[Flarna]

FWIW We had a discussion that time during guideline creation and that time als.use() was also the main sample to not enforce explict dispose method or recommend to explicit dispose.

[jasnell]

The test needs to also cover the case where withScope is used without the using keyword. I assume it will mean the scope leaks but it's non-obvious.

[jasnell]

Few issues to address still I think.