-
-
Notifications
You must be signed in to change notification settings - Fork 34.7k
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 89.76% 89.74% -0.03%
==========================================
Files 673 675 +2
Lines 203944 204585 +641
Branches 39191 39315 +124
==========================================
+ Hits 183080 183600 +520
- Misses 13194 13250 +56
- Partials 7670 7735 +65
🚀 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
|
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 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
74ba1a5 to
90dda43
Compare
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
90dda43 to
045d98f
Compare
| ```mjs | ||
| import process from 'node:process'; | ||
|
|
||
| process.addUncaughtExceptionCaptureCallback((err) => { |
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.
Won't it be available on global process?
|
|
||
| This feature is not available in [`Worker`][] threads. | ||
|
|
||
| ## `process.addUncaughtExceptionCaptureCallback(fn)` |
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.
Should we land it as experimental first?
| method with a non-`null` argument while another capture function is set will | ||
| throw an error. | ||
|
|
||
| Using this function is mutually exclusive with using the deprecated |
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.
I guess we should add this to changes: at the top of this function documentation
lib/internal/process/execution.js
Outdated
| // Otherwise, the primary callback (if set) is called. | ||
| function addUncaughtExceptionCaptureCallback(fn) { | ||
| if (typeof fn !== 'function') { | ||
| throw new ERR_INVALID_ARG_TYPE('fn', 'Function', fn); |
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.
| throw new ERR_INVALID_ARG_TYPE('fn', 'Function', fn); | |
| validateFunction(fn, 'fn'); |
- Mark addUncaughtExceptionCaptureCallback as experimental - Add changes entry to setUncaughtExceptionCaptureCallback docs - Use validateFunction instead of manual type check
6f50f41 to
6125d23
Compare
|
@RafaelGSS ping |
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