Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

async_hooks: add SymbolDispose support to AsyncLocalStorage #52065

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented 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 expression to a new function body could be evaluated in different ways.

For example, given an async generator:

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:

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:

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:

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 current async resource scope.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Mar 13, 2024

Review requested:

  • @nodejs/realm
  • @nodejs/async_hooks
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Mar 13, 2024
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()`.
@legendecas legendecas force-pushed the async_hooks/disposable branch 2 times, most recently from 56fff75 to 1f997de Compare March 13, 2024 08:56
@legendecas legendecas marked this pull request as ready for review March 13, 2024 08:58
@legendecas legendecas added the async_hooks Issues and PRs related to the async hooks subsystem. label 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.
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link
Member

@Qard Qard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

@aduh95
Copy link
Contributor

aduh95 commented May 11, 2024

This needs a rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_hooks Issues and PRs related to the async hooks subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants