-
-
Notifications
You must be signed in to change notification settings - Fork 31.9k
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
Changes from all commits
a34766a
6ef6024
917ecbe
64b9744
78e0c38
c5cae2e
242de21
e388d3b
d46db9f
425b7db
f5a038b
cd44076
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(() => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we can have the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are you asking if we could move 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? I am not extremely against the idea but yeah it wouldn't be my personal preference 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 }); | ||
|
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(); | ||
})); |
There was a problem hiding this comment.
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:There was a problem hiding this comment.
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:
resume
(this could cause subtle bugs with potential listeners of such events (
rli.on('resume', () => {...})
))try { rli.resume(); } catch { ... }
)Both of the above seem pretty incorrect behaviors to me 🤔
What would the benefit of deferring be?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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 thenextTick
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 to400ms
to pass the test, so I am not too sure if300ms
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.
There was a problem hiding this comment.
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? 🙂
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
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
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I mentioned it I figured it'd be nice to test this too 👀 f6b9d13
There was a problem hiding this comment.
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!
I think my concern was not only to make the tests happy but more from a user land perspective that
.end
for example: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 👍