Skip to content
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

repl: don't use deprecated domain module #55204

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RedYetiDev
Copy link
Member

@RedYetiDev RedYetiDev commented Oct 1, 2024

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.setUncaughtExceptionCaptureCallback.

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.

@RedYetiDev RedYetiDev 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 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
@RedYetiDev RedYetiDev added the needs-citgm PRs that need a CITGM CI run. label Oct 1, 2024
@RedYetiDev RedYetiDev marked this pull request as draft October 1, 2024 00:33
@RedYetiDev

This comment was marked as resolved.

@RedYetiDev RedYetiDev marked this pull request as ready for review October 1, 2024 00:34
doc/api/repl.md Outdated Show resolved Hide resolved
lib/repl.js Outdated Show resolved Hide resolved
test/parallel/test-repl-save-load.js Show resolved Hide resolved
test/parallel/test-repl-tab-complete-nested-repls.js Outdated Show resolved Hide resolved
test/parallel/test-repl-tab.js Show resolved Hide resolved
test/parallel/test-repl-top-level-await.js Outdated Show resolved Hide resolved
Copy link

codecov bot commented Oct 1, 2024

Codecov Report

Attention: Patch coverage is 95.89041% with 3 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (fa8f149) to head (dcf5e94).
Report is 57 commits behind head on main.

Files with missing lines Patch % Lines
lib/repl.js 95.89% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #55204      +/-   ##
==========================================
- Coverage   88.41%   88.40%   -0.02%     
==========================================
  Files         652      653       +1     
  Lines      186918   187451     +533     
  Branches    36072    36082      +10     
==========================================
+ Hits       165270   165718     +448     
- Misses      14891    14965      +74     
- Partials     6757     6768      +11     
Files with missing lines Coverage Δ
lib/internal/errors.js 96.99% <ø> (-0.01%) ⬇️
lib/repl.js 94.84% <95.89%> (-0.12%) ⬇️

... and 128 files with indirect coverage changes

@RedYetiDev RedYetiDev marked this pull request as draft October 1, 2024 19:54
@RedYetiDev RedYetiDev added the domain Issues and PRs related to the domain subsystem. label Oct 1, 2024
@RedYetiDev RedYetiDev marked this pull request as ready for review October 1, 2024 20:00
@RedYetiDev RedYetiDev added the review wanted PRs that need reviews. label Oct 5, 2024
lib/repl.js Outdated Show resolved Hide resolved
Copy link
Member

@mcollina mcollina 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 a rough feeling that you'd need to add
a FinalizationRegistry to clear up any instances

lib/repl.js Outdated Show resolved Hide resolved
@RedYetiDev RedYetiDev removed the review wanted PRs that need reviews. label Oct 7, 2024
@RedYetiDev
Copy link
Member Author

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?

@RedYetiDev RedYetiDev marked this pull request as draft October 16, 2024 15:16
@RedYetiDev RedYetiDev added the wip Issues and PRs that are still a work in progress. label Oct 16, 2024

if (!kHasSetUncaughtListener) {
kAsyncHook.enable();
process.on('uncaughtException', handleUncaughtException);
Copy link
Member

Choose a reason for hiding this comment

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

This is still leaking things in the process object, and domain is a much safer way to do so anyway.

Copy link
Member Author

Choose a reason for hiding this comment

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

and domain is a much safer way to do so anyway.

I disagree. This is different than the domain. The domain bypasses the listener and inserts a callback before these listeners can even be called. This merely adds a listener, and later removes it. How is that "leaking"? The attachment to the process object is controlled.

@RedYetiDev RedYetiDev removed the wip Issues and PRs that are still a work in progress. label Oct 22, 2024
@RedYetiDev RedYetiDev marked this pull request as ready for review October 22, 2024 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. needs-citgm PRs that need a CITGM CI run. repl Issues and PRs related to the REPL subsystem. 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