Skip to content

Conversation

@mcollina
Copy link
Member

Port the domain module from createHook (async_hooks) to AsyncLocalStorage using the AsyncContextFrame-based implementation.

Key changes:

  • Use AsyncLocalStorage for domain context propagation instead of async_hooks.createHook()
  • Lazy initialization that triggers AsyncContextFrame prototype swap on first domain use
  • Use enterWith instead of ALS.run() so domain context is NOT automatically restored on exception - this matches the original domain.run() behavior where exit() only runs on success
  • Add ERR_ASYNC_RESOURCE_DOMAIN_REMOVED error for AsyncResource.domain
  • Update DEP0097 to End-of-Life status
  • Remove tests that relied on the removed MakeCallback domain property

The domain module now uses the AsyncContextFrame version of AsyncLocalStorage directly for proper context propagation across async boundaries.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/userland-migrations

@mcollina mcollina added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 17, 2025
@nodejs-github-bot nodejs-github-bot added async_hooks Issues and PRs related to the async hooks subsystem. domain Issues and PRs related to the domain subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. labels Dec 17, 2025
@codecov
Copy link

codecov bot commented Dec 17, 2025

Codecov Report

❌ Patch coverage is 92.88390% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.51%. Comparing base (81e05e1) to head (1eda843).
⚠️ Report is 30 commits behind head on main.

Files with missing lines Patch % Lines
lib/domain.js 92.63% 19 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #61095      +/-   ##
==========================================
- Coverage   88.53%   88.51%   -0.03%     
==========================================
  Files         703      703              
  Lines      208538   208662     +124     
  Branches    40224    40235      +11     
==========================================
+ Hits       184629   184696      +67     
- Misses      15912    15975      +63     
+ Partials     7997     7991       -6     
Files with missing lines Coverage Δ
lib/async_hooks.js 100.00% <100.00%> (ø)
lib/internal/async_hooks.js 99.36% <100.00%> (-0.02%) ⬇️
lib/internal/errors.js 97.52% <100.00%> (+<0.01%) ⬆️
lib/domain.js 95.42% <92.63%> (-2.97%) ⬇️

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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 18, 2025
Port the domain module from createHook (async_hooks) to
AsyncLocalStorage using the AsyncContextFrame-based implementation.

Key changes:
- Use AsyncLocalStorage for domain context propagation instead of
  async_hooks.createHook()
- Lazy initialization that triggers AsyncContextFrame prototype swap
  on first domain use
- Use enterWith instead of ALS.run() so domain context is NOT
  automatically restored on exception - this matches the original
  domain.run() behavior where exit() only runs on success
- Add ERR_ASYNC_RESOURCE_DOMAIN_REMOVED error for AsyncResource.domain
- Update DEP0097 to End-of-Life status
- Remove tests that relied on the removed MakeCallback domain property

The domain module now uses the AsyncContextFrame version of
AsyncLocalStorage directly for proper context propagation across
async boundaries.
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 19, 2025
@nodejs-github-bot
Copy link
Collaborator

Copy link
Member

@Flarna Flarna left a comment

Choose a reason for hiding this comment

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

I have to admit that I do not fully understand domains so take my review with a grain of salt.

Remove the useDomainTrampoline function and related domain_cb
variable that are no longer used after domain was ported to
AsyncLocalStorage.
The test no longer tests async-id map leaks. Rename to reflect
its actual purpose: verifying that accessing domain property on
AsyncResource throws ERR_ASYNC_RESOURCE_DOMAIN_REMOVED.
Use require('async_hooks').AsyncLocalStorage instead of directly
importing internal modules. This allows proper auto-selection of
the ALS implementation based on --async-context-frame flag.
Extract common domain context setup/teardown logic into a single
runInDomain helper function, reducing code duplication.
When an EventEmitter emits 'error' and the domain's error handler
throws, the exception should propagate to the parent domain, not
recursively call the same domain's error handler.

The bug was that currentDomain/currentStack were not updated when
temporarily switching to the parent domain context during error
emission. Since domainExceptionHandler checks currentDomain first,
exceptions would incorrectly route back to the same domain.

Fixes by updating currentDomain/currentStack alongside the ALS store.
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. domain Issues and PRs related to the domain subsystem. errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants