stream: fix removeAllListeners() for Stream.Readable#20924
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the stream 
apapirovski left a comment
apapirovski left a comment There was a problem hiding this comment.
LGTM but we could make the condition simpler (or, well, get rid of it).
lib/_stream_readable.js Outdated
There was a problem hiding this comment.
Actually, could you do this instead: Stream.prototype.removeAllListeners.apply(this, arguments)?
kaelzhangforce-pushed the master branch 2 times, most recently from 771108d to 24320ecCompare
Perhaps a better fix might be to change |
ryzokuken left a comment
ryzokuken left a comment There was a problem hiding this comment.
Looks nice, going forward, but I wasn't really satisfied by the unit test. Could you make the changes I asked for?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I've changed the test a little bit. Any suggestions?
apapirovski commented May 24, 2018
I don't think |
mscdex commented May 24, 2018
Which is why I said it'd be semver-major. I can't imagine anyone relying on |
BridgeAR commented May 24, 2018
I agree with @mscdex that it would be best to change |
mcollina commented May 28, 2018
BridgeAR commented May 28, 2018
@apapirovski@mcollina@lpinca is anyone of you strongly in favor of this over just fixing not converting |
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 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 commented May 28, 2018
CI failures are unrelated. This can land. |
mcollina added the author ready mcollina commented May 28, 2018
mcollina commented May 28, 2018
Landed as 9f4bf4c. |
Fixes: #20923 PR-URL: #20924 Refs: #20923 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
Fixes: #20923 PR-URL: #20924 Refs: #20923 Reviewed-By: Anatoli Papirovski <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]>
mcollina added the notable-change 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
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
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
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.
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.
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.
8 participants
Refs: #20923
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes