align with node streams #47
Conversation
ronag
force-pushed
the
refactor-streams
branch
from
4a5783f
to
81b204f
Compare
ronag
force-pushed
the
refactor-streams
branch
3 times, most recently
from
ac4d1a6
to
8a178f8
Compare
| @@ -96,7 +101,6 @@ class MultiStream extends stream.Readable { | |||
| _gotNextStream (stream) { | |||
| if (!stream) { | |||
| this.push(null) | |||
| this.destroy() | |||
There was a problem hiding this comment.
Use autoDestroy to ensure proper event ordering
| if (!readableEnded && !stream.destroyed) { | ||
| const err = new Error('ERR_STREAM_PREMATURE_CLOSE') | ||
| err.code = 'ERR_STREAM_PREMATURE_CLOSE' | ||
| this.destroy(err) |
There was a problem hiding this comment.
close without end is an error
| @@ -118,6 +127,7 @@ class MultiStream extends stream.Readable { | |||
| stream.removeListener('readable', onReadable) | |||
| stream.removeListener('end', onEnd) | |||
| stream.removeListener('close', onClose) | |||
| stream.destroy() | |||
There was a problem hiding this comment.
not all streams will auto destroy on 'end', so we should explicitly destroy just to make sure we don't have any dangling resources in buggy streams
| @@ -58,19 +57,25 @@ class MultiStream extends stream.Readable { | |||
| this._forwarding = false | |||
| } | |||
|
|
|||
| destroy (err) { | |||
There was a problem hiding this comment.
We should not override destroy as that will bypass important teardown logic and event ordering.
ronag
force-pushed
the
refactor-streams
branch
from
8a178f8
to
06ef915
Compare
This refactors destroy to better align with the node stream invariants.
ronag
force-pushed
the
refactor-streams
branch
from
06ef915
to
7b878f8
Compare
ronag
force-pushed
the
refactor-streams
branch
from
36c876b
to
22856b2
Compare
ronag
force-pushed
the
refactor-streams
branch
from
22856b2
to
c329b7b
Compare
|
I think this looks good. I'll let @feross click review as I don't want to imply any warranty 🤣 I get enough "frustrated stream breakage github issues" as is. |
|
@mafintosh I took this one step further... though I'm not sure if it's maybe too much changes... what do you think ronag#1? |
|
FYI - I won't have time to review this for a while. Will do as soon as I can. |
|
I agree with @pcothenet. This PR also fixes the |
|
I'm more than happy to help out here. But repo admin needs to review and eventually merge. |
|
@feross will you have time for this in the near future? the PR seems to work pretty well on my end! |
|
@feross FYI: still no issue with this PR on production for us after 6 months. |
|
I'd recommend to use this approach, for me it works in more cases correctly. const merge = (...streams) => {
let pass = new PassThrough();
let waiting = streams.length;
for (const stream of streams) {
pass = stream.pipe(pass, { end: false });
stream.once('end', () => --waiting === 0 && pass.emit('end'));
}
return pass;
};P.S. I tried both Given from https://stackoverflow.com/questions/16431163/concatenate-two-or-n-streams |
|
I'm happy to merge this, but the conflicts need to be fixed. @mafintosh Does this still look good to merge? |
|
👍 from me |
|
@ronag Looks like this still has conflicts – can you resolve those and then this looks ready to merge! |
| stream | ||
| .on('error', callback) | ||
| .on('close', () => callback())) | ||
| .destroy(err, callback) |
There was a problem hiding this comment.
Does the destroy() method really take a callback?
|
Sorry the delay everyone. This is now merged and released as 4.1.0! |
Since feross/multistream#47 was merged, we shouldn't need to pin this anymore.
This refactors destroy to better align with the node stream invariants.