Description

niels4

opened on Oct 18, 2016

Issue body actions

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

Activity

1. addaleax

   added

   streamIssues and PRs related to Node.js streams.Issues and PRs related to Node.js streams.

   confirmed-bugIssues and PRs for confirmed bugs.Issues and PRs for confirmed bugs.

   on Oct 18, 2016
2. addaleax commented on Oct 18, 2016

   

   addaleax

   on Oct 18, 2016

   Member

   More actions

   @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.
3. niels4 commented on Oct 18, 2016

   

   niels4

   on Oct 18, 2016

   Author

   More actions

   No problem, thanks for moving so quickly on this
4. lpinca commented on Oct 18, 2016

   

   lpinca

   on Oct 18, 2016

   Member

   More actions

   Kinda sad that all reviewers, me included, missed that. Nice find @niels4.
5. addaleax commented on Oct 18, 2016

   

   addaleax

   on Oct 18, 2016

   Member

   More actions

   /cc @nodejs/streams fyi
6. MylesBorins commented on Oct 18, 2016

   

   MylesBorins

   on Oct 18, 2016

   Contributor

   More actions

   /cc @nodejs/release @nodejs/lts if we can get a fix in ASAP I would like to put out a v6.8.2 tomorrow
7. addaleax commented on Oct 18, 2016

   

   addaleax

   on Oct 18, 2016

   Member

   More actions

   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?
8. gibfahn commented on Oct 19, 2016

   

   gibfahn

   on Oct 19, 2016

   Member

   More actions

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

   closed this as completedin 8043ca7on Oct 19, 2016
10. MylesBorins

    added 3 commits that reference this issue on Oct 19, 2016

    streams: fix regression in `unpipe()`

    8244efb

    streams: fix regression in `unpipe()`

    c272c8b

    streams: fix regression in `unpipe()`

    f4b766f
11. MylesBorins commented on Oct 19, 2016

    

    MylesBorins

    on Oct 19, 2016

    Contributor

    More actions

    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
    .

    Reacted by Oleh Aleinyk

    Reacted by Gibson Fahnestock
12. sjarmak

    added a commit that references this issue on Jul 27, 2026

    streams: fix regression in `unpipe()`

    8043ca7

Issue actions