| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
This provides a way to use the `using` syntax (when available) to manage AsyncLocalStorage contexts, as an alternative to `run()`.
Codecov Report❌ Patch coverage is 87.75510% with 6 lines in your changes missing coverage. Please review.
@@ Coverage Diff @@
## main #58104 +/- ##
=======================================
Coverage 90.20% 90.21%
=======================================
Files 630 630
Lines 186446 186495 +49
Branches 36622 36629 +7
=======================================
+ Hits 168189 168246 +57
- Misses 11051 11058 +7
+ Partials 7206 7191 -15
... and 28 files with indirect coverage changes 🚀 New features to boost your workflow:
|
Sorry, something went wrong.
Failed to start CI⚠ No approving reviews found ✘ Refusing to run CI on potentially unsafe PRhttps://github.com/nodejs/node/actions/runs/14782552537 |
Sorry, something went wrong.
There was a problem hiding this comment.
Like #52065, i think this does not work with an async function before the first await that the value will leak out the async function scope. This will need a patch to make V8 CPED aware of async function scopes as well.
async function foo() {
using value = als.use(newValue);
await 0;
}
foo() // no await
al's.getStore() // value changed.
Sorry, something went wrong.
| <!-- YAML | ||
| added: REPLACEME | ||
| --> | ||
|
|
There was a problem hiding this comment.
Is it intended to be added as stable ?
Otherwise add > Stability: 1 - Experimental here.
Sorry, something went wrong.
| * `store` {any} | ||
| * Returns: {Disposable} A disposable object. | ||
|
|
||
| Transitions into the given context, and transitions back into the previous |
There was a problem hiding this comment.
I think it's not the previous context. It's that one seen at the time use() was called.
If use and the matching dispose are called in right order this is the same but if they are called in different order it's not.
e.g. a cast like this (condensed, in real world this would be distributed at independent locations):
const { AsyncLocalStorage } = require("node:async_hooks");
const als1 = new AsyncLocalStorage();
const als2 = new AsyncLocalStorage();
const d1 = als1.use(store1);
const d2 = als2.use(store2);
const d3 = als1.use(store3);
als1.getStore(); // Returns store3
als2.getStore(); // Returns store2
d1[Symbol.dispose]();
als1.getStore(); // Returns undefined
als2.getStore(); // Returns undefined
d2[Symbol.dispose]();
als1.getStore(); // Returns store1
als2.getStore(); // Returns undefined
d3[Symbol.dispose]();
als1.getStore(); // Returns store1
als2.getStore(); // Returns store2Likely a user problem and once using is a thing harder to do.
But I think it should be clear documented what happens or avoid such cases.
In special that one ALS user might effect others is critical in my opinion.
Note that only the AsyncContextFrame variant shows side effects to all ALS users.
Sorry, something went wrong.
There was a problem hiding this comment.
I think if folks are manually calling Symbol.dispose they're rather on their own at that point. It's a bit unfortunate that the ERM model gives them that ability but there's only do much we can do.
Sorry, something went wrong.
There was a problem hiding this comment.
That said, we'll want to be sure that this works as expected when using DisposableStack when that is available.
Sorry, something went wrong.
There was a problem hiding this comment.
I fully agree once using keyword is a thing. But until this is true users have to do it manually.
Sorry, something went wrong.
There was a problem hiding this comment.
I notices that the side effect to other ALS instances is not bound to wrong usage of dispose:
const d1 = als1.use("store1");
als2.enterWith("store2");
console.log(als1.getStore()); // store1
console.log(als2.getStore()); // store2
d1[Symbol.dispose]();
console.log(als1.getStore()); // undefined
console.log(als2.getStore()); // undefined, but should be store2
Sorry, something went wrong.
There was a problem hiding this comment.
I noticed that this global side effect is not new in the AsyncContextFrame variant. See #58149.
Sorry, something went wrong.
There was a problem hiding this comment.
use() is intended to roughly mimic run(), and since that's been changed in #58149 to isolate the storage instances, I'll do that here as well in the next rev.
Sorry, something went wrong.
| #oldFrame = undefined; | ||
|
|
||
| constructor(store, storage) { | ||
| this.#oldFrame = AsyncContextFrame.current(); |
There was a problem hiding this comment.
I guess by capturing/restoring only the owned store instead the complete frame the side effects to other ALS users could be avoided.
Sorry, something went wrong.
There was a problem hiding this comment.
Agreed, an operation on an AsyncLocalStorage should not affect other instances. For reference: tc39/proposal-async-context-disposable#2
Sorry, something went wrong.
There was a problem hiding this comment.
Should be marked as experimental but otherwise lgtm
Sorry, something went wrong.
| function inner() { | ||
| // Once `using` syntax is supported, you can use that here, and omit the | ||
| // dispose call at the end of this function. | ||
| const disposable = asyncLocalStorage.use(store); |
There was a problem hiding this comment.
using works meanwhile on main (see #58154) therefore this should be updated accordingly.
Sorry, something went wrong.
There was a problem hiding this comment.
🫶 this
Sorry, something went wrong.
Do you see that as a blocker? Or would it just be fine to document this limitation for now? |
Sorry, something went wrong.
I would not see it as a blocker on this particular PR. It would be great to have this PR landed with this API marked as an experimental (1.1 Active development) API and with the limitation documented as a caveat. I think to promote this to stable, we'll need this semantic as a blocker. |
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
There was a problem hiding this comment.
as there are already approvals but there are still important todos I clearly mark it so to avoid accidental merging.
Sorry, something went wrong.
|
I never got around to merging this but I have a fix for the CPED issue: https://chromium-review.googlesource.com/c/v8/v8/+/6020635 |
Sorry, something went wrong.
|
@devsnek do you think you could find some time to pick it up again and finish that? :) |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This provides a way to use the using syntax (when available) to manage AsyncLocalStorage contexts, as an alternative to run().