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

pipeline() doesn't finish with sandwiched duplex stream #43682

Closed
vweevers opened this issue Jul 4, 2022 · 1 comment · Fixed by #43701
Closed

pipeline() doesn't finish with sandwiched duplex stream #43682

vweevers opened this issue Jul 4, 2022 · 1 comment · Fixed by #43701
Labels
stream Issues and PRs related to the stream subsystem.

Comments

@vweevers
Copy link
Contributor

vweevers commented Jul 4, 2022

Version

18.4.0

Platform

Linux & Microsoft Windows NT 10.0.19044.0 x64

Subsystem

stream

What steps will reproduce the bug?

const { pipeline, Duplex, PassThrough, finished } = require('stream')

const remote = new PassThrough()
const local = new Duplex({
  read () {},
  write (chunk, enc, callback) {
    callback()
  }
})

for (const event of ['close', 'finish', 'end']) {
  remote.on(event, () => console.log('remote', event))
  local.on(event, () => console.log('local', event))
}

pipeline(remote, local, remote, function () {
  // We never get here
  console.log('done')
})

// To fix, uncomment:
// finished(local, { writable: true, readable: false }, () => local.destroy())

setImmediate(() => {
  remote.end()
})

How often does it reproduce? Is there a required condition?

Always

What is the expected behavior?

remote end
remote finish
local finish
remote close
local close
done

Not necessarily in that order.

What do you see instead?

remote end
remote finish
local finish
remote close

Additional information

The writable side of the local Duplex stream finishes, but the readable side does not. I can see two arguments here, for deciding whether this is a bug in core.

  1. Because the local stream has disconnected readable and writable sides, that stream is responsible for destroying itself and/or ending its readable side.
  2. On the other hand, the readable side of the local stream is being piped to the now-closed remote stream, so it's a dead end. It'd be a different story for pipeline(x, local), i.e. local being the last stream, because then the pipeline isn't consuming the readable side of local. But in pipeline(x, local, x) I expect the pipeline to fully manage the lifetime of local.

Last version where it works is 12.22.9 (or readable-stream@3). On 14.0.0, 16.14.2, 18.4.0, or readable-stream@4, it does not.

@lpinca lpinca added the stream Issues and PRs related to the stream subsystem. label Jul 4, 2022
@aduh95
Copy link
Contributor

aduh95 commented Jul 5, 2022

@nodejs/streams

ronag added a commit to nxtedition/node that referenced this issue Jul 6, 2022
If the destination stream is closed before the source has completed
the pipeline should finnish with premature close.

Fixes: nodejs#43682
ronag added a commit to nxtedition/node that referenced this issue Jul 6, 2022
If the destination stream is closed before the source has completed
the pipeline should finnish with premature close.

Fixes: nodejs#43682
nodejs-github-bot pushed a commit that referenced this issue Jul 11, 2022
If the destination stream is closed before the source has completed
the pipeline should finnish with premature close.

Fixes: #43682

PR-URL: #43701
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this issue Jul 12, 2022
If the destination stream is closed before the source has completed
the pipeline should finnish with premature close.

Fixes: #43682

PR-URL: #43701
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants