Follow up: Readline proposal #2756

Closed
Southern opened this Issue Feb 15, 2012 · 3 comments

Comments

Projects
None yet
2 participants

This is a follow up for #2737.

I have talked with @indutny, and we have agreed on the following as changes for the readline module.

Closing the input with this.input.destroy() would not be sufficient to solve the problem with the readline module hanging when it's closed. This is because returning a new interface from rl.resume() would cause users to have to reassign the interface. For example: var rl = rl.resume(); rl.prompt();, instead of rl.prompt() (where prompt would cause the stream to resume on its own).

We think it would be best to get rid of rl.close() altogether, and instead use only rl.pause() and rl.resume(). rl.pause() and rl.resume() will also emit a pause and resume event, in case users may want to trigger some functionality when the stream is paused or resumed.

This will not effect a user wanting to catch the SIGINT event when running readline, as readline already emits a SIGINT event if there is one present. If there is not a SIGINT event to be emitted, it then defaults to attempting to close the stream (which will be changed to a pause in the proposal).

Feedback, disagreements, or commentary welcome.

/cc: @isaacs @bnoordhuis

Owner

indutny commented Feb 15, 2012

@Southern can you please put this on nodejs email list too, I want to see if it changes things for someone

Sure.

Edit: Added.

@Southern Southern added a commit to Southern/node that referenced this issue Feb 16, 2012

@Southern Southern Readline proposal and bugfixes.
Related: #2737 #2756

- Removed extra newline from .question(); Users can input a
  newline if it they require it.
- Removed .close() due to it only emulating closing, causing a bug where
  readline is left open to trigger events such as .on('line', ...').
- Removed ._attemptClose()
- .pause() now triggers event .on('pause', ...)
- .resume() now triggers event .on('resume', ...)
- CTRL-C (SIGINT) in readline will now default to .pause() if no SIGINT event
  is present.
- CTRL-D (delete right) will also default to .pause() if there is nothing to
  delete (signaling the end of the file).
- Added new event `SIGTSTP`
- Added new event `SIGCONT`
- Added `resume` to `write` to resume the stream if paused.
- Docs updated.
- Updated repl.js
a0f7221

@indutny indutny added a commit that referenced this issue Feb 16, 2012

@Southern @indutny Southern + indutny Readline proposal and bugfixes. Related: #2737 #2756
- Removed extra newline from .question(); Users can input a
  newline if it they require it.
- Removed .close() due to it only emulating closing, causing a bug where
  readline is left open to trigger events such as .on('line', ...').
- Removed ._attemptClose()
- .pause() now triggers event .on('pause', ...)
- .resume() now triggers event .on('resume', ...)
- CTRL-C (SIGINT) in readline will now default to .pause() if no SIGINT event
  is present.
- CTRL-D (delete right) will also default to .pause() if there is nothing to
  delete (signaling the end of the file).
- Added new event `SIGTSTP`
- Added new event `SIGCONT`
- Added `resume` to `write` to resume the stream if paused.
- Docs updated.
- Updated repl.js
ce48579

Implemented as of #2757.

Southern closed this Feb 16, 2012

@isaacs isaacs added a commit to isaacs/node-v0.x-archive that referenced this issue Feb 23, 2012

@isaacs isaacs 2012.02.23, Version 0.7.5 (unstable)
* startup speed improvements (Maciej Małecki)

* crypto: add function getDiffieHellman() (Tomasz Buchert)

* buffer: support decoding of URL-safe base64 (Ben Noordhuis)

* Make QueryString.parse() even faster (Brian White)

* url: decode url entities in auth section (Ben Noordhuis)

* http: support PURGE request method (Ben Noordhuis)

* http: Generate Date headers on responses (Mark Nottingham)

* Fix #2762: Add callback to close function. (Mikeal Rogers)

* dgram: fix out-of-bound memory read (Ben Noordhuis)

* repl: add automatic loading of built-in libs (Brandon Benvie)

* repl: remove double calls where possible (Fedor Indutny)

* Readline improvements. Related: #2737 #2756 (Colton Baker)

* build: support shared V8 properly (T.C. Hollingsworth)

* build: disable -fomit-frame-pointer on solaris (Dave Pacheco)

* build: arch detection improvements (Nathan Rajlich)

* build: Make a fat binary for the OS X `make pkg`. (Nathan Rajlich)

* jslint src/ and lib/ on 'make test' (isaacs)
6ce41cb

@isaacs isaacs added a commit that referenced this issue Feb 23, 2012

@isaacs isaacs 2012.02.23, Version 0.7.5 (unstable)
* startup speed improvements (Maciej Małecki)

* crypto: add function getDiffieHellman() (Tomasz Buchert)

* buffer: support decoding of URL-safe base64 (Ben Noordhuis)

* Make QueryString.parse() even faster (Brian White)

* url: decode url entities in auth section (Ben Noordhuis)

* http: support PURGE request method (Ben Noordhuis)

* http: Generate Date headers on responses (Mark Nottingham)

* Fix #2762: Add callback to close function. (Mikeal Rogers)

* dgram: fix out-of-bound memory read (Ben Noordhuis)

* repl: add automatic loading of built-in libs (Brandon Benvie)

* repl: remove double calls where possible (Fedor Indutny)

* Readline improvements. Related: #2737 #2756 (Colton Baker)

* build: disable -fomit-frame-pointer on solaris (Dave Pacheco)

* build: arch detection improvements (Nathan Rajlich)

* build: Make a fat binary for the OS X `make pkg`. (Nathan Rajlich)

* jslint src/ and lib/ on 'make test' (isaacs)
d384b8b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment