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.68%. Comparing base (06a8240) to head (b055801).
⚠️ Report is 48 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #61674   +/-   ##
=======================================
  Coverage   89.68%   89.68%           
=======================================
  Files         676      677    +1     
  Lines      206578   206621   +43     
  Branches    39555    39567   +12     
=======================================
+ Hits       185267   185308   +41     
- Misses      13446    13447    +1     
- Partials     7865     7866    +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 28 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.

[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.

[legendecas]

https://github.com/tc39/proposal-async-context-disposable is the dedicated extension proposal.

[Qard]

So as this is presently, forgetting using or a manual dispose will result in the context persisting until it would next change, which would be basically any async boundary so I'm not too worried about leaking anywhere unexpected. All the task loop stuff at the end of an I/O tick will get their own contexts before execution, so there's really not that much context would persist to that is unexpected beyond just the rest of the sync execution of the current tick. Event handlers run in the same tick could leak context to each other, but I would argue that should be expected when not exiting the context. 🤷🏻

[jasnell]

hmm... I'm still not entirely sold on the approach but not strongly enough to block. There's a fair amount of risk in the edge cases here tho... enough that I'd definitely like to see this stay experimental for a while.

[Flarna]

I agree that this is not really nice. But we have already enterWith() which has the same problems and even more but a lot people say that without it ALS is useless for them.

[jasnell]

Few issues to address still I think.

[Qard]

What are folks' opinions on public versus internal for this? The primary motivation for this is just to eliminate all the closure nesting in channel.runStores(...). We could opt for landing this undocumented, as an internal-only tool, if we're concerned about the possible safety issues of using ERM wrong.

[nodejs-github-bot]

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

[Flarna]

I see no benefit with internal only. enterWith is exposed already which allows to leak context already now.

[nodejs-github-bot]

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

[nodejs-github-bot]

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