repl: don't use deprecated `domain` module by avivkeller · Pull Request #55204 · nodejs/node

avivkeller · 2024-10-01T00:26:55Z

This PR refactors the REPL module by eliminating the deprecated node:domain module. It replaces its functionality with a modern approach using a try/catch block and process listener.

Why?

The domain module has been deprecated for a while, and its use is discouraged. Despite this, Node.js still relies on it in a key component: the REPL. To lead by example and promote best practices, this PR updates the REPL module to use more up-to-date error-handling mechanisms, completely removing reliance on the deprecated domain module.

Additionally, this update simplifies the REPL's code and results in a slight performance improvement—though the gains are give-or-take a few ticks and may not be noticeable.

Breaking Changes

This PR is marked as semver-major PRs that contain breaking changes and should be released in the next major version. because it introduces functional changes to the REPL:

Removal of options.domain: This undocumented feature is removed (without a formal deprecation cycle, given the domain module itself is deprecated, and the feature was never documented nor tested).
Removal of ERR_INVALID_REPL_INPUT: This error is no longer needed, as after this change, there are no restricted inputs.

This PR is also marked needs-citgm PRs that need a CITGM CI run. in case any of the above cause breakages.

CC @nodejs/repl
CC @nodejs/tsc due to semver-major PRs that contain breaking changes and should be released in the next major version.

doc/api/repl.md

lib/repl.js

test/parallel/test-repl-save-load.js

test/parallel/test-repl-tab-complete-nested-repls.js

test/parallel/test-repl-tab.js

test/parallel/test-repl-top-level-await.js

codecov · 2024-10-01T01:53:51Z

Codecov Report

Attention: Patch coverage is 92.94118% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.42%. Comparing base (fa8f149) to head (b0d6991).
Report is 1144 commits behind head on main.

Files with missing lines	Patch %	Lines
lib/repl.js	92.94%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #55204    +/-   ##
========================================
  Coverage   88.41%   88.42%            
========================================
  Files         652      654     +2     
  Lines      186918   187688   +770     
  Branches    36072    36126    +54     
========================================
+ Hits       165270   165956   +686     
- Misses      14891    14963    +72     
- Partials     6757     6769    +12

Files with missing lines	Coverage Δ
lib/internal/errors.js	`96.99% <ø> (-0.01%)`	⬇️
lib/repl.js	`94.72% <92.94%> (-0.25%)`	⬇️

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

doc/api/errors.md

lib/repl.js

mcollina

I have a rough feeling that you'd need to add
a FinalizationRegistry to clear up any instances

test/parallel/test-repl-uncaught-exception-async.js

lib/repl.js

avivkeller · 2024-10-07T21:13:27Z

I have a rough feeling that you'd need to add a FinalizationRegistry to clear up any instances

Why? Is unsetting the callback on close() not enough?

lib/repl.js

mcollina

lgtm

I'm convinced.

nodejs-github-bot · 2024-10-26T10:56:34Z

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

aduh95 · 2024-11-03T11:11:38Z

Can someone start a CITGM if they thinks it's necessary. IMO we can use the most recent main branch CITGM for comparison

I doubt we have any package on CITGM that use repl, do we?

avivkeller · 2024-11-04T20:10:52Z

Got it! I've removed the label. IIRC semver-major PRs need two TSC approvals to land, would you mind reviewing?

nodejs-github-bot · 2024-11-10T20:10:36Z

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

nodejs-github-bot · 2024-11-12T17:42:45Z

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

mcollina · 2024-11-18T18:50:29Z

@nodejs/tsc add it to the agenda for visibility.

addaleax · 2024-11-18T19:50:33Z

doc/api/repl.md

-
-* Uncaught exceptions only emit the [`'uncaughtException'`][] event in the
-  standalone REPL. Adding a listener for this event in a REPL within
-  another Node.js program results in [`ERR_INVALID_REPL_INPUT`][].


I'd replace this section with one that describes the new behavior and amend the changes: YAML entry instead of dropping it (since the new behavior is definitely one that doesn't matter to some users but will have a significant negative impact on users who rely on it)

I don't think a specific section is needed, IMO there's nothing to document. it adds a listener to the process object, what negative impact does that have?

@redyetidev Well, before this change, uncaughtException listeners wouldn't have seen uncaught exceptions coming from the REPL, and now they do, right? It's not a massive change or anything, but I'd still document it somewhere. If you feel that no separate section is necessary, I'd still keep the changes: block to document the change in behavior over time

