Stream#pipe bugfixes #929

Closed
wants to merge 3 commits into
from

Projects

None yet

3 participants

@felixge
felixge commented Apr 14, 2011

This pull request includes 2 bug fixes and one whitespace fix.

Please check the commits for details and feel free to cherry-pick.

felixge added some commits Apr 14, 2011
@felixge felixge Fix: Fix pipes for streams that only emit 'close'
Some readable streams may only emit a 'close' event, and no 'end'
event. This patch fixes Stream#pipe so that it will still call
end() on the destination stream in that case.

Note: The test case for this patch also exposed another issue where
multiple pipes to the same writable stream will each call end() on
the source. This will break all remaining pipes.
e67ffcd
@felixge felixge Fix: Multiple pipes to the same stream were broken
When creating multiple .pipe()s to the same destination stream, the
first source to end would close the destination, breaking all remaining
pipes. This patch fixes the problem by keeping track of all open
pipes, so that we only call end on destinations that have no more
sources piping to them.
9e55a82
@felixge
Owner

Note: There is no need to keep track if onend() has been called because it is guaranteed to be only called once due to the cleanup() method.

@felixge
Owner

The data structure here could probably be improved, but I think the overall idea is good, and this is unlikely to become a bottleneck.

Why not just unregister the on end handler after the first call? No pushing, no slicing, easy. Or maybe I'm missing what this does.

Owner

@polotek: Yeah, I think you're misunderstanding the patch. Did you read the commit message carefully?

Clearly not. It was 2am :) I understand now though. I'm still not crazy about the indexOf and slice. But it sound like you're not either. I don't have a better idea off hand.

Owner

Well, the problem is that this indexOf() is probably O(n), and wouldn't do so well with 10k+ streams being piping at the same time.

I don't think it's a huge issue right now, but future improvements could improve:

  • Reversing the list of pipes: It may be reasonable to assume that the destination of an ended source is to be found closer to the end of the list than to the beginning. But this would probably only help with pipes of roughly the same duration, and it would still be O(n)
  • Extending the writable stream interface to include a list of "pipes" or "sources" that are pumping data into this. A ref counter would probably be enough. That should get us back to O(1), but feels a little dirty.

Either way, the first step would be to demonstrate an actual performance problem with this implementation. Optimizing something that has not shown to be a problem is kind of pointless.

@ry ry added a commit that closed this pull request Apr 14, 2011
@felixge @ry felixge + ry Fix: Multiple pipes to the same stream were broken
When creating multiple .pipe()s to the same destination stream, the
first source to end would close the destination, breaking all remaining
pipes. This patch fixes the problem by keeping track of all open
pipes, so that we only call end on destinations that have no more
sources piping to them.

closes #929
6c5b31b
@ry ry closed this in 6c5b31b Apr 14, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment