stream: don't destroy final readable stream in pipeline

[ronag]

If the last stream in a pipeline is still usable/readable
don't destroy it to allow further composition.

Fixes: #32105

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

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29570/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29577/

[mcollina]

lgtm

[ronag]

@nodejs/streams

[MylesBorins]

CI LGTM (not signing off as I am not familliar with codebase). If no one else wants to volunteer I can help get this out in a release next week

[vweevers]

Good for composition. Might be surprising in some cases - e.g. when the user isn't aware that their last stream is a duplex, for example with gulp.dest() - so should be documented later on.

[ronag]

Good for composition. Might be surprising in some cases - e.g. when the user isn't aware that their last stream is a duplex, for example with gulp.dest() - so should be documented later on.

Why is it a Duplex? There are cases e.g. net.Socket where whether it's "duplex" or not is a runtime decision. In such a case e.g. Duplex.readable should be set to false to indicate that it's only writable event though it is a Duplex type, in which case it would work as expected.

I agree some documentation might be in order.

[vweevers]

@ronag Many gulp examples suggest that gulp.dest() is a writable stream:

const vfs = require('vinyl-fs') // or gulp
const { pipeline } = require('stream')

const src = vfs.src('./index.js')
const dest = vfs.dest('./dest')

pipeline(src, dest, function (err) {
  if (err) throw err
})

And following that, you'd expect dest to be destroyed. However, you can also do:

const src = vfs.src('./index.js')
const dest1 = vfs.dest('./dest1')
const dest2 = vfs.dest('./dest2')

pipeline(src, dest1, dest2, function (err) {
  if (err) throw err
  assert.strictEqual(dest2.readable, true)
})

[ronag]

And following that, you'd expect dest to be destroyed. However, you can also do:

I don't understand what that last example is supposed to do? Will dest1 pipe into dest2? Does that work with vinyl? That's interesting.

[vweevers]

Yes, they are duplex streams that pass through their input. In a nutshell, the snippet above copies index.js to both dest1/index.js and dest2/index.js.

I can't think of cases other than gulp though and I consider the above to be a flaw in its API design rather than a problem that node streams should address.

[vweevers]

I wanted to suggest a CITGM, which includes vinyl-fs (marked as flaky?) but unfortunately its tests use pump rather than pipeline.

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29608/

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/29610/

[ronag]

@vweevers You are a member of nodejs/streams but GitHub doesn't count your approval? How come?

[mcollina]

@vweevers You are a member of nodejs/streams but GitHub doesn't count your approval? How come?

It does not because @vweevers is not a Node.js collaborator. We’ll need another approval.

[vweevers]

I don't have write access to this repo. @mcollina invited me to the streams wg in relation to readable-stream (I'm active there, helping with browser tests and reviews, but wasn't so much active here). If you'll have me, and time permitting, I'd like to help out here as well.

[mcollina]

You are welcome to chime in here @vweevers!

[ronag]

Landed in 4d93e10