Skip to content

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

Closed
kaelzhang wants to merge 1 commit intonodejs:masterfrom
kaelzhang:master
Closed

stream: fix removeAllListeners() for Stream.Readable#20924
kaelzhang wants to merge 1 commit intonodejs:masterfrom
kaelzhang:master

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
Contributor

@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
Copy link
Contributor

@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

@kaelzhang kaelzhang May 24, 2018

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
Copy link
Contributor

@ryzokuken ryzokuken May 24, 2018

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

@kaelzhang kaelzhang May 24, 2018

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
Contributor

apapirovski commented May 24, 2018

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

BridgeAR commented May 24, 2018

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

mcollina commented May 28, 2018

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

@BridgeAR
Copy link
Member

BridgeAR commented May 28, 2018

@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

mcollina commented May 28, 2018

@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
Contributor

apapirovski commented May 28, 2018

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

mcollina commented May 28, 2018

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

mcollina commented May 28, 2018

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

@mcollina
Copy link
Member

mcollina commented May 28, 2018

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

@mcollina mcollina mcollina approved these changes

@lpinca lpinca lpinca approved these changes

+2 more reviewers

@ryzokuken ryzokuken ryzokuken left review comments

@apapirovski apapirovski apapirovski approved these changes

Reviewers whose approvals may not affect merge requirements

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