readline: add questionCancel() method to cancel ongoing question

[mattiasrunge]

In some cases a question asked needs to be canceled. For instance
it might be desirable to cancel a question when a user presses
ctrl+c and triggers the SIGINT event.
Also an initial empty string was set for this.line since the
cursor methods fail if line is not initialized. This happend
in the tests when a question is asked and canceled before any
input has been received which would set line to a string. This
is due to questionCancel calling clearline.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[mattiasrunge]

This would probably have been nicer with a promise-based solution where the question-method is async and returns a promise which is rejected if the question is canceled. But the proposed solution has no impact on the existing interface which I guess is desirable.

[addaleax]

But the proposed solution has no impact on the existing interface which I guess is desirable.

@mattiasrunge … fully agree on that. I think with the current interface, this is the best we can do.

There are two things that maybe don’t have to be answered by this PR, but that you should be aware of:

• We are considering adding AbortController support to Node.js. My understanding is that this kind of situation is exactly what it would be used for. I’m not saying this feature should wait for that to land, but maybe it’s worth a thought?
• I agree about the Promise concern. There is no promisified version of question(), but it would sure be nice to have something that has the behavior you described.

[devsnek]

cancellation concerns and design considerations for node core apis: https://docs.google.com/document/d/1OA3LVQWj7StZSL9F8zuITEssm1gB8Yws4jwT-vOHWRg/edit#

[mattiasrunge]

@addaleax I was not aware of the AbortController discussion but have read it now, thank you. Personally I think it is a bit verbose for simple things :) but I absolutely see the benefit of having a common solution which is based on a specification.

So maybe this PR should wait/be closed if it probably would be deprecated as soon as the AbortController is merged?

Should I put the effort into enabling the question method to be promisified as a preparation for the AbortController?

[lundibundi]

I think a simple questionCancel method would be better here instead of using AbortController. IMO AbortController is for things more complex in nature and it would add unnecessary complexity for such a simple use case. (also it's not a Promise based API)
Though I didn't have much time to investigative AbortController more in-depth so perhaps I'm missing something.

We could probably add a new Promise-based API with AbortController support if desired though I don't think there is much need for it.

[lundibundi]

ping @nodejs/readline

[lundibundi]

This LGTM but I want someone more familiar with readline to approve before this lands.

[jasnell]

I'd prefer to hold off on this until the AbortController lands. The existing API can then be extended to allow the optional AbortController to be passed in:

const ac = new AbortController()
rl.question('what?', ac, callback)
ac.abort()

[benjamingr]

I think it's probably worth to wait until AbortController lands and add support with AbortController rather than another way to cancel something.

Presumably that would be:

const controller = new AbortController();
r1.question('question', callback, { signal: controller.signal });
controller.cancel();

the actual use case itself sounds reasonable.

@devsnek I've been slacking on that document and got sucked into EventTarget stuff / work. I do intend to continue it but feel free to pick some parts up. I got stuck when I got to FS and had to play around with the code/PR to figure out what to do.

[lundibundi]

@mattiasrunge I think this can now proceed with integrating AbortController into rl.question(). I think we can just add optional second argument object options to the rl.question(query, options, cb) with signal from AbortController in it.

[mattiasrunge]

@lundibundi: I have now utilized the AbortController as per your suggestion.

[benjamingr]

Thanks! and thanks for following through and bearing with us 🙏

I think this behaviour (of rejecting in the promisified version but not the callback version) is very weird but I don't think we can do better given rl.question is just weird like that - and honestly people will likely use the promisified version anyway.

[benjamingr]

cc @nodejs/readline

[benjamingr]

I want to try @nodejs/readline one more time :]

[codecov-io]

Codecov Report

Merging #33676 into master will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master   #33676      +/-   ##
==========================================
- Coverage   96.41%   96.40%   -0.02%     
==========================================
  Files         220      220              
  Lines       73681    73719      +38     
==========================================
+ Hits        71041    71069      +28     
- Misses       2640     2650      +10     

Impacted Files	Coverage Δ
lib/internal/errors.js	97.97% <100.00%> (+<0.01%)	⬆️
lib/readline.js	96.74% <100.00%> (+0.09%)	⬆️
lib/fs.js	94.13% <0.00%> (-0.28%)	⬇️
lib/_http_server.js	98.34% <0.00%> (-0.21%)	⬇️
lib/internal/util/inspect.js	96.05% <0.00%> (-0.10%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9fd6122...87d694c. Read the comment docs.

[antsmartian]

we should probably not attach these stuff into the prototype anymore. This is something, we had discussion last time in changing the readline/repo codebase. Thoughts cc @BridgeAR

[mattiasrunge]

I can change it but may need a pointer to how you want it. Changing the whole readline to not add stuff to the prototype seems like out of scope for this pull-request to me. If you tell me how you want it I could do another pull-request to change that if you wish.

[PoojaDurgad]

@mattiasrunge - need a rebase due to git conflicts

[mattiasrunge]

@benjamingr I have tried to do the changes you wanted. Unsure if I used the correct way of not having _questionCancel on the prototype, but used the same approach as _ttyWriteDumb.

[nodejs-github-bot]

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

[mattiasrunge]

I have done a rebase now, is there anything more for me to do to get this in?

[nodejs-github-bot]

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

[benjamingr]

I'll try landing this manually since our automated land/build from GitHub infra is apparently broken.

[benjamingr]

Landed in 19981ed

Thanks a lot and thank you for your patience and work and re-making this API with AbortController 🎉