stream: Simplify event listeners #5711

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants
Member

geek commented Jun 17, 2013

A suggestion for making the stream listeners simpler.

everything here is 4 spaced. style is two spaces for indentation.

Member

geek commented Jun 18, 2013

fixed the spacing

@bnoordhuis bnoordhuis commented on an outdated diff Jun 18, 2013

lib/stream.js
@@ -67,23 +67,19 @@ Stream.prototype.pipe = function(dest, options) {
// If the 'end' option is not supplied, dest.end() will be called when
// source gets the 'end' or 'close' events. Only dest.end() once.
if (!dest._isStdio && (!options || options.end !== false)) {
- source.on('end', onend);
- source.on('close', onclose);
+ source.once('end', onend);
+ source.once('close', onclose);
}
var didOnEnd = false;

@bnoordhuis bnoordhuis commented on the diff Jun 18, 2013

lib/stream.js
}
var didOnEnd = false;
function onend() {
- if (didOnEnd) return;
- didOnEnd = true;
-
+ source.removeListener('close', onclose);
dest.end();
@bnoordhuis

bnoordhuis Jun 18, 2013

Owner

This PR makes it possible for dest.end() to get called multiple times. I don't think there's any guarantee it's omnipotent (IOW, safe to call twice.)

@geek

geek Jun 18, 2013

Member

Since the only code calling onclose or onend is from the event listeners... shouldn't it be impossible for them to be called multiple times when the listeners are removed when one of them is executed? If not, then that sounds like a bug in removeListener.

@bnoordhuis bnoordhuis commented on the diff Jun 18, 2013

lib/stream.js
dest.end();
}
function onclose() {
- if (didOnEnd) return;
- didOnEnd = true;
-
+ source.removeListener('end', onend);
if (typeof dest.destroy === 'function') dest.destroy();
@bnoordhuis

bnoordhuis Jun 18, 2013

Owner

Ditto for dest.destroy().

geek closed this Oct 2, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment