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.

Sign up for GitHub

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

Jump to bottom

streams: fix regression in unpipe() #9171

Closed
wants to merge 3 commits into from
Closed

streams: fix regression in unpipe() #9171

wants to merge 3 commits into from

Conversation

addaleax
Copy link
Member

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

stream

Description of change

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.

Credit for the bug report and the test case go to @niels4. Since it’s a rather big bug, I’ve opened a PR myself. (If you let me know an email address/name in time, I can add you as the git author of the test file.)

Fixes: #9170

evanlucas
evanlucas approved these changes Oct 18, 2016
View reviewed changes
Copy link
Contributor

@evanlucas evanlucas left a comment

Choose a reason for hiding this comment

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

LGTM

@addaleax
Copy link
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/5667/

@niels4
Copy link

niels4 commented Oct 18, 2016 •
edited
Loading

Thanks for getting this started. I was just finishing a meeting and then it took me a little bit to figure out where to put the test.

I took some time to change the test case to look similar to the other tests already in place (this one doesn't refer directly to _readableState). Looks like you already have this pull request started, but my changes can be found in this commit
niels4@bc88190

My name is Niels Nielsen, email niels@protectwise.com

@addaleax
Copy link
Member Author

@niels4 awesome, I’ve updated the PR with bits from your change + set you as the author here

addaleax and others added 2 commits October 18, 2016 22:58
@addaleax
streams: fix regression in unpipe()
431638d 
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
@addaleax
test: add regression test for unpipe()
b41cf22 
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.
lpinca
lpinca reviewed Oct 18, 2016
View reviewed changes
test/parallel/test-stream-pipe-unpipe-streams.js

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

Copy link
Member

Choose a reason for hiding this comment

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

Nit: probably unrelated but maybe it makes sense to assert that source._readableState.pipes contains both dest1 and dest2 now.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lpinca done

lpinca
lpinca approved these changes Oct 18, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

LGTM

@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Oct 18, 2016
@mscdex mscdex mentioned this pull request Oct 18, 2016
Closed
jasnell
jasnell approved these changes Oct 18, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM

@jasnell jasnell added this to the 7.0.0 milestone Oct 18, 2016
@addaleax
Copy link
Member Author

New CI: https://ci.nodejs.org/job/node-test-commit/5675/

mcollina
mcollina approved these changes Oct 19, 2016
View reviewed changes
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

@MylesBorins
Copy link
Contributor

LGTM

@MylesBorins
Copy link
Contributor

freebsd 11 stalled, arm7 wheezy and smart os 14 never started

smart os 15 fails are all known flakes

There are two freebsd10 failures related to timers, I do not think they are related to this change

I'm going to go ahead and land the change as this is a pretty big break.

@MylesBorins
Copy link
Contributor

MylesBorins commented Oct 19, 2016 •
edited
Loading

landed in bb173f9...92ece86

edit: backported to v7.x-staging and v6.x-staging. release coming soon

MylesBorins pushed a commit that referenced this pull request Oct 19, 2016
@addaleax
streams: fix regression in unpipe()
8043ca7 
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 <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 19, 2016
test: add regression test for unpipe()
92ece86 
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.

PR-URL: #9171
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 19, 2016
@addaleax
streams: fix regression in unpipe()
8244efb 
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 <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 19, 2016
test: add regression test for unpipe()
6bef1db 
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.

PR-URL: #9171
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 19, 2016
@addaleax
streams: fix regression in unpipe()
c272c8b 
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 <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 19, 2016
test: add regression test for unpipe()
56368e3 
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.

PR-URL: #9171
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 19, 2016
@addaleax
streams: fix regression in unpipe()
f4b766f 
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 <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 19, 2016
test: add regression test for unpipe()
6072326 
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.

PR-URL: #9171
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 19, 2016
Merged
@addaleax addaleax mentioned this pull request Oct 19, 2016
Merged
2 tasks
@MylesBorins MylesBorins mentioned this pull request Nov 11, 2016
Closed
3 tasks
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Nov 11, 2016
@addaleax
streams: fix regression in unpipe()
c25eb61 
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 <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Nov 11, 2016
test: add regression test for unpipe()
a1d7201 
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.

Ref: nodejs#9553
PR-URL: nodejs#9171
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 14, 2016
@addaleax
streams: fix regression in unpipe()
9b9762c 
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 <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 14, 2016
test: add regression test for unpipe()
d9c3364 
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.

Ref: #9553
PR-URL: #9171
Reviewed-By: Evan Lucas <evanlucas@me.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Myles Borins <myles.borins@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Nov 22, 2016
Merged
@addaleax addaleax deleted the fix-stream-unpipe-regression branch December 6, 2016 16:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@MylesBorins MylesBorins MylesBorins approved these changes

@evanlucas evanlucas evanlucas approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
7.0.0
Development

Successfully merging this pull request may close these issues.
9 participants
@addaleax @niels4 @MylesBorins @mcollina @jasnell @evanlucas @lpinca @cjihrig @mscdex