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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

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
@addaleax addaleax mentioned this pull request Jun 8, 2017
Closed
@refack refack added the regression Issues related to regressions. label Jun 8, 2017
@addaleax
Copy link
Member Author

addaleax commented Jun 8, 2017

CI: https://ci.nodejs.org/job/node-test-commit/10460/

@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. ;)

@gibfahn gibfahn mentioned this pull request Jun 8, 2017
Closed
3 tasks
@ilovezfs ilovezfs mentioned this pull request Jun 8, 2017
Closed
4 tasks
@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

@addaleax addaleax mentioned this pull request Jun 9, 2017
Closed
13 tasks
@benjamn benjamn mentioned this pull request Jun 9, 2017
Merged
11 tasks
@mafintosh mafintosh mentioned this pull request Jun 9, 2017
Closed
@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
Closed
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
@addaleax
Revert "readline: clean up event listener in onNewListener"
c6f074f 
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
@addaleax
test: add regression test for 13557
871e4d0 
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
@addaleax
Revert "readline: clean up event listener in onNewListener"
0df6c0b 
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
@addaleax
test: add regression test for 13557
2ea529b 
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
@addaleax
2017-06-12, Version 8.1.1 (Current)
e1b2a17 
* **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
Merged
@davej davej mentioned this pull request Jun 11, 2017
Closed
davej added a commit to pingyhq/pingy-cli that referenced this pull request Jun 11, 2017
@davej
Test Node 8.0 only. Node 8.1 is broken because of this issue: nodejs/…
e241b63 
…node#13560
@elgrafico elgrafico mentioned this pull request Jun 12, 2017
Closed
addaleax added a commit that referenced this pull request Jun 12, 2017
@addaleax
2017-06-12, Version 8.1.1 (Current)
2b0a8bd 
* **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
@addaleax
2017-06-12, Version 8.1.1 (Current)
e90ad78 
* **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
@addaleax
2017-06-13, Version 8.1.1 (Current)
c7f35e2 
* **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
@addaleax @MylesBorins
2017-06-13, Version 8.1.1 (Current)
3e1492a 
* **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
@mscdex mscdex mentioned this pull request Jun 13, 2017
Closed
@kotarac kotarac mentioned this pull request Jun 13, 2017
Closed
jasnell pushed a commit that referenced this pull request Jun 13, 2017
@addaleax @jasnell
2017-06-13, Version 8.1.1 (Current)
9dc3f93 
* **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
Revert "Work around a Node 8.1.0 regression that broke meteor login."
854f113 
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
Revert "Work around a Node 8.1.0 regression that broke meteor login."
7f44db9 
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
Closed
3 tasks
@MylesBorins
Copy link
Contributor

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

@robinhouston robinhouston mentioned this pull request Aug 1, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@cjihrig cjihrig cjihrig approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
readline Issues and PRs related to the built-in readline module. regression Issues related to regressions.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@addaleax @jasnell @gibfahn @Pajn @lpinca @princejwesley @MylesBorins @refack @cjihrig @nodejs-github-bot