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

stream: fix removeAllListeners() for Stream.Readable #20924

Closed
wants to merge 1 commit into from
Closed

stream: fix removeAllListeners() for Stream.Readable #20924

wants to merge 1 commit into from

Conversation

kaelzhang
Copy link
Contributor

@kaelzhang kaelzhang commented May 24, 2018 •
edited
Loading

Refs: #20923

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label May 24, 2018
apapirovski
apapirovski approved these changes May 24, 2018
View reviewed changes
Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but we could make the condition simpler (or, well, get rid of it).

lib/_stream_readable.js Outdated
@@ -846,7 +846,9 @@ Readable.prototype.removeListener = function(ev, fn) {
};

Readable.prototype.removeAllListeners = function(ev) {
const res = Stream.prototype.removeAllListeners.call(this, ev);
const res = arguments.length === 0 ?
Copy link
Member

@apapirovski apapirovski May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, could you do this instead: Stream.prototype.removeAllListeners.apply(this, arguments)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right.

@kaelzhang kaelzhang force-pushed the master branch 2 times, most recently from 771108d to 24320ec Compare May 24, 2018 03:49
@mscdex
Copy link
Contributor

mscdex commented May 24, 2018 •
edited
Loading

Perhaps a better fix might be to change removeAllListeners() instead (although would be semver-major)?

ryzokuken
ryzokuken reviewed May 24, 2018
View reviewed changes
Copy link
Contributor

@ryzokuken ryzokuken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, going forward, but I wasn't really satisfied by the unit test. Could you make the changes I asked for?

test/parallel/test-stream-readable-event.js Outdated
{
const r = new Readable();

r.removeAllListeners();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you remove this line right here? Do the tests still pass? They probably should because we never added any listeners to begin with. Could you try adding a few listeners, assert that r.eventNames() is non-zero, call this line and then assert it's zero please?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK，I'll fix it

Copy link
Contributor Author

@kaelzhang kaelzhang May 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the test a little bit. Any suggestions?

@apapirovski
Copy link
Member

Perhaps a better fix might be to change removeAllListeners() instead (although would be semver-major)?

I don't think EE#removeAllListeners is doing anything wrong though, is it? Right now undefined would be converted to string (so undefined event) in both addListener and removeListener. This behaviour has been around since Node's beginning, afaik.

@mscdex
Copy link
Contributor

mscdex commented May 24, 2018

Right now undefined would be converted to string (so undefined event) in both addListener and removeListener. This behaviour has been around since Node's beginning, afaik.

Which is why I said it'd be semver-major. I can't imagine anyone relying on undefined being converted to 'undefined' though.

@BridgeAR
Copy link
Member

I agree with @mscdex that it would be best to change addListener and removeListener. Relying on undefined would be very surprising.

mcollina
mcollina approved these changes May 28, 2018
View reviewed changes
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mcollina
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/15125/

@BridgeAR
Copy link
Member

@apapirovski @mcollina @lpinca is anyone of you strongly in favor of this over just fixing not converting undefined to 'undefined' in the underlying function? I definitely think that would be the better solution even though it is semver-major.

@mcollina
Copy link
Member

@BridgeAR this fix a bad bug which could lead a potential memory leak. If you want to change the underlining function in a semver-major might be a good call to avoid this in the future.

@apapirovski
Copy link
Member

This should just land barring the CI being red. I'm whatever on the other change... if people want to, fine by me but I don't really care either way.

@mcollina
Copy link
Member

CI failures are unrelated. This can land.

@mcollina mcollina added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 28, 2018
@mcollina
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/15131/

@mcollina
Copy link
Member

Landed as 9f4bf4c.

@mcollina mcollina closed this May 28, 2018
mcollina pushed a commit that referenced this pull request May 28, 2018
@kaelzhang @mcollina
stream: fix removeAllListeners() for Stream.Readable
9f4bf4c 
Fixes: #20923

PR-URL: #20924
Refs: #20923
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request May 29, 2018
@kaelzhang @MylesBorins
stream: fix removeAllListeners() for Stream.Readable
8f52c3f 
Fixes: #20923

PR-URL: #20924
Refs: #20923
Reviewed-By: Anatoli Papirovski <apapirovski@mac.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@MylesBorins MylesBorins mentioned this pull request May 29, 2018
Merged
@mcollina mcollina added the notable-change PRs with changes that should be highlighted in changelogs. label May 29, 2018
MylesBorins added a commit that referenced this pull request May 29, 2018
@MylesBorins
2018-05-29, Version 10.3.0 (Current)
b68b1df 
Notable Changes:

* **deps**:
  - upgrade npm to 6.1.0 (Rebecca Turner)
    #20190
* **fs**:
  - fix reads with pos \> 4GB (Mathias Buus)
    #21003
* **net**:
  - new option to allow IPC servers to be readable and writable
    by all users (Bartosz Sosnowski)
    #19472
* **stream**:
  - fix removeAllListeners() for Stream.Readable to work as expected
    when no arguments are passed (Kael Zhang)
    #20924
* **Added new collaborators**
  - John-David Dalton (https://github.com/jdalton)

PR-URL: #21011
MylesBorins added a commit that referenced this pull request May 29, 2018
@MylesBorins
2018-05-29, Version 10.3.0 (Current)
c1dae43 
Notable Changes:

* **deps**:
  - upgrade npm to 6.1.0 (Rebecca Turner)
    #20190
* **fs**:
  - fix reads with pos \> 4GB (Mathias Buus)
    #21003
* **net**:
  - new option to allow IPC servers to be readable and writable
    by all users (Bartosz Sosnowski)
    #19472
* **stream**:
  - fix removeAllListeners() for Stream.Readable to work as expected
    when no arguments are passed (Kael Zhang)
    #20924
* **Added new collaborators**
  - John-David Dalton (https://github.com/jdalton)

PR-URL: #21011
MylesBorins added a commit that referenced this pull request May 29, 2018
@MylesBorins
2018-05-29, Version 10.3.0 (Current)
9ae658e 
Notable Changes:

* **deps**:
  - upgrade npm to 6.1.0 (Rebecca Turner)
    #20190
* **fs**:
  - fix reads with pos \> 4GB (Mathias Buus)
    #21003
* **net**:
  - new option to allow IPC servers to be readable and writable
    by all users (Bartosz Sosnowski)
    #19472
* **stream**:
  - fix removeAllListeners() for Stream.Readable to work as expected
    when no arguments are passed (Kael Zhang)
    #20924
* **Added new collaborators**
  - John-David Dalton (https://github.com/jdalton)

PR-URL: #21011
daviwil added a commit to atom/atom that referenced this pull request Jan 17, 2019
@daviwil
Remove listeners for specific events in Task class
aa00904 
This change is necessary for Electron 3 to a bug in Node.js 10.2.x which causes
the `removeAllListeners` method to no longer remove all listeners for any event
name when no argument is passed:

nodejs/node#20924

This issue has been fixed in Node.js 10.3.0+ so we will have to remove this
workaround when we move to Electron 4.0 to avoid future event handler leaks.
daviwil added a commit to atom/atom that referenced this pull request Jan 18, 2019
@daviwil
Remove listeners for specific events in Task class
7b7fd7b 
This change is necessary for Electron 3 to a bug in Node.js 10.2.x which causes
the `removeAllListeners` method to no longer remove all listeners for any event
name when no argument is passed:

nodejs/node#20924

This issue has been fixed in Node.js 10.3.0+ so we will have to remove this
workaround when we move to Electron 4.0 to avoid future event handler leaks.
rafeca pushed a commit to atom/atom that referenced this pull request May 6, 2019
@daviwil @rafeca
Remove listeners for specific events in Task class
cc7a1d0 
This change is necessary for Electron 3 to a bug in Node.js 10.2.x which causes
the `removeAllListeners` method to no longer remove all listeners for any event
name when no argument is passed:

nodejs/node#20924

This issue has been fixed in Node.js 10.3.0+ so we will have to remove this
workaround when we move to Electron 4.0 to avoid future event handler leaks.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ryzokuken ryzokuken ryzokuken left review comments

@lpinca lpinca lpinca approved these changes

@mcollina mcollina mcollina approved these changes

@apapirovski apapirovski apapirovski approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. notable-change PRs with changes that should be highlighted in changelogs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@kaelzhang @mscdex @apapirovski @BridgeAR @mcollina @lpinca @ryzokuken @nodejs-github-bot