readline: add stricter validation for functions called after closed #57680

dario-piotrowicz · 2025-03-29T23:07:41Z

This PR makes it so that calling pause(), resume() or write() on a closed readline interface results in ERR_USE_AFTER_CLOSE errors being thrown (following the same behavior as question())

Note

I'm slightly concerned regarding the tests I needed to update, as those might indicate that the stricted
readline checks might have some potential unwanted implications, if the changes here were to land it might be
safer to add them as a semver major I think 🤔

codecov · 2025-03-30T01:41:21Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (33f6e1e) to head (f6b9d13).
Report is 5 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #57680   +/-   ##
=======================================
  Coverage   90.24%   90.25%           
=======================================
  Files         630      630           
  Lines      185245   185206   -39     
  Branches    36299    36300    +1     
=======================================
- Hits       167173   167153   -20     
+ Misses      11018    10995   -23     
- Partials     7054     7058    +4

Files with missing lines	Coverage Δ
lib/internal/readline/interface.js	`96.97% <100.00%> (+0.01%)`	⬆️

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

geeksilva97 · 2025-03-31T13:35:54Z

test/parallel/test-readline-interface.js

+    rli.question('What\'s your name?', common.mustCall((name) => {
+      assert.strictEqual(name, 'Node.js');
+      rli.close();
+      assert.throws(() => {


Maybe we can have the rli.write call here too? Seems like the behavior is the same for the three calls when the interface is closed.

Are you asking if we could move rli.write from line 1212 to here? and fully remove that test block?

Sure, we can 🙂, although I personally am a fan of small focused tests, so that if one thing breaks ideally only a single test fails, so my preference would be to keep them as they are, I am however happy to combine the two, especially if some other folks share your same preference 🙂

(PS: you could argue that I am already combining together pause and resume here, that's be a valid argument, but those feel to me like they go together that's why I put them in the same test 😅)

Yeah, this is not a block. That's just all the functions end up with the same behavior when the interface is closed, hence the test is testing the same behavior and it would be good to have them together.

I had another look and I remembered that there are already two separate testing blocks for question and question promisified, so I'm indeed (loosely) following this pattern with my additions, so if I were to combine things I feel like at that point it would make sense to combine all these blocks right?
but at that point we have a single big test/block which tests all the various functions being called after close 🤔 feels a untidy to me 🤔

I am not extremely against the idea but yeah it wouldn't be my personal preference 🤔

As I said, not a blocker at all.

jasnell

LGTM. Unfortunately I think the additional throw here will make this semver-major

dario-piotrowicz · 2025-04-01T20:32:53Z

Unfortunately I think the additional throw here will make this semver-major

yeah I understand and agree 🙂, I also did think the same and mentioned this in my PR description 👍

nodejs-github-bot · 2025-04-03T20:42:16Z

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

test/parallel/test-readline-promises-tab-complete.js

test/parallel/test-repl-import-referrer.js

mcollina

lgtm

nodejs-github-bot · 2025-04-04T15:11:35Z

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

jakecastelli · 2025-04-05T01:24:11Z

this test - test-repl-import-referrer seems constantly failing between CI runs, would you mind taking a look? I will rerun the CI after your investigation 👍 cheers

dario-piotrowicz · 2025-04-05T15:19:03Z

Hi @jakecastelli 👋

I just had a look, the setTimeout is needed there because:

the REPL, which is implements a readline interface,
under the hood calls finishExecution at the end of the command evaluation
this in turn calls finish
which in turn calls displayPrompt
which calls the readline prompt
which tries to call resume on the interface

This, without the setTimeout causes the stricter readline validation to error, since child.stdin.end(); causes the readline interface to be closed

My guess now is that in CI the REPL can take more than 100ms to call finishExecution causing the error (note: the command being executed is await import('./message.mjs') so I do imagine that the time for the dynamic import to resolve can vary depending on the underlying platform 🤔), so I increased the timeout time to 300 hoping that that will solve the issue

Alternatively I could update the code in test/parallel/test-repl-import-referrer.js not to use a setTimeout at all but, depending on the output decide when to end the child.stdin stream, I started looking into that but I think that that solution would likely be more brittle / less clean than just having a setTimeout call 🤔

WDYT? could you try re-running the CI and see if the new timeout value helps? 🙏

jakecastelli · 2025-04-06T13:01:26Z

Hi @dario-piotrowicz 👋 thanks for the detailed explanation, I didn't remember I've mentioned setTimeout is not needed, maybe I confused you with this comment? #57680 (comment), if so, my intention was that setTimeout used to strictly require a duration as second argument in our linting rules, I think this may have changed at sometime.

dario-piotrowicz · 2025-04-06T13:26:05Z

Hi @dario-piotrowicz 👋 thanks for the detailed explanation, (#57680 (comment)), if so, my intention was that setTimeout used to strictly require a duration as second argument in our linting rules, I think this may have changed at sometime.

Hey @jakecastelli thanks for re-running the CI 🙂 (let's hope that it works now 😁🤞)

No I don't think there was some confusion, in my comment I was explaining why the setTimeout is needed just for discussing the potential alternatives and also because I would have liked not to have used it in the first place 🙂

Regarding your comment about setTimeout being called without a specified delay, would you like me to add the missing delays? (I can also increase them from 0 to say 100 or something, it can help avoiding potential future CI
flakiness?)

jakecastelli · 2025-04-06T13:37:32Z

lib/internal/readline/interface.js

@@ -548,6 +551,9 @@ class Interface extends InterfaceConstructor {
   * @returns {void | Interface}
   */
  resume() {
+    if (this.closed) {
+      throw new ERR_USE_AFTER_CLOSE('readline');


Just a suggestion - any chance we could defer the throw in the nextTick? Something like:

Suggested change

throw new ERR_USE_AFTER_CLOSE('readline');

process.nextTick(()=> {

throw new ERR_USE_AFTER_CLOSE('readline');

});

mh... sorry but I think I'd be pretty against this 🤔

deferring it to the next tick would mean that:

the rest of the function would incorrectly run, for example emitting events such as resume
(this could cause subtle bugs with potential listeners of such events (rli.on('resume', () => {...})))

the error could no be try-catchable (e.g. it would not allow users to do try { rli.resume(); } catch { ... })

Both of the above seem pretty incorrect behaviors to me 🤔

What would the benefit of deferring be?

Sorry mate, I missed a return statement after the nextTick in my code suggestion.

You are right that the error would not be try-catchable. But looking at the test test/parallel/test-repl-import-referrer.js that needs setting a large number seems a bit flimsy, I just gave a try on my less powerful intel macbook pro (2020) and I need to set it to 400ms to pass the test, so I am not too sure if 300ms would be sufficient (as our CI has some underpowered machines).

That leads me to the deferred error throw path, but it is just a little bit too late for me to dive into it, I will try to take a look tomorrow if I can, but my suggestion / concern is definitely non-blocking.

I see, ok thanks for the input and interest here 🙂

If it's just to make the tests happy then I would really strongly prefer to instead try to rework the tests not to use setTimeout at all, which is something I'd be completely happy to look into (as I mentioned before I already attempted that and it didn't look worth it, but given the issues you're seeing maybe it would be?)

WDYT? 🙂

What do you think of this solution? 🙂 ff336e5

the rest of the function would incorrectly run, for example emitting events such as resume
(this could cause subtle bugs with potential listeners of such events (rli.on('resume', () => {...})))

Since I mentioned it I figured it'd be nice to test this too 👀 f6b9d13

I think the solution is more robust indeed!

If it's just to make the tests happy then I would really strongly prefer to instead try to rework the tests not to use setTimeout at all

I think my concern was not only to make the tests happy but more from a user land perspective that .end for example:

child.stdin.write('await import(\'./message.mjs\');\n'); child.stdin.write('.exit'); child.stdin.end();

taken from test test/parallel/test-repl-import-referrer.js has an unpredictable behaviour. I've started a new CI and once its green let's kick started a CITGM and see the impact on the user land packages 👍

jakecastelli · 2025-04-06T13:42:02Z

Regarding your comment about setTimeout being called without a specified delay, would you like me to add the missing delays? (I can also increase them from 0 to say 100 or something, it can help avoiding potential future CI
flakiness?)

By default if no duration is passed in, setTimeout will default the duration to 1, I would suggest to be explicit e.g. setTimeout(yourFunc, 1) or any duration you think it is necessary, but as long as linter is happy, it is totally up to you 👍 I am pretty sure @aduh95 will be able to chip in more 😄

dario-piotrowicz · 2025-04-06T14:20:07Z

Regarding your comment about setTimeout being called without a specified delay, would you like me to add the missing delays? (I can also increase them from 0 to say 100 or something, it can help avoiding potential future CI
flakiness?)

By default if no duration is passed in, setTimeout will default the duration to 1, I would suggest to be explicit e.g. setTimeout(yourFunc, 1) or any duration you think it is necessary, but as long as linter is happy, it is totally up to you 👍 I am pretty sure @aduh95 will be able to chip in more 😄

I've added common.platformTimeout(100) as the missing delays 🙂
It might not be necessary, bu I don't think it hurts any since such delay is barely noticeable and on the plus side as I mentioned it could help in avoiding potential flakiness in the future 🙂

Co-authored-by: jakecastelli <[email protected]>

dario-piotrowicz · 2025-04-06T17:05:56Z

@jakecastelli no more setTimeouts in sight! 🙅

nodejs-github-bot · 2025-04-07T08:17:48Z

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

nodejs-github-bot · 2025-04-07T13:11:32Z

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

jakecastelli · 2025-04-09T04:50:01Z

Would you mind taking a look at the CI failure when you have time.

22:52:34 not ok 2831 parallel/test-repl-no-terminal
22:52:34   ---
22:52:34   duration_ms: 281.76900
22:52:34   severity: fail
22:52:34   exitcode: 1
22:52:34   stack: |-
22:52:34     > 
22:52:34     Error: Could not open history file.
22:52:34     REPL session history will not be persisted.
22:52:34     �[1G�[0J> �[3G
22:52:34     node:internal/event_target:1101
22:52:34       process.nextTick(() => { throw err; });
22:52:34                                ^
22:52:34     Error [ERR_USE_AFTER_CLOSE]: readline was closed
22:52:34         at REPLServer.resume (node:internal/readline/interface:555:13)
22:52:34         at oninit (node:internal/repl/history:81:12)
22:52:34         at FSReqCallback.oncomplete (node:fs:175:23)
22:52:34         at FSReqCallback.callbackTrampoline (node:internal/async_hooks:130:17) {
22:52:34       code: 'ERR_USE_AFTER_CLOSE'
22:52:34     }
22:52:34

let me know if you need to rerun the failure CI again

nodejs-github-bot added needs-ci PRs that need a full CI run. readline Issues and PRs related to the built-in readline module. labels Mar 29, 2025

dario-piotrowicz force-pushed the dario/readline-after-close-checks branch from c575cee to 7f5b064 Compare March 29, 2025 23:12

dario-piotrowicz changed the title ~~readline: add stricter validation called after closed~~ readline: add stricter validation for functions called after closed Mar 30, 2025

dario-piotrowicz force-pushed the dario/readline-after-close-checks branch from 7f5b064 to 360e127 Compare March 30, 2025 09:24

geeksilva97 reviewed Mar 31, 2025

View reviewed changes

jasnell approved these changes Apr 1, 2025

View reviewed changes

jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. needs-citgm PRs that need a CITGM CI run. labels Apr 1, 2025

jasnell requested review from mcollina and anonrig April 1, 2025 10:34

geeksilva97 approved these changes Apr 1, 2025

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

BridgeAR approved these changes Apr 3, 2025

View reviewed changes

jakecastelli reviewed Apr 4, 2025

View reviewed changes

test/parallel/test-readline-promises-tab-complete.js Outdated Show resolved Hide resolved

test/parallel/test-repl-import-referrer.js Outdated Show resolved Hide resolved

mcollina approved these changes Apr 4, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 4, 2025

anonrig approved these changes Apr 4, 2025

View reviewed changes

dario-piotrowicz requested a review from jakecastelli April 5, 2025 16:12

jakecastelli added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 6, 2025

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 6, 2025

This comment was marked as outdated.

Sign in to view

jakecastelli reviewed Apr 6, 2025

View reviewed changes

dario-piotrowicz and others added 4 commits April 6, 2025 17:14

readline: add stricter validation for functions called after closed

66ce01a

Update test/parallel/test-repl-import-referrer.js

18cd0cc

Co-authored-by: jakecastelli <[email protected]>

increase timeout

00202a1

add delays to setTimeouts

a104b57

dario-piotrowicz force-pushed the dario/readline-after-close-checks branch from 210cf04 to a104b57 Compare April 6, 2025 16:14

avoid setTimeout in test-repl-import-referrer.js

ff336e5

dario-piotrowicz force-pushed the dario/readline-after-close-checks branch from b571760 to ff336e5 Compare April 6, 2025 16:59

remove setTimeout calls (by replacing rli.close() with fi.end())

70eed9a

test that no resume nor pause events get emitted

f6b9d13

dario-piotrowicz force-pushed the dario/readline-after-close-checks branch from c4e42bd to f6b9d13 Compare April 6, 2025 18:19

jakecastelli removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 7, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

readline: add stricter validation for functions called after closed #57680

readline: add stricter validation for functions called after closed #57680

dario-piotrowicz commented Mar 29, 2025 •

edited

Loading

codecov bot commented Mar 30, 2025 •

edited

Loading

geeksilva97 Mar 31, 2025

dario-piotrowicz Mar 31, 2025 •

edited

Loading

geeksilva97 Apr 1, 2025

dario-piotrowicz Apr 1, 2025

geeksilva97 Apr 3, 2025

jasnell left a comment

dario-piotrowicz commented Apr 1, 2025

nodejs-github-bot commented Apr 3, 2025

mcollina left a comment

nodejs-github-bot commented Apr 4, 2025

jakecastelli commented Apr 5, 2025

dario-piotrowicz commented Apr 5, 2025 •

edited

Loading

jakecastelli commented Apr 6, 2025 •

edited

Loading

This comment was marked as outdated.

dario-piotrowicz commented Apr 6, 2025 •

edited

Loading

jakecastelli Apr 6, 2025

dario-piotrowicz Apr 6, 2025

jakecastelli Apr 6, 2025 •

edited

Loading

dario-piotrowicz Apr 6, 2025

dario-piotrowicz Apr 6, 2025 •

edited

Loading

dario-piotrowicz Apr 6, 2025 •

edited

Loading

jakecastelli Apr 7, 2025

jakecastelli commented Apr 6, 2025

dario-piotrowicz commented Apr 6, 2025

dario-piotrowicz commented Apr 6, 2025

nodejs-github-bot commented Apr 7, 2025

nodejs-github-bot commented Apr 7, 2025

jakecastelli commented Apr 9, 2025

readline: add stricter validation for functions called after closed #57680

Are you sure you want to change the base?

readline: add stricter validation for functions called after closed #57680

Conversation

dario-piotrowicz commented Mar 29, 2025 • edited Loading

codecov bot commented Mar 30, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

dario-piotrowicz Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell left a comment

Choose a reason for hiding this comment

dario-piotrowicz commented Apr 1, 2025

nodejs-github-bot commented Apr 3, 2025

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 4, 2025

jakecastelli commented Apr 5, 2025

dario-piotrowicz commented Apr 5, 2025 • edited Loading

jakecastelli commented Apr 6, 2025 • edited Loading

This comment was marked as outdated.

dario-piotrowicz commented Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakecastelli Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dario-piotrowicz Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

dario-piotrowicz Apr 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakecastelli commented Apr 6, 2025

dario-piotrowicz commented Apr 6, 2025

dario-piotrowicz commented Apr 6, 2025

nodejs-github-bot commented Apr 7, 2025

nodejs-github-bot commented Apr 7, 2025

jakecastelli commented Apr 9, 2025

dario-piotrowicz commented Mar 29, 2025 •

edited

Loading

codecov bot commented Mar 30, 2025 •

edited

Loading

dario-piotrowicz Mar 31, 2025 •

edited

Loading

dario-piotrowicz commented Apr 5, 2025 •

edited

Loading

jakecastelli commented Apr 6, 2025 •

edited

Loading

dario-piotrowicz commented Apr 6, 2025 •

edited

Loading

jakecastelli Apr 6, 2025 •

edited

Loading

dario-piotrowicz Apr 6, 2025 •

edited

Loading

dario-piotrowicz Apr 6, 2025 •

edited

Loading