-
-
Notifications
You must be signed in to change notification settings - Fork 34.4k
repl: remove dependency on domain module #61227
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
base: main
Are you sure you want to change the base?
Conversation
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
6ef82e6 to
e86555a
Compare
Codecov Report❌ Patch coverage is
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
🚀 New features to boost your workflow:
|
|
@nodejs/tsc this needs more reviews |
|
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
e86555a to
855468d
Compare
|
@aduh95 done |
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 PRhttps://github.com/nodejs/node/actions/runs/20913460585 |
marco-ippolito
left a comment
There was a problem hiding this 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
|
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. |
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
addUncaughtExceptionCaptureCallbackAPI for auxiliary callbacks, allowing REPL and domain to coexist