async_hooks: add using scopes to AsyncLocalStorage#61674
async_hooks: add using scopes to AsyncLocalStorage#61674Qard wants to merge 2 commits intonodejs:mainfrom
Conversation
af2ad3e to
d8e83aa
Compare
|
I guess this replaces #58104 right? |
|
Ah, yes. Forgot that one existed. 😅 |
8b875c4 to
09cb6ad
Compare
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/21678728231 |
Codecov Report✅ All modified and coverable lines are covered by tests. 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
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
| promise to the caller. At that point, the scope change becomes visible in the | ||
| caller's context and will persist in subsequent synchronous code until something | ||
| else changes the scope value. For async operations, prefer using `run()` which | ||
| properly isolates context across async boundaries. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. 🙈
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| with JavaScript's `using` syntax. | ||
|
|
||
| The scope automatically restores the previous store value when the `using` block | ||
| exits, whether through normal completion or by throwing an error. |
There was a problem hiding this comment.
What happens if someone forgets to use using with this? That also feels like a bit of a footgun here.
There was a problem hiding this comment.
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. 😐
There was a problem hiding this comment.
Yep. I'll be sure to raise this in the committee.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
https://github.com/tc39/proposal-async-context-disposable is the dedicated extension proposal.
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
Few issues to address still I think.
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.
Adds support for
using scope = storage.withScope(data)to do the equivalent of astorage.run(data, fn)with using syntax. This enables avoiding unnecessary closures.cc @nodejs/diagnostics