Skip to content

Conversation

@mcollina
Copy link
Member

@mcollina mcollina commented Dec 30, 2025

Replace domain-based error handling with AsyncLocalStorage and setUncaughtExceptionCaptureCallback. This removes REPL's dependency on the deprecated domain module while preserving existing behavior and allowing the domain module to still be used within REPL sessions.

Changes

  • Use AsyncLocalStorage to track REPL instance context
  • Add addUncaughtExceptionCaptureCallback API for auxiliary callbacks, allowing REPL and domain to coexist
  • REPL checks for active domain before handling errors locally
  • Remove mutual exclusivity between domain and setUncaughtExceptionCaptureCallback

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Dec 30, 2025
@avivkeller avivkeller added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 1, 2026
mcollina added a commit to mcollina/node that referenced this pull request Jan 10, 2026
Replace the domain-based error handling with AsyncLocalStorage and
setUncaughtExceptionCaptureCallback. This removes the REPL's dependency
on the deprecated domain module while preserving all existing behavior:

- Synchronous errors during eval are caught and displayed
- Async errors (setTimeout, promises, etc.) are caught via the
  uncaught exception capture callback
- Top-level await errors are caught and displayed
- The REPL continues operating after errors
- Multiple REPL instances can coexist with errors routed correctly

Changes:
- Use AsyncLocalStorage to track which REPL instance owns an async
  context, replacing domain's automatic async tracking
- Add setupExceptionCapture() to install
  setUncaughtExceptionCaptureCallback for catching async errors and
  routing them to the correct REPL
- Extract error handling logic into REPLServer.prototype._handleError()
- Wrap eval execution in replContext.run() for async context tracking
- Update newListener protection to check AsyncLocalStorage context
- Throw ERR_INVALID_ARG_VALUE if options.domain is passed

PR-URL: nodejs#61227
@mcollina mcollina force-pushed the repl/remove-domain-dependency branch from 6ef82e6 to e86555a Compare January 10, 2026 10:35
@mcollina mcollina marked this pull request as ready for review January 10, 2026 10:35
@codecov
Copy link

codecov bot commented Jan 10, 2026

Codecov Report

❌ Patch coverage is 95.65217% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.53%. Comparing base (4f24aff) to head (74ba1a5).
⚠️ Report is 141 commits behind head on main.

Files with missing lines Patch % Lines
lib/repl.js 94.62% 10 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #61227    +/-   ##
========================================
  Coverage   88.53%   88.53%            
========================================
  Files         703      704     +1     
  Lines      208546   208837   +291     
  Branches    40217    40314    +97     
========================================
+ Hits       184634   184896   +262     
+ Misses      15926    15924     -2     
- Partials     7986     8017    +31     
Files with missing lines Coverage Δ
lib/domain.js 98.33% <100.00%> (-0.05%) ⬇️
lib/internal/bootstrap/node.js 99.58% <100.00%> (+<0.01%) ⬆️
lib/internal/process/execution.js 95.10% <100.00%> (+0.97%) ⬆️
lib/repl.js 93.87% <94.62%> (-0.25%) ⬇️

... and 81 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
Copy link
Member Author

@nodejs/tsc this needs more reviews

@aduh95
Copy link
Contributor

aduh95 commented Jan 10, 2026

There are linter and test failures to address

Replace the domain-based error handling with AsyncLocalStorage and
setUncaughtExceptionCaptureCallback. This removes the REPL's dependency
on the deprecated domain module while preserving all existing behavior:

- Synchronous errors during eval are caught and displayed
- Async errors (setTimeout, promises, etc.) are caught via the
  uncaught exception capture callback
- Top-level await errors are caught and displayed
- The REPL continues operating after errors
- Multiple REPL instances can coexist with errors routed correctly

Changes:
- Use AsyncLocalStorage to track which REPL instance owns an async
  context, replacing domain's automatic async tracking
- Add setupExceptionCapture() to install
  setUncaughtExceptionCaptureCallback for catching async errors and
  routing them to the correct REPL
- Extract error handling logic into REPLServer.prototype._handleError()
- Wrap eval execution in replContext.run() for async context tracking
- Update newListener protection to check AsyncLocalStorage context
- Throw ERR_INVALID_ARG_VALUE if options.domain is passed

PR-URL: nodejs#61227
@mcollina mcollina force-pushed the repl/remove-domain-dependency branch from e86555a to 855468d Compare January 11, 2026 12:46
@mcollina
Copy link
Member Author

@aduh95 done

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 12, 2026
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Jan 12, 2026
@github-actions
Copy link
Contributor

Failed to start CI
   ⚠  Commits were pushed since the last approving review:
   ⚠  - repl: remove dependency on domain module
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/20913460585

Copy link
Member

@marco-ippolito marco-ippolito left a comment

Choose a reason for hiding this comment

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

LGTM with green CI

Add auxiliary callback mechanism to setUncaughtExceptionCaptureCallback
to allow REPL and domain module to coexist. The REPL uses the new
addUncaughtExceptionCaptureCallback API which doesn't conflict with
domain's use of the primary callback.

- Add addUncaughtExceptionCaptureCallback for non-exclusive callbacks
- Update REPL to check for active domain before handling errors
- Remove mutual exclusivity checks from domain module
- Restore test-repl-domain.js test
- Update domain coexistence tests
@mcollina mcollina added semver-minor PRs that contain new features and should be released in the next minor version. baking-for-lts PRs that need to wait before landing in a LTS release. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. and removed semver-major PRs that contain breaking changes and should be released in the next major version. labels Jan 13, 2026
@mcollina
Copy link
Member Author

I’ve updated this PR and made it minor by adding a new API and make it backward compatible with using domain.

I would wait for backporting this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

baking-for-lts PRs that need to wait before landing in a LTS release. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. semver-minor PRs that contain new features and should be released in the next minor version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants