Skip to content

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

Closed
kaelzhang wants to merge 1 commit into
nodejs:masterfrom
kaelzhang:master
Closed

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

Conversation

@kaelzhang

@kaelzhangkaelzhang commented May 24, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

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-botnodejs-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

@apapirovskiapapirovski left a comment

Copy link
Copy Markdown
Contributor

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

Comment threadlib/_stream_readable.js Outdated

@apapirovskiapapirovskiMay 24, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

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)?

@kaelzhangkaelzhangMay 24, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

You're right.

@kaelzhang
kaelzhangforce-pushed the master branch 2 times, most recently from 771108d to 24320ecCompareMay 24, 2018 03:49
@mscdex

mscdex commented May 24, 2018•
edited
Loading

Copy link
Copy Markdown
Contributor

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

ryzokuken
ryzokuken reviewed May 24, 2018
View reviewed changes

@ryzokukenryzokuken left a comment

Copy link
Copy Markdown
Contributor

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?

Comment threadtest/parallel/test-stream-readable-event.js Outdated

@ryzokukenryzokukenMay 24, 2018

Copy link
Copy Markdown
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?

@kaelzhangkaelzhangMay 24, 2018

Copy link
Copy Markdown
ContributorAuthor

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

@kaelzhangkaelzhangMay 24, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

apapirovski commented May 24, 2018

Copy link
Copy Markdown
Contributor

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

mscdex commented May 24, 2018

Copy link
Copy Markdown
Contributor

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

BridgeAR commented May 24, 2018

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

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@mcollina

mcollina commented May 28, 2018

Copy link
Copy Markdown
Member

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

@BridgeAR

BridgeAR commented May 28, 2018

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

mcollina commented May 28, 2018

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

apapirovski commented May 28, 2018

Copy link
Copy Markdown
Contributor

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

mcollina commented May 28, 2018

Copy link
Copy Markdown
Member

CI failures are unrelated. This can land.

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

mcollina commented May 28, 2018

Copy link
Copy Markdown
Member

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

@mcollina

mcollina commented May 28, 2018

Copy link
Copy Markdown
Member

Landed as 9f4bf4c.

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>
@MylesBorinsMylesBorins mentioned this pull request May 29, 2018
Merged
@mcollinamcollina 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 freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollinamcollinamcollina approved these changes
@lpincalpincalpinca approved these changes
+2 more reviewers
@ryzokukenryzokukenryzokuken left review comments
@apapirovskiapapirovskiapapirovski approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.notable-changePRs with changes that should be highlighted in changelogs.streamIssues 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