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

Revert "readline: clean up event listener in onNewListener" #13560

Closed
wants to merge 2 commits into from

Conversation

addaleax
Copy link
Member

@addaleax addaleax commented Jun 8, 2017

This reverts PR #13266 and adds a regression test for #13557. After thinking about it for a bit, my patch proposed in the issue is not the right approach; #13266 was actually incorrect and should be undone.

The interface instance passed to emitKeypressEvents() is explicitly not for decoder lifetime control; it is just used to toggle tab completion on it. We provide emitKeypressEvents() as a standalone method as well, which should work on its own exactly as documented (i.e. independently of any readline interfaces by default).

My initial suggestion of removing the keypress decoder from the stream completely might leave partial reads in the string decoder unread, which we also want to avoid.

/cc @iarna @gibfahn

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

readline

@addaleax addaleax added the readline Issues and PRs related to the built-in readline module. label Jun 8, 2017
@nodejs-github-bot nodejs-github-bot added the readline Issues and PRs related to the built-in readline module. label Jun 8, 2017
@refack refack added the regression Issues related to regressions. label Jun 8, 2017
@addaleax
Copy link
Member Author

addaleax commented Jun 8, 2017

@jasnell
Copy link
Member

jasnell commented Jun 8, 2017

Ugh. Ok. Lgtm

@gibfahn
Copy link
Member

gibfahn commented Jun 8, 2017

Sorry for breaking stuff 😞

@addaleax
Copy link
Member Author

addaleax commented Jun 8, 2017

@gibfahn Don’t worry about it, it really isn’t obvious that that would be a breaking change. This will go out with the next 8.x release and the world will keep turning. ;)

@jasnell
Copy link
Member

jasnell commented Jun 9, 2017

Breaking stuff happens. As @addaleax said, this one is super non-obvious and, on its face, not an incorrect change. This largely underscores the importance of growing our ecosystem testing, tho

@Pajn
Copy link

Pajn commented Jun 9, 2017

When will a new version be released?
Nodesource only hosts the latest one so rolling back is a PITA

@legodude17 legodude17 mentioned this pull request Jun 9, 2017
12 tasks
@addaleax
Copy link
Member Author

@Pajn That’s mostly a question for @nodejs/release but I would assume Monday or Tuesday. I should be able to prepare it that helps.

@Pajn
Copy link

Pajn commented Jun 10, 2017

@addaleax Thanks! Just knowing the timeframe is helpful.

@addaleax
Copy link
Member Author

I’m going to land this a bit later today if nobody objects.

@lpinca
Copy link
Member

lpinca commented Jun 10, 2017

@gibfahn I also broke stuff #13435 if that makes you feel better.

@princejwesley
Copy link
Contributor

@gibfahn Sorry if its started because of my comment!

@jasnell
Copy link
Member

jasnell commented Jun 10, 2017

If no one else has started the release proposal by Monday I'll kick it off with an eye towards releasing on Tuesday.

@addaleax
Copy link
Member Author

Landed in 2529119...871e4d0

@addaleax addaleax closed this Jun 10, 2017
@addaleax addaleax deleted the fix-readline-close branch June 10, 2017 15:43
addaleax added a commit that referenced this pull request Jun 10, 2017
This reverts commit 81ddeb9.

Ref: #13266
PR-URL: #13560
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Jun 10, 2017
Fixes: #13557
PR-URL: #13560
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Jun 10, 2017
This reverts commit 81ddeb9.

Ref: #13266
PR-URL: #13560
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Jun 10, 2017
Fixes: #13557
PR-URL: #13560
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax added a commit that referenced this pull request Jun 10, 2017
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)
@addaleax addaleax mentioned this pull request Jun 10, 2017
davej added a commit to pingyhq/pingy-cli that referenced this pull request Jun 11, 2017
addaleax added a commit that referenced this pull request Jun 12, 2017
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
addaleax added a commit that referenced this pull request Jun 12, 2017
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
addaleax added a commit that referenced this pull request Jun 12, 2017
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
MylesBorins pushed a commit that referenced this pull request Jun 13, 2017
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
jasnell pushed a commit that referenced this pull request Jun 13, 2017
* **Child processes**
  * `stdout` and `stderr` are now available on the error output of a
    failed call to the `util.promisify()`ed version of
    `child_process.exec`.
    [[`d66d4fc94c`](d66d4fc94c)]
    [#13388](#13388)

* **HTTP**
  * A regression that broke certain scenarios in which HTTP is used together
    with the `cluster` module has been fixed.
    [[`fff8a56d6f`](fff8a56d6f)]
    [#13578](#13578)

* **HTTPS**
  * The `rejectUnauthorized` option now works properly for unix sockets.
    [[`c4cbd99d37`](c4cbd99d37)]
    [#13505](#13505)

* **Readline**
  * A change that broke `npm init` and other code which uses `readline`
    multiple times on the same input stream is reverted.
    [[`0df6c0b5f0`](0df6c0b5f0)]
    [#13560](#13560)

PR-URL: #13598
benjamn pushed a commit to meteor/meteor that referenced this pull request Jun 14, 2017
This reverts commit dd11432.

This bug that necessitated this workaround was fixed by
nodejs/node#13560, as mentioned in
https://nodejs.org/en/blog/release/v8.1.1/.
benjamn pushed a commit to meteor/meteor that referenced this pull request Jun 14, 2017
This reverts commit dd11432.

This bug that necessitated this workaround was fixed by
nodejs/node#13560, as mentioned in
https://nodejs.org/en/blog/release/v8.1.1/.
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@MylesBorins
Copy link
Contributor

Original commit did not land, marking this as dont-land for v6.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
readline Issues and PRs related to the built-in readline module. regression Issues related to regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants