Skip to content

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

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

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/internal/readline/interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -536,6 +536,9 @@ class Interface extends InterfaceConstructor {
* @returns {void | Interface}
*/
pause() {
if (this.closed) {
throw new ERR_USE_AFTER_CLOSE('readline');
}
if (this.paused) return;
this.input.pause();
this.paused = true;
Expand All @@ -548,6 +551,9 @@ class Interface extends InterfaceConstructor {
* @returns {void | Interface}
*/
resume() {
if (this.closed) {
throw new ERR_USE_AFTER_CLOSE('readline');
Copy link
Member

Choose a reason for hiding this comment

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

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');
});

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@jakecastelli jakecastelli Apr 6, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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? 🙂

Copy link
Member Author

@dario-piotrowicz dario-piotrowicz Apr 6, 2025

Choose a reason for hiding this comment

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

What do you think of this solution? 🙂 ff336e5

Copy link
Member Author

@dario-piotrowicz dario-piotrowicz Apr 6, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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 👍

}
if (!this.paused) return;
this.input.resume();
this.paused = false;
Expand All @@ -568,6 +574,9 @@ class Interface extends InterfaceConstructor {
* @returns {void}
*/
write(d, key) {
if (this.closed) {
throw new ERR_USE_AFTER_CLOSE('readline');
}
if (this.paused) this.resume();
if (this.terminal) {
this[kTtyWrite](d, key);
Expand Down
41 changes: 41 additions & 0 deletions test/parallel/test-readline-interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -1202,6 +1202,47 @@ for (let i = 0; i < 12; i++) {
fi.emit('data', 'Node.js\n');
}

// Call write after close
{
const [rli, fi] = getInterface({ terminal });
rli.question('What\'s your name?', common.mustCall((name) => {
assert.strictEqual(name, 'Node.js');
rli.close();
assert.throws(() => {
rli.write('I said Node.js');
}, {
name: 'Error',
code: 'ERR_USE_AFTER_CLOSE'
});
}));
fi.emit('data', 'Node.js\n');
}

// Call pause/resume after close
{
const [rli, fi] = getInterface({ terminal });
rli.question('What\'s your name?', common.mustCall((name) => {
assert.strictEqual(name, 'Node.js');
rli.close();
// No 'resume' nor 'pause' event should be emitted after close
rli.on('resume', common.mustNotCall());
rli.on('pause', common.mustNotCall());
assert.throws(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@dario-piotrowicz dario-piotrowicz Mar 31, 2025

Choose a reason for hiding this comment

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

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 😅)

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

As I said, not a blocker at all.

rli.pause();
}, {
name: 'Error',
code: 'ERR_USE_AFTER_CLOSE'
});
assert.throws(() => {
rli.resume();
}, {
name: 'Error',
code: 'ERR_USE_AFTER_CLOSE'
});
}));
fi.emit('data', 'Node.js\n');
}

// Can create a new readline Interface with a null output argument
{
const [rli, fi] = getInterface({ output: null, terminal });
Expand Down
2 changes: 1 addition & 1 deletion test/parallel/test-readline-promises-interface.js
Original file line number Diff line number Diff line change
Expand Up @@ -204,7 +204,7 @@ function assertCursorRowsAndCols(rli, rows, cols) {
fi.emit('data', character);
}
fi.emit('data', '\n');
rli.close();
fi.end();
}

// \t when there is no completer function should behave like an ordinary
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-readline-promises-tab-complete.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ if (process.env.TERM === 'dumb') {
output = '';
});
}
rli.close();
fi.end();
});
});
});
Expand Down Expand Up @@ -114,5 +114,5 @@ if (process.env.TERM === 'dumb') {
assert.match(output, /^Tab completion error: Error: message/);
output = '';
});
rli.close();
fi.end();
}
16 changes: 10 additions & 6 deletions test/parallel/test-repl-import-referrer.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,20 +8,24 @@ const args = ['--interactive'];
const opts = { cwd: fixtures.path('es-modules') };
const child = cp.spawn(process.execPath, args, opts);

let output = '';
const outputs = [];
child.stdout.setEncoding('utf8');
child.stdout.on('data', (data) => {
output += data;
outputs.push(data);
if (outputs.length === 3) {
// All the expected outputs have been received
// so we can close the child process's stdin
child.stdin.end();
}
});

child.on('exit', common.mustCall(() => {
const results = output.replace(/^> /mg, '').split('\n').slice(2);
assert.deepStrictEqual(
const results = outputs[2].split('\n')[0];
assert.strictEqual(
results,
['[Module: null prototype] { message: \'A message\' }', '']
'[Module: null prototype] { message: \'A message\' }'
);
}));

child.stdin.write('await import(\'./message.mjs\');\n');
child.stdin.write('.exit');
child.stdin.end();
13 changes: 9 additions & 4 deletions test/parallel/test-repl-no-terminal.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
'use strict';
const common = require('../common');

const ArrayStream = require('../common/arraystream');
const repl = require('repl');
const r = repl.start({ terminal: false });
r.setupHistory('/nonexistent/file', common.mustSucceed());
process.stdin.unref?.();

const stream = new ArrayStream();

const replServer = repl.start({ terminal: false, input: stream, output: stream });

replServer.setupHistory('/nonexistent/file', common.mustSucceed(() => {
replServer.close();
}));
2 changes: 1 addition & 1 deletion test/parallel/test-repl-uncaught-exception-async.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ r.write(
' throw new RangeError("abc");\n' +
'}, 1);console.log()\n'
);
r.close();

setTimeout(() => {
r.close();
const len = process.listenerCount('uncaughtException');
process.removeAllListeners('uncaughtException');
assert.strictEqual(len, 0);
Expand Down
Loading