That's incorrect. Before this change, uncaughtException listeners still saw REPL exceptions

Oops. Nevermind then!

I am confused about this change. As far as I see it in the code, only the stand alone repl works with the uncaughtException listeners. In case more than a single REPL instance is running, it is not possible to identify the correct listeners, so none receive the errors.

addaleax

Needs docs; I'd also add a test for the specific behavior that we now have when two separate REPL instances receive the same uncaught exception.

There's probably even room for a way to restore the current behavior (which is imo the clearly correct one, even if it makes use of code that's deprecated for good reasons) – we'd a) need to make the REPL take an option that tells it whether to install the global process.on('uncaughtException') listener and b) allow users to set that flag to off, and start a REPL within a domain on their own + move errors from the domain to the REPL instance. That's not too complex, I think. I probably misread the code and the current logic is trying to re-establish this behavior, all good.

avivkeller · 2024-11-20T13:19:16Z

I'd also add a test for the specific behavior that we now have when two separate REPL instances receive the same uncaught exception.

That won't happen. It's specifically set up so that an uncaught exception triggered from REPL (A) won't affect REPL (B). A test to ensure this is https://github.com/nodejs/node/pull/55204/files#diff-ca6f6e3faa6e2dc3fba74eabfc212f36fa2f84b7a17e3f73662d24b678d18f73 (although it doesn't check two REPLs, rather it verifies that an uncaught exception triggered from outside the REPL does not affect the REPL, which [testing-wise] is a similar test)

BridgeAR

This is definitely nice work, I still think there are a few open parts though.

BridgeAR · 2024-11-27T16:05:56Z

lib/repl.js

@@ -612,13 +626,8 @@ function REPLServer(prompt,
        }
      } catch (e) {
        err = e;


Nit: the assignment is AFAIK not needed anymore

BridgeAR · 2024-12-05T19:21:18Z

lib/repl.js

+  },
+});
+
+let kHasSetUncaughtListener = false;


This is not a constant, so I would name the variable different.

Suggested change

let kHasSetUncaughtListener = false;

let hasUncaughtExceptionListener = false;

BridgeAR · 2024-12-05T19:42:51Z

lib/repl.js

