Readline bugfix, and proposal. #2737

Closed
wants to merge 1 commit into
from

Projects

None yet

2 participants

Before editing readline.js, I was having problems with closing a readline interface. The interface would "close", but would still trigger the rl.on('line', ...) event, and also echoing the command that was being input.

I notice that rl.close() only emulates closing. It doesn't actually destroy the input with input.destroy(). I left it emulating closing for the proposal.

The proposal: since rl.close() is only emulating closing, and doesn't actually destroy the input, the input still keeps the program open. I added code into rl.resume() to resume the readline without having to run rl.createInterface() again.

However, the problem with doing this is (as I pointed out) the readline makes the program hang from not destroying the input. So, I would also propose that the input be destroyed, but rl.resume() run rl.createInterface() with the input, output, and completer that was already provided.

Already spoke about this with @isaacs and @bnoordhuis, but wanted to see what everyone else has to say/add.

@indutny indutny was assigned Feb 11, 2012
Owner
indutny commented Feb 12, 2012

Can you please add a regression test for your Pull Request?

Owner
indutny commented Feb 12, 2012

Otherwise, lgtm

Actually, instead of having to destroy the input.. pausing would work. Working on adding a few other checks and things in as well, while I'm at it. Will update the repo sometime later.

Going to close this request. Will open another when finished.

@Southern Southern closed this Feb 12, 2012
@Southern Southern added a commit to Southern/node that referenced this pull request 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 pull request 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
@isaacs isaacs added a commit to isaacs/node-v0.x-archive that referenced this pull request 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 pull request 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