Skip to content

Conversation

legendecas
Copy link
Member

@legendecas legendecas commented Apr 25, 2025

This fixes the leak behavior when using enterWith when no
AsyncLocalStorages were enabled inside a promise. With this
change, the following code snippets will behave the same:

import { AsyncLocalStorage } from 'node:async_hooks';

const als = new AsyncLocalStorage();

async function main () {
  await 1
  als.enterWith('internal');
}

await main()

console.log(als.getStore() ?? 'default');
import { AsyncLocalStorage } from 'node:async_hooks';

const als = new AsyncLocalStorage();
als.enterWith(undefined); // Forcefully enable the AsyncLocalStorage.

async function main () {
  await 1
  als.enterWith('internal');
}

await main()

console.log(als.getStore() ?? 'default');

On the performance side, given that an AsyncLocalStorage is constructed, we should assume that it will be used and prefer correctness over lazy-initialization optimization.

Fixes: #53037
Refs: #58019

/cc @nodejs/diagnostics

This fixes the leak behavior when using `enterWith` when no
`AsyncLocalStorage`s were enabled inside a promise.
@nodejs-github-bot nodejs-github-bot added async_local_storage AsyncLocalStorage needs-ci PRs that need a full CI run. labels Apr 25, 2025
Copy link

codecov bot commented Apr 25, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.27%. Comparing base (25fe802) to head (0ee37f3).
Report is 18 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58029      +/-   ##
==========================================
- Coverage   90.27%   90.27%   -0.01%     
==========================================
  Files         630      630              
  Lines      186159   186161       +2     
  Branches    36473    36476       +3     
==========================================
- Hits       168053   168048       -5     
+ Misses      10976    10974       -2     
- Partials     7130     7139       +9     
Files with missing lines Coverage Δ
lib/internal/async_local_storage/async_hooks.js 97.95% <100.00%> (+0.02%) ⬆️

... and 19 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.

@Flarna Flarna added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 26, 2025
@nodejs-github-bot
Copy link
Collaborator

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

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

@Qard
Copy link
Member

Qard commented Apr 26, 2025

Does this only reproduce with the async_hooks version of AsyncLocalStorage?

@legendecas
Copy link
Member Author

Does this only reproduce with the async_hooks version of AsyncLocalStorage?

Yes.

@legendecas legendecas added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. labels Apr 27, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Apr 27, 2025
@nodejs-github-bot nodejs-github-bot merged commit 8e7ae60 into nodejs:main Apr 27, 2025
73 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 8e7ae60

@legendecas legendecas deleted the als/enable branch April 27, 2025 23:48
@mcollina
Copy link
Member

I'm adding a backing-for-lts comment as we should evaluate if it has real-life breakage before backporting

@mcollina mcollina added the baking-for-lts PRs that need to wait before landing in a LTS release. label Apr 28, 2025
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
This fixes the leak behavior when using `enterWith` when no
`AsyncLocalStorage`s were enabled inside a promise.

PR-URL: #58029
Fixes: #53037
Refs: #58019
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
This fixes the leak behavior when using `enterWith` when no
`AsyncLocalStorage`s were enabled inside a promise.

PR-URL: #58029
Fixes: #53037
Refs: #58019
Reviewed-By: Gerhard Stöbich <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
async_local_storage AsyncLocalStorage author ready PRs that have at least one approval, no pending requests for changes, and a CI started. baking-for-lts PRs that need to wait before landing in a LTS release. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AsyncLocalStorage loses the store if used together with createHook if there is a async function in between
6 participants