+    }
+  };
+
+  self._onEvalError = function _onEvalError(e) {


It would be great not to expose this property.
I would just pass the instance to the method so that this method is a purely internal one.

For testing purposes, we would only have to verify that nothing else is written to REPLinstance.output.write().

BridgeAR · 2024-12-05T19:47:41Z

lib/repl.js

-  self._domain.on('error', function debugDomainError(e) {
-    debug('domain error');
+    try {
+      FunctionPrototypeApply(eval_, this, args);


eval_ could be an async function provided by the user. Would it be a good idea to await that method? I am not certain if that would improve things or if it makes it more difficult

BridgeAR · 2024-12-05T19:52:26Z

lib/repl.js

 const kLoadingSymbol = Symbol('loading');
+const kListeningREPLs = new SafeSet();
+const kAsyncREPLMap = new SafeMap();
+let kActiveREPL;


Suggested change

let kActiveREPL;

let activeREPL;

BridgeAR · 2024-12-05T19:59:53Z

lib/repl.js

+  if (kActiveREPL) {
+    kActiveREPL._onEvalError(er);


Are we definitely safe, that the active repl is definitely the correct one?
Let's say one repl starts an operation at time 0 and the operation needs 4 time intervals. A second repl instance start an operation at time 1 and that operation needs 2 intervals. The first instance fails after 2 intervals. Which repl instance is the error now reported upon? Do we maybe have a test for this?

BridgeAR · 2024-12-05T20:06:03Z

doc/api/repl.md

-
-* Uncaught exceptions only emit the [`'uncaughtException'`][] event in the
-  standalone REPL. Adding a listener for this event in a REPL within
-  another Node.js program results in [`ERR_INVALID_REPL_INPUT`][].


I am confused about this change. As far as I see it in the code, only the stand alone repl works with the uncaughtException listeners. In case more than a single REPL instance is running, it is not possible to identify the correct listeners, so none receive the errors.

avivkeller added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 1, 2024

nodejs-github-bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Oct 1, 2024

avivkeller added the needs-citgm PRs that need a CITGM CI run. label Oct 1, 2024

avivkeller marked this pull request as draft October 1, 2024 00:33

This comment was marked as resolved.

Sign in to view

avivkeller marked this pull request as ready for review October 1, 2024 00:34

anonrig reviewed Oct 1, 2024

View reviewed changes

aduh95 reviewed Oct 1, 2024

View reviewed changes

doc/api/errors.md Show resolved Hide resolved

avivkeller force-pushed the repl-no-domain branch from 42e6441 to fb9057d Compare October 1, 2024 19:52

avivkeller marked this pull request as draft October 1, 2024 19:54

avivkeller added the domain Issues and PRs related to the domain subsystem. label Oct 1, 2024

avivkeller marked this pull request as ready for review October 1, 2024 20:00

avivkeller requested review from aduh95 and anonrig October 1, 2024 20:01

avivkeller added the review wanted PRs that need reviews. label Oct 5, 2024

mcollina requested changes Oct 5, 2024

View reviewed changes

lib/repl.js Outdated Show resolved Hide resolved

avivkeller force-pushed the repl-no-domain branch from 47e132b to 9be6f17 Compare October 5, 2024 21:35

avivkeller requested a review from mcollina October 5, 2024 21:35

avivkeller force-pushed the repl-no-domain branch from 9be6f17 to 5f6d5ec Compare October 5, 2024 21:36

mcollina requested changes Oct 7, 2024

View reviewed changes

test/parallel/test-repl-uncaught-exception-async.js Show resolved Hide resolved

lib/repl.js Outdated Show resolved Hide resolved

avivkeller removed the review wanted PRs that need reviews. label Oct 7, 2024

avivkeller marked this pull request as draft October 16, 2024 15:16

avivkeller added the wip Issues and PRs that are still a work in progress. label Oct 16, 2024

avivkeller force-pushed the repl-no-domain branch from 5f6d5ec to dcf5e94 Compare October 16, 2024 18:46

mcollina reviewed Oct 17, 2024

View reviewed changes

lib/repl.js Show resolved Hide resolved

avivkeller removed the wip Issues and PRs that are still a work in progress. label Oct 22, 2024

mcollina approved these changes Oct 26, 2024

View reviewed changes

avivkeller added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Oct 26, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 26, 2024

repl: don't use deprecated domain module

b0d6991

avivkeller force-pushed the repl-no-domain branch from dcf5e94 to b0d6991 Compare October 26, 2024 16:56

This comment has been minimized.

Sign in to view

avivkeller mentioned this pull request Oct 30, 2024

doc,lib: remove deprecated domain module #45839

Draft

avivkeller removed the needs-citgm PRs that need a CITGM CI run. label Nov 3, 2024

avivkeller added the review wanted PRs that need reviews. label Nov 5, 2024

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2024

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 10, 2024

mcollina added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Nov 18, 2024

addaleax reviewed Nov 18, 2024

View reviewed changes

addaleax requested changes Nov 18, 2024

View reviewed changes

addaleax approved these changes Nov 19, 2024

View reviewed changes

avivkeller mentioned this pull request Nov 20, 2024

Node.js Technical Steering Committee (TSC) Meeting 2024-11-20 nodejs/TSC#1652

Closed

This was referenced Nov 25, 2024

Node.js Technical Steering Committee (TSC) Meeting 2024-11-27 nodejs/TSC#1656

Closed

Node.js Technical Steering Committee (TSC) Meeting 2024-12-04 nodejs/TSC#1660

Closed

BridgeAR requested changes Dec 5, 2024

View reviewed changes

BridgeAR removed the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Dec 5, 2024

aduh95 removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 7, 2024

avivkeller closed this Apr 1, 2025

	let kHasSetUncaughtListener = false;
	let hasUncaughtExceptionListener = false;

Uh oh!

Conversation

avivkeller commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why?

Breaking Changes

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Oct 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

avivkeller commented Oct 7, 2024

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Oct 26, 2024

Uh oh!

This comment has been minimized.

aduh95 commented Nov 3, 2024

Uh oh!

avivkeller commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Nov 10, 2024

Uh oh!

nodejs-github-bot commented Nov 12, 2024

Uh oh!

mcollina commented Nov 18, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avivkeller commented Nov 20, 2024

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avivkeller commented Oct 1, 2024 •

edited

Loading

codecov bot commented Oct 1, 2024 •

edited

Loading

avivkeller commented Nov 4, 2024 •

edited

Loading

addaleax left a comment •

edited

Loading