stream: add the pipeline destoryDestOnError option

[rickyes]

Fixes: #26311

/cc @mcollina@ronag

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[ronag]

Could we add a new signature to pipeline with something along the lines of:

pipeline(arrayOfStreams,options,callback)

[rickyes]

Could we add a new signature to pipeline with something along the lines of:

pipeline(arrayOfStreams,options,callback)

Yes, I also found the missing documentation for this section, maybe we can create a new PR to document this separately.

[ronag]

I think in readable.pipe the option is called end to not end(). Maybe we should have { end, destroy } as options?

[rickyes]

I think in readable.pipe the option is called end to not end(). Maybe we should have { end, destroy } as options?

It may be a little different, the destoryDestOnError option takes effect when an error occurs in the stream.

[mcollina]

Can you add a test with more than 2 streams?

[rickyes]

Can you add a test with more than 2 streams?

done

[ronag]

I don't find this very elegant. Though I guess there is nothing wrong with it per se... though couldn't you easily achieve the same thing with:

constdst=streams.pop()awaitfinished(pipeline(streams,()=>{}).on('error',(err)=>{// ...}).pipe(dst))

[rickyes]

I don't find this very elegant. Though I guess there is nothing wrong with it per se... though couldn't you easily achieve the same thing with:

constdst=streams.pop()awaitfinished(pipeline(streams,()=>{}).on('error',(err)=>{// ...}).pipe(dst))

Come to think of it, maybe the way @ronag offers it would have fewer side effects. The PR implementation, when an error occurs during the transmission of a stream (response stream has been data sent) we send end('error') again which may be incorrect.

[rickyes]

However, the disadvantage is that we need to combine two api together, which is not very convenient.

[ronag]

However, the disadvantage is that we need to combine two api together, which is not very convenient.

This is a bit of an edge case though... So I'm not sure how important the convenience is :). I'm not opposed to this PR, but I'm unsure whether we need it.

[rickyes]

ping @mcollina

[mcollina]

I would really like some feedbacks from @mafintosh

[jasnell]

Suggested change

	*`destroyDestOnError` {boolean} If value is false, the destination
	is not destroyed when a stream error occurs.
	**Default:**`true`.
	*`destroyDestOnError` {boolean} If value is `false`, the destination
	is not destroyed when a stream error occurs. **Default:** `true`.

Not particularly in love with name destroyDestOnError but also don't have any better suggestions.

[rickyes]

fixed

[rickyes]

ping again @mafintosh

[mafintosh]

I don’t really see the use-case, since you’d have to do some error detection as well to make sure that the dest stream hasn’t been partially written, as the error could be lots of things from the pipeline.

For the use case described in the issue I’d wait for the open event instead that @ronag recently standardized.

In general, PR looks fine, -0 on the feature.

[rickyes]

I don’t really see the use-case, since you’d have to do some error detection as well to make sure that the dest stream hasn’t been partially written, as the error could be lots of things from the pipeline.

Yes, that's what I was worried about at first, and I don't have a good way to handle the situation yet, so if people have some suggestions then Would be great.

[rickyes]

Due to lack of sufficient impetus, I will close this PR.