search

grncdr / merge-stream

Merge multiple streams into one interleaved stream
MIT License
214 stars 16 forks source link

handle node 0.10.0 compatibility edge case. #7

Closed rgaskill closed 10 years ago

rgaskill commented 10 years ago

See explanation here

This resolves the issue I described on #6. I would assume it also fixes @heneryville description as well.

grncdr commented 10 years ago

Thanks so much for the patch! Unfortunately, I can't accept it as is, because the unconditional resume() makes the stream act like a "streams1" stream. The stream can potentially lose data by emitting 'data' events before there is a listener.

That this resolves the issue you saw on #6 is interesting though, and provides an important bit of information in debugging that. I'd be very grateful if @heneryville could give it a look and see if it fixes the bad behaviour he was seeing as well.

grncdr commented 10 years ago

Also, from looking at the code, I think the on('unpipe', remove) at line 14 is suspect, does commenting that out fix your test?

rgaskill commented 10 years ago

Yeah your comment about the resume() makes sense. In my case the data loss is not a concern because the streams I am merging in have no output I care about. I can just call .resume() externally after adding all the streams to the merge stream and before I return it to the gulp task. That resolves my issue as well.

There is definitely something interesting going on here. Commenting out the on('unpipe', remove) does not resolve the issue. Interestingly though, removing the source.pipe(output, {end: false}) does resolve my issue.

My current hypothesis as to what is going on in my example are the streams that I am merging in are actually completing before gulp has a chance to interact with the merged stream. So everything works fine until I introduce a longer running stream requiring gulp to wait for the completion of merge stream. Which it does not unless I add a on('data', noop) or a resume() or remove the source.pipe().

I need to investigate what gulp is expecting from the stream I return to the task. I am hoping that will give me a clearer understanding as to what is going on.

rgaskill commented 10 years ago

After digging further into how gulp was handling completion notifications from streams I found my issue to be orchestrator/orchestrator#48 and nothing to do with merge-stream.