Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

stream: don't destroy final readable stream in pipeline #32110

Closed
wants to merge 1 commit into from

Conversation

ronag
Copy link
Member

@ronag ronag commented Mar 5, 2020

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

@ronag ronag requested review from mcollina and lpinca March 5, 2020 22:20
@ronag ronag changed the title stream: don't destroy last stream if readable in pipeline don't destroy final readable stream in pipeline Mar 5, 2020
@nodejs-github-bot

This comment has been minimized.

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

Fixes: nodejs#32105
@ronag

This comment has been minimized.

@nodejs-github-bot
Copy link
Collaborator

@ronag ronag added the stream Issues and PRs related to the stream subsystem. label Mar 5, 2020
@ronag ronag changed the title don't destroy final readable stream in pipeline stream: don't destroy final readable stream in pipeline Mar 5, 2020
@nodejs-github-bot
Copy link
Collaborator

@ronag ronag added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-open-v13.x labels Mar 5, 2020
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ronag
Copy link
Member Author

ronag commented Mar 6, 2020

@nodejs/streams

@MylesBorins
Copy link
Contributor

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

Copy link
Contributor

@vweevers vweevers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member Author

ronag commented Mar 7, 2020

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
Copy link
Contributor

vweevers commented Mar 7, 2020

@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
Copy link
Member Author

ronag commented Mar 7, 2020

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
Copy link
Contributor

vweevers commented Mar 7, 2020

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
Copy link
Contributor

vweevers commented Mar 7, 2020

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
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ronag
Copy link
Member Author

ronag commented Mar 8, 2020

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

@mcollina
Copy link
Member

mcollina commented Mar 8, 2020

@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
Copy link
Contributor

vweevers commented Mar 8, 2020

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
Copy link
Member

mcollina commented Mar 8, 2020

You are welcome to chime in here @vweevers!

ronag added a commit that referenced this pull request Mar 8, 2020
If the last stream in a pipeline is still usable/readable
don't destroy it to allow further composition.

Fixes: #32105

PR-URL: #32110
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@ronag
Copy link
Member Author

ronag commented Mar 8, 2020

Landed in 4d93e10

@ronag ronag closed this Mar 8, 2020
MylesBorins pushed a commit that referenced this pull request Mar 9, 2020
If the last stream in a pipeline is still usable/readable
don't destroy it to allow further composition.

Fixes: #32105
Backport-PR-URL: #32111
PR-URL: #32110
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Mar 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pipeline aborts all HTTP requests
6 participants