stream.Readable unpipes the wrong stream when piped into multiple streams

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.

@niels4

@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.

No problem, thanks for moving so quickly on this

@niels4

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

/cc @nodejs/streams fyi

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

@niels4

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?

@thealphanerd

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

streams: fix regression in `unpipe()`

@thealphanerd

yes I did 😊

On Wed, Oct 19, 2016, 1:21 PM Gibson Fahnestock notifications@github.com
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
.

streams: fix regression in `unpipe()`

Navigation Menu

Repository navigation

stream.Readable unpipes the wrong stream when piped into multiple streams #9170

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

stream.Readable unpipes the wrong stream when piped into multiple streams #9170

Description

Activity

addaleax commented on Oct 18, 2016

niels4 commented on Oct 18, 2016

lpinca commented on Oct 18, 2016

addaleax commented on Oct 18, 2016

MylesBorins commented on Oct 18, 2016

addaleax commented on Oct 18, 2016

gibfahn commented on Oct 19, 2016

MylesBorins commented on Oct 19, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions