Skip to content
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

readline: fix freeze if `keypress` event throws #2107

Closed
wants to merge 1 commit into from

Conversation

@rlidwka
Copy link
Contributor

commented Jul 5, 2015

The issue is described here: #1968

I consider keypress event in readline a public api, and if such a handler throws, readline should continue to work normally.

Basically, I see are two good ways to deal with it:

  1. restart the generator when it errors out
  2. throw an error in the next tick

Since everything in readline is synchronous now, adding nextTick would break things. So I went with option 1.

This PR fixes a particular bug. But avoiding throwing in repl might still be a good idea. So PR #1968 might still make sense, but as a refactor, not a bugfix (I'll let someone else to review it though).

// cc: @monsanto

readline: fix freeze if `keypress` event throws
`emitKeys` is a generator which emits `keypress` events in an infinite
loop. But if `keypress` event handler throws, the error stops the loop,
leaving generator in a broken state. So this patch restarts the generator
when an error occures.

@mscdex mscdex added the readline label Jul 5, 2015

@monsanto

This comment has been minimized.

Copy link
Contributor

commented Jul 6, 2015

I agree that if keypress is a public API it should not error out. It should be documented though.

LGTM

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Jul 7, 2015

LGTM.

rlidwka added a commit that referenced this pull request Jul 11, 2015
readline: fix freeze if `keypress` event throws
`emitKeys` is a generator which emits `keypress` events in an infinite
loop. But if `keypress` event handler throws, the error stops the loop,
leaving generator in a broken state. So this patch restarts the generator
when an error occures.

PR-URL: #2107
Reviewed-By: Christopher Monsanto <chris@monsan.to>
Reviewed-By: Jeremiah Senkpiel <fishrock123@rocketmail.com>
@rlidwka

This comment has been minimized.

Copy link
Contributor Author

commented Jul 11, 2015

landed in bd01603

@rlidwka rlidwka closed this Jul 11, 2015

Fishrock123 added a commit to Fishrock123/node that referenced this pull request Jul 17, 2015
2015-07-17 io.js v2.4.0 Release
Notable changes

* src: Added a new `--track-heap-objects` flag to track heap object
allocations for heap snapshots (Bradley Meck)
nodejs#2135.
* readline: Fixed a freeze that affected the repl if the keypress event
handler threw (Alex Kocharin) nodejs#2107.
* npm: Upgraded to v2.13.0, release notes can be found in
https://github.com/npm/npm/releases/tag/v2.13.0 (Forrest L Norvell)
nodejs#2152.

PR-URL: nodejs#2189
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.