async_hooks: add SymbolDispose support to AsyncLocalStorage#52065
Closed
legendecas wants to merge 3 commits intonodejs:mainfrom
Closed
async_hooks: add SymbolDispose support to AsyncLocalStorage#52065legendecas wants to merge 3 commits intonodejs:mainfrom
legendecas wants to merge 3 commits intonodejs:mainfrom
Conversation
Collaborator
|
Review requested:
|
Use a shared object as the top level async resource in JS and C++ so that when there is no active async resource, the same top level async resource object is returned from `async_hooks.executionAsyncResource()`.
56fff75 to
1f997de
Compare
1f997de to
5a98409
Compare
mcollina
reviewed
Mar 13, 2024
This introduces explicit resource management support to
AsyncLocalStorage. In order to avoid unexpected leakage of an async
local storage value to outer scope, `asyncLocalStorage.run` requires a
callback to nest all sub-procedures in a new function scope. However,
when using `AsyncLocalStorage` with different function types, like
async generator, moving statements and expressions to a new function
body could be evaluated in different ways.
For example, given an async generator:
```js
class C {
async* foo() {
await a();
yield b();
await c();
}
}
```
Then, if we want to modify the async local storage value for `b()` and
`c()`, simply moving statements would not work:
```js
const storage = new AsyncLocalStorage();
class C {
async* foo() {
await a();
storage.run(value, () => {
yield b(); // syntax error, arrow function is not a generator
await this.c(); // syntax error, arrow function is not async
});
}
}
```
Making the arrow function to be an async generator as well still
requires significant refactoring:
```js
const storage = new AsyncLocalStorage();
class C {
async* foo() {
await a();
yield* storage.run(value, async function*() => {
yield b(); // ok
await this.c(); // reference error, this is undefined
});
}
}
```
This could be potentially more convenient with `using` declarations:
```js
const storage = new AsyncLocalStorage();
class C {
async* foo() {
await a();
using _ = storage.disposableStore(value);
yield b();
await this.c();
}
}
```
However, creating a disposable without a `using` declaration could
still be a problem leading to leakage of the async local storage
value. To help identifying such mis-use, an
`ERR_ASYNC_LOCAL_STORAGE_NOT_DISPOSED` error is thrown if an
`AsyncLocalStorageDisposableStore` instance is not disposed at the end
of the synchronous execution.
5a98409 to
9099911
Compare
Qard
reviewed
Mar 13, 2024
Member
Qard
left a comment
There was a problem hiding this comment.
I would prefer if we avoid further entangling AsyncLocalStorage with async_hooks, which I'm presently trying to undo. Can we instead make each constructed AsyncLocalStorageDisposableStore just queue a microtask to check if it is disposed yet by the time the microtask is run and skip the use of createHook entirely?
Contributor
|
This needs a rebase. |
Member
Author
|
Closed in favor of #58104. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This introduces explicit resource management support to AsyncLocalStorage. In order to avoid unexpected leakage of an async local storage value to outer scope,
asyncLocalStorage.runrequires a callback to nest all sub-procedures in a new function scope. However, when usingAsyncLocalStoragewith different function types, like async generator, moving statements and expression to a new function body could be evaluated in different ways.For example, given an async generator:
Then, if we want to modify the async local storage value for
b()andc(), simply moving statements would not work:Making the arrow function to be an async generator as well still requires significant refactoring:
This could be potentially more convenient with
usingdeclarations:However, creating a disposable without a
usingdeclaration could still be a problem leading to leakage of the async local storage value. To help identifying such mis-use, anERR_ASYNC_LOCAL_STORAGE_NOT_DISPOSEDerror is thrown if anAsyncLocalStorageDisposableStoreinstance is not disposed at the end of the current async resource scope.