repl: catch promise errors during eval in completion #58943

islandryu · 2025-07-03T14:20:11Z

Fixes: #58903
The issue occurred when an unhandled Promise was executed during the completion's eval.

This change catches and ignores the error, similar to the handling here.

node/lib/repl.js

Line 1544 in 6a3b545

const evalExpr = `try { ${expr} } catch {}`;

codecov · 2025-07-03T16:52:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.05%. Comparing base (ab694d5) to head (86b5a9b).
Report is 4 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58943      +/-   ##
==========================================
+ Coverage   90.03%   90.05%   +0.01%     
==========================================
  Files         648      648              
  Lines      190969   191013      +44     
  Branches    37432    37451      +19     
==========================================
+ Hits       171938   172010      +72     
+ Misses      11672    11622      -50     
- Partials     7359     7381      +22

Files with missing lines	Coverage Δ
lib/repl.js	`94.43% <100.00%> (+0.18%)`	⬆️

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

dario-piotrowicz

The fix itself looks good to me, however I feel like the tests could be improved

dario-piotrowicz · 2025-07-05T13:03:56Z

test/parallel/test-repl-eval-promises.js

+  { send: `async function f() {}; f().` },
+];
+
+const tests = [


Why are there non-completion tests here?

If these tests are necessary I would suggest to have them in their own separate test file, I don't think there's any benefit in running both set of tests here, is there?

I had originally added it just to be safe, even though I knew it wouldn't have any real effect.
But since it’s not strictly necessary, I’ve removed it.

I think we can add it as a separate test file, as I had a quick look and I feel like promise evaluation is not being tested? anyways that can be also done separately if we want I think 🙂

Oh I see, I'll keep it as well in a separate file.
Is it okay to include that in this PR?

Oh I see, I'll keep it as well in a separate file.

yeah sounds good to me 🙂

Is it okay to include that in this PR?

Of course 🙂

If you're up for it I think it would be great if you could rebase and have two commits here, one for the tab-complete and one for the new eval tests then we can commit-queue-rebase and have the two clean commits in, I think that that would be the cleanest way to land this 🙂 , but if you don't feel like it the current merge squash is completely fine too 👍

I’ve rebased the branch!

dario-piotrowicz · 2025-07-05T13:14:03Z

test/parallel/test-repl-eval-promises.js

+  await runReplTests(tests);
+})().then(common.mustCall());
+
+async function runReplCompleteTests(tests) {


In my opinion tab completion tests should use the repl's complete function as I think that it is the cleanest most robust/stable way for testing this functionality (no need to collect the repl output and add setTimeouts).

For example you can see here: https://github.com/dario-piotrowicz/node/blob/34f24960fb813609f3bd0080ac26bbee1611f142/test/parallel/test-repl-tab-complete-computed-props.js how I recently did it

You can also notice in my tests that the strategy allows to use a single repl instance (since anyways the tests don't rely on state) instead of creating a new one every time

Thanks, Your approach definitely makes things much simpler.
I've updated the tests to use repl.complete, and also renamed the file accordingly to reflect the change.

dario-piotrowicz

Looks great, thanks for the updates @islandryu 🫶

nodejs-github-bot · 2025-07-09T08:22:00Z

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

islandryu · 2025-07-10T12:23:21Z

@dario-piotrowicz
Could you take another look? I’ve updated the commit.
I only resolved conflicts with the main branch.

nodejs-github-bot · 2025-07-10T12:28:21Z

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

dario-piotrowicz

@dario-piotrowicz Could you take another look? I’ve updated the commit. I only resolved conflicts with the main branch.

Hi @islandryu 👋

Sorry about the merge conflicts those were my fault 🙇

The changes still look good to me 😄

However now there's an extra eval that I think should also include your check:

node/lib/repl.js

Line 1822 in 7b2d30c

`try { ${exprStr} } catch {} `, ctx, getREPLResourceName(), (err, obj) => {

If you could also add the check there (alongside a test if it's not too problematic) that'd be great 😄

But if you want I can also do that in a separate PR, up to you 🙂

islandryu · 2025-07-10T14:44:15Z

Thanks for pointing that out!
I'll go ahead and add the check and a test for that part as well.
I’ll probably be able to take a look over the weekend

islandryu · 2025-07-13T14:52:51Z

#59044
There is a conflict, so I will address it after finishing this fix.

dario-piotrowicz · 2025-07-20T13:54:10Z

lib/repl.js

@@ -351,6 +353,7 @@ function REPLServer(prompt,
  this.allowBlockingCompletions = !!options.allowBlockingCompletions;
  this.useColors = !!options.useColors;
  this._domain = options.domain || domain.create();
+  this._completeDomain = domain.create();


why is this needed? the domain API is deprecated so I think that we should ideally use it as less as possible (although it is already quite pervasive in the repl logic... but I think we should not make the issue worse if we can help it)

Thank you for taking a look.
This is the reason for it:
#58943 (comment)

I've completely removed this _completeDomain from the source code (alongside completeEval) and also from the test files

Diff

For completeness this is my whole diff:

diff --git a/lib/repl.js b/lib/repl.js index 49848069063..6e7643c18e7 100644 --- a/lib/repl.js +++ b/lib/repl.js @@ -353,7 +353,6 @@ function REPLServer(prompt, this.allowBlockingCompletions = !!options.allowBlockingCompletions; this.useColors = !!options.useColors; this._domain = options.domain || domain.create(); - this._completeDomain = domain.create(); this.useGlobal = !!useGlobal; this.ignoreUndefined = !!ignoreUndefined; this.replMode = replMode || module.exports.REPL_MODE_SLOPPY; @@ -671,8 +670,6 @@ function REPLServer(prompt, } self.eval = self._domain.bind(eval_); - self.completeEval = self._completeDomain.bind(eval_); - self._completeDomain.on('error', (err) => { }); self._domain.on('error', function debugDomainError(e) { debug('domain error'); @@ -1546,7 +1543,7 @@ function complete(line, callback) { return includesProxiesOrGetters( completeTargetAst.body[0].expression, parsableCompleteTarget, - this.completeEval, + this.eval, this.context, (includes) => { if (includes) { @@ -1563,7 +1560,7 @@ function complete(line, callback) { const memberGroups = []; const evalExpr = `try { ${expr} } catch {}`; - this.completeEval(evalExpr, this.context, getREPLResourceName(), (e, obj) => { + this.eval(evalExpr, this.context, getREPLResourceName(), (e, obj) => { try { reclusiveCatchPromise(obj); let p; diff --git a/test/fixtures/repl-tab-completion-nested-repls.js b/test/fixtures/repl-tab-completion-nested-repls.js index 5a0c64d60a8..1d2b154f2b3 100644 --- a/test/fixtures/repl-tab-completion-nested-repls.js +++ b/test/fixtures/repl-tab-completion-nested-repls.js @@ -32,7 +32,7 @@ const putIn = new ArrayStream(); const testMe = repl.start('', putIn); // Some errors are passed to the domain, but do not callback. -testMe._completeDomain.on('error', function(err) { +testMe._domain.on('error', function(err) { throw err; }); diff --git a/test/parallel/test-repl-tab-complete-getter-error.js b/test/parallel/test-repl-tab-complete-getter-error.js index 5ea9e4266f1..e2e36b85c58 100644 --- a/test/parallel/test-repl-tab-complete-getter-error.js +++ b/test/parallel/test-repl-tab-complete-getter-error.js @@ -21,7 +21,7 @@ async function runTest() { terminal: true }); - replServer._completeDomain.on('error', (e) => { + replServer._domain.on('error', (e) => { assert.fail(`Error in REPL domain: ${e}`); }); diff --git a/test/parallel/test-repl-tab-complete-promises.js b/test/parallel/test-repl-tab-complete-promises.js index b2225f8b03c..86aac9b508b 100644 --- a/test/parallel/test-repl-tab-complete-promises.js +++ b/test/parallel/test-repl-tab-complete-promises.js @@ -64,13 +64,13 @@ async function runReplCompleteTests(tests) { if (completeError) { completeErrorPromise = new Promise((resolve) => { const handleError = () => { - replServer._completeDomain.removeListener('error', handleError); + replServer._domain.removeListener('error', handleError); resolve(); }; - replServer._completeDomain.on('error', handleError); + replServer._domain.on('error', handleError); }); } else { - replServer._completeDomain.on('error', onError); + replServer._domain.on('error', onError); } await replServer.complete( @@ -88,7 +88,7 @@ async function runReplCompleteTests(tests) { if (!completeError) { await new Promise((resolve) => { setImmediate(() => { - replServer._completeDomain.removeListener('error', onError); + replServer._domain.removeListener('error', onError); resolve(); }); }); diff --git a/test/parallel/test-repl-tab-complete.js b/test/parallel/test-repl-tab-complete.js index d77067cb8f5..f37916e30d8 100644 --- a/test/parallel/test-repl-tab-complete.js +++ b/test/parallel/test-repl-tab-complete.js @@ -44,7 +44,7 @@ function prepareREPL() { }); // Some errors are passed to the domain, but do not callback - replServer._completeDomain.on('error', assert.ifError); + replServer._domain.on('error', assert.ifError); return { replServer, input }; }

After that (and rebuilding node of course) it looks like all the reply tests are still passing?

am I missing something?

I don't think the test will fail.

if (completeError) { completeErrorPromise = new Promise((resolve) => { const handleError = () => { - replServer._completeDomain.removeListener('error', handleError); + replServer._domain.removeListener('error', handleError); resolve(); }; - replServer._completeDomain.on('error', handleError); + replServer._domain.on('error', handleError); }); } else { - replServer._completeDomain.on('error', onError); + replServer._domain.on('error', onError); }

However, the problem is that this test throws an error in replServer._domain.
When completeError is true, an error is expected.
If the error occurs in replServer._domain, it can cause an infinite.

If _domain is not used for completion, errors won't affect the REPL even if they occur.

@dario-piotrowicz
Just a gentle ping, no hurry.

Yes I can see that

function foo() { Promise.reject() } foo().

currently does cause an infinite loop, but I wonder if the correct solution here wouldn't be to prevent the tab completion altogether (via AST analysis) since the function could also be side effectful, so it does feel more correct to me to prevent it running at all if possible? 🤔

That's exactly right, I'll give it a try.

However, I'm concerned that completely eliminating this issue might require extensive AST traversal, potentially excluding a large number of functions from execution.
For example, this issue can also occur in cases like the following:

async function foo() { fetch("") }; function bar() { foo() }; bar().

This means we would need to traverse all called functions — including transitively called functions — and if any part involves an async function, we would have to exclude it from execution.

Anyway, I'll see if there's an easy way to make this work.

That's exactly right, I'll give it a try.

However, I'm concerned that completely eliminating this issue might require extensive AST traversal, potentially excluding a large number of functions from execution. For example, this issue can also occur in cases like the following:

async function foo() { fetch("") }; function bar() { foo() }; bar().

This means we would need to traverse all called functions — including transitively called functions — and if any part involves an async function, we would have to exclude it from execution.

I am not sure I follow, why would traversal be needed?
If we encounter bar(). we see that it's a member accessor on a function call and stop there, I don't think there is any need to then traverse the "call stack"? 🤔

Regarding completely eliminating the issue I feel like there is a limit to what the REPL can do (and there are already a few things that it's not great at) and I think it's ok for it not to covert every possible small edge case. There are cases like the one you presented in your comment, that I struggle to imagine people actually unintentionally encountering, so at the end of the day my personal opinion is that if some solutions require a significant effort to maintain and implement and/or have significant drawbacks but almost no one will ever need them then there it's not really worth investing time and effort into them (or possibly trying to solve them only after people legitimately encounter them and open issues regarding them).

I had assumed that we would traverse the function to check whether it could throw an async error, and only in that case disable completion.
But are you saying that, regardless of the function’s contents, completion is simply disabled for a member accessor on a function call?

dario-piotrowicz · 2025-07-20T13:57:11Z

PS: I removed my approval as the PR seems to have non trivially changed since my approval (and I am not sure if I agree with the new changes)

Fixes: nodejs#58903

islandryu

Sorry that the diff became larger compared to the previous commit, but while adding more tests, I realized that the following points also need to be considered:

There may be Promises nested inside objects.
There may be some code that is inherently uncatchable.

Therefore, I've made the corresponding changes in the following parts.

islandryu · 2025-07-20T14:25:25Z

lib/repl.js

@@ -1911,6 +1922,33 @@ function includesProxiesOrGetters(expr, exprStr, evalFn, ctx, callback) {
  return callback(false);
 }

+function reclusiveCatchPromise(obj, seen = new SafeWeakSet()) {


This reclusive code is necessary to handle errors that might be thrown from within objects or similar structures.
https://github.com/nodejs/node/pull/58943/files#diff-24ded3e78d8bb56bd21deb058a4766995ee891242cf993a9a185c57213ce38c0R15

islandryu · 2025-07-20T14:38:31Z

lib/repl.js

@@ -351,6 +353,7 @@ function REPLServer(prompt,
  this.allowBlockingCompletions = !!options.allowBlockingCompletions;
  this.useColors = !!options.useColors;
  this._domain = options.domain || domain.create();
+  this._completeDomain = domain.create();


_completeDomain is for handling errors that cannot be caught.
https://github.com/nodejs/node/pull/58943/files#diff-24ded3e78d8bb56bd21deb058a4766995ee891242cf993a9a185c57213ce38c0R21-R23
For instance, in this test, baz[getPropText()] is evaluated as part of the completion process, but since it doesn't return a Promise object, the error cannot be caught.
Because the error is unavoidable in this situation, I've chosen to run it in a separate domain to prevent it from impacting the main domain.

However, I'm not entirely confident that this is the correct or most appropriate approach, so it might be better to leave this type of case as a TODO for now and hold off on implementing a fix.

islandryu · 2025-07-21T00:43:01Z

I made some minor changes to the test.
I've split the commits to make it easier to review, but I'll rebase them before merging.

nodejs-github-bot added needs-ci PRs that need a full CI run. repl Issues and PRs related to the REPL subsystem. labels Jul 3, 2025

dario-piotrowicz reviewed Jul 5, 2025

View reviewed changes

islandryu force-pushed the 58903 branch from 3fa4356 to e11cad6 Compare July 8, 2025 13:17

dario-piotrowicz approved these changes Jul 8, 2025

View reviewed changes

dario-piotrowicz added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Jul 8, 2025

jasnell approved these changes Jul 8, 2025

View reviewed changes

dario-piotrowicz approved these changes Jul 8, 2025

View reviewed changes

islandryu force-pushed the 58903 branch from b403c3e to f1f0185 Compare July 9, 2025 01:13

islandryu added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. and removed commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Jul 9, 2025

islandryu force-pushed the 58903 branch from f1f0185 to 7b2d30c Compare July 10, 2025 09:37

islandryu requested a review from dario-piotrowicz July 10, 2025 12:22

dario-piotrowicz approved these changes Jul 10, 2025

View reviewed changes

islandryu mentioned this pull request Jul 13, 2025

repl: handle errors from getters during completion #59044

Merged

islandryu force-pushed the 58903 branch from 7b2d30c to ba167d1 Compare July 20, 2025 13:10

dario-piotrowicz reviewed Jul 20, 2025

View reviewed changes

dario-piotrowicz self-requested a review July 20, 2025 13:54

islandryu added 2 commits July 20, 2025 23:06

repl: catch promise errors during eval in completion

949f08a

Fixes: nodejs#58903

test: add repl test for promise eval

0e44668

islandryu force-pushed the 58903 branch from ba167d1 to 0e44668 Compare July 20, 2025 14:20

islandryu commented Jul 20, 2025

View reviewed changes

use mustCall for domain error

5f15bef

assert no error occurs in _domain

86b5a9b

Uh oh!

repl: catch promise errors during eval in completion #58943

Are you sure you want to change the base?

repl: catch promise errors during eval in completion #58943

Conversation

islandryu commented Jul 3, 2025

Uh oh!

codecov bot commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dario-piotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz Jul 5, 2025 • 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!

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!

dario-piotrowicz Jul 5, 2025 • 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!

islandryu Jul 8, 2025 • 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!

dario-piotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Jul 9, 2025

Uh oh!

islandryu commented Jul 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 10, 2025

Uh oh!

dario-piotrowicz left a comment

Choose a reason for hiding this comment

Uh oh!

islandryu commented Jul 10, 2025

Uh oh!

islandryu commented Jul 13, 2025

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!

dario-piotrowicz Jul 28, 2025 • 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!

islandryu Jul 29, 2025 • 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!

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!

dario-piotrowicz commented Jul 20, 2025

Uh oh!

islandryu left a comment

codecov bot commented Jul 3, 2025 •

edited

Loading

dario-piotrowicz Jul 5, 2025 •

edited

Loading

dario-piotrowicz Jul 5, 2025 •

edited

Loading

islandryu Jul 8, 2025 •

edited

Loading

islandryu commented Jul 10, 2025 •

edited

Loading

dario-piotrowicz Jul 28, 2025 •

edited

Loading

islandryu Jul 29, 2025 •

edited

Loading

islandryu Jul 20, 2025 •

edited

Loading