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.Readable unpipes the wrong stream when piped into multiple streams #9170

Closed
niels4 opened this issue Oct 18, 2016 · 8 comments
Closed
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.

Comments

@niels4
Copy link

niels4 commented Oct 18, 2016

  • Version: 6.8.0 and later
  • Platform: all
  • Subsystem: stream.Readable

Since node 6.8.0 there is a bug where unpiping a stream from a readable stream that has a _readableState.pipesCount > 1 will cause it to remove the first stream in the _.readableState.pipes array no matter where in the list the dest stream was.

Example test case:

"use strict"
const PassThrough = require('stream').PassThrough

const source = PassThrough()
const dest1 = PassThrough()
const dest2 = PassThrough()

source.pipe(dest1)
source.pipe(dest2)

source.unpipe(dest2)

console.log(source._readableState.pipes === dest1) //false
console.log(source._readableState.pipes === dest2) //true

As you can see, the wrong stream was unpiped

It looks like this is the commit that broke things (2e568d9#diff-ba6a0df0f5212f5cba5ca5179e209a17R670)
The variable used to splice was renamed to index on line 670, however the splice call on line 674 is still using the incorrect variable i.

@addaleax addaleax added stream Issues and PRs related to the stream subsystem. confirmed-bug Issues with confirmed bugs. labels Oct 18, 2016
@addaleax
Copy link
Member

addaleax commented Oct 18, 2016

@niels4 Wow, that’s a pretty big bug to go unnoticed until now. Since you basically have the test case ready and identified what the bug is, do you want to go ahead with a pull request? The CONTRIBUTING.md has some pointers but feel free to ask anything here or in #node-dev on Freenode if anything’s unclear.

@niels4
Copy link
Author

niels4 commented Oct 18, 2016

No problem, thanks for moving so quickly on this

@lpinca
Copy link
Member

lpinca commented Oct 18, 2016

Kinda sad that all reviewers, me included, missed that. Nice find @niels4.

@addaleax
Copy link
Member

/cc @nodejs/streams fyi

@MylesBorins
Copy link
Contributor

/cc @nodejs/release @nodejs/lts if we can get a fix in ASAP I would like to put out a v6.8.2 tomorrow

@addaleax
Copy link
Member

Yeah I’ll have one ready if @niels4 doesn’t open a PR. In that case I’d be happy to hear a name + email address @niels4 for attribution?

addaleax added a commit to addaleax/node that referenced this issue Oct 18, 2016
Since 2e568d9 there is a bug where unpiping a stream
from a readable stream that has `_readableState.pipesCount > 1`
will cause it to remove the first stream in the
`_.readableState.pipes` array no matter where in the list the
`dest` stream was.

This patch corrects that problem.

Fixes: nodejs#9170
@gibfahn
Copy link
Member

gibfahn commented Oct 19, 2016

@thealphanerd Do you mean v6.9.1 rather than v6.8.2?

MylesBorins pushed a commit that referenced this issue Oct 19, 2016
Since 2e568d9 there is a bug where unpiping a stream
from a readable stream that has `_readableState.pipesCount > 1`
will cause it to remove the first stream in the
`_.readableState.pipes` array no matter where in the list the
`dest` stream was.

This patch corrects that problem.

PR-URL: #9171
Fixes: #9170
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 19, 2016
Since 2e568d9 there is a bug where unpiping a stream
from a readable stream that has `_readableState.pipesCount > 1`
will cause it to remove the first stream in the
`_.readableState.pipes` array no matter where in the list the
`dest` stream was.

This patch corrects that problem.

PR-URL: #9171
Fixes: #9170
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this issue Oct 19, 2016
Since 2e568d9 there is a bug where unpiping a stream
from a readable stream that has `_readableState.pipesCount > 1`
will cause it to remove the first stream in the
`_.readableState.pipes` array no matter where in the list the
`dest` stream was.

This patch corrects that problem.

PR-URL: #9171
Fixes: #9170
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
@MylesBorins
Copy link
Contributor

yes I did 😊

On Wed, Oct 19, 2016, 1:21 PM Gibson Fahnestock [email protected]
wrote:

@thealphanerd https://github.com/TheAlphaNerd Do you mean v6.9.1 rather
than v6.8.2?


You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#9170 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAecV9nQSLB6mp7cBUBTu5EyJE0p4BnKks5q1gtWgaJpZM4KaMCF
.

MylesBorins pushed a commit to MylesBorins/node that referenced this issue Nov 11, 2016
Since 2e568d9 there is a bug where unpiping a stream
from a readable stream that has `_readableState.pipesCount > 1`
will cause it to remove the first stream in the
`_.readableState.pipes` array no matter where in the list the
`dest` stream was.

This patch corrects that problem.

Ref: nodejs#9553
PR-URL: nodejs#9171
Fixes: nodejs#9170
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
MylesBorins pushed a commit that referenced this issue Nov 14, 2016
Since 2e568d9 there is a bug where unpiping a stream
from a readable stream that has `_readableState.pipesCount > 1`
will cause it to remove the first stream in the
`_.readableState.pipes` array no matter where in the list the
`dest` stream was.

This patch corrects that problem.

Ref: #9553
PR-URL: #9171
Fixes: #9170
Reviewed-By: Evan Lucas <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: Myles Borins <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants