streams: fix regression in `unpipe()` by addaleax · Pull Request #9171 · nodejs/node

addaleax · 2016-10-18T20:35:16Z

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

LGTM

addaleax · 2016-10-18T20:38:48Z

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

niels4 · 2016-10-18T20:50:41Z

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 · 2016-10-18T20:57:34Z

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

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

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 · 2016-10-18T21:11:01Z

test/parallel/test-stream-pipe-unpipe-streams.js

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


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

@lpinca done

lpinca

LGTM

jasnell

LGTM

addaleax · 2016-10-19T08:32:20Z

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

mcollina

LGTM

MylesBorins · 2016-10-19T11:19:35Z

LGTM

MylesBorins · 2016-10-19T12:55:43Z

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 · 2016-10-19T13:00:44Z

landed in bb173f9...92ece86

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

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

evanlucas approved these changes Oct 18, 2016

View reviewed changes

cjihrig approved these changes Oct 18, 2016

View reviewed changes

addaleax force-pushed the fix-stream-unpipe-regression branch from 9859353 to c3dbd05 Compare October 18, 2016 20:57

addaleax and others added 2 commits October 18, 2016 22:58

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.

addaleax force-pushed the fix-stream-unpipe-regression branch from c3dbd05 to b41cf22 Compare October 18, 2016 20:59

lpinca reviewed Oct 18, 2016

View reviewed changes

lpinca approved these changes Oct 18, 2016

View reviewed changes

[squash] addres nit for checking _readableState.pipes before unpiping

f140c3f

mscdex added the stream Issues and PRs related to the stream subsystem. label Oct 18, 2016

mscdex mentioned this pull request Oct 18, 2016

Bot failed to label PR nodejs/github-bot#86

Closed

jasnell approved these changes Oct 18, 2016

View reviewed changes

jasnell added this to the 7.0.0 milestone Oct 18, 2016

mcollina approved these changes Oct 19, 2016

View reviewed changes

addaleax added the lts-watch-v6.x label Oct 19, 2016

MylesBorins approved these changes Oct 19, 2016

View reviewed changes

MylesBorins closed this Oct 19, 2016

MylesBorins mentioned this pull request Oct 19, 2016

V6.9.1 proposal #9186

Merged

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Oct 19, 2016

addaleax mentioned this pull request Oct 19, 2016

meta: remove let from for loops #8873

Merged

2 tasks

MylesBorins added dont-land-on-v4.x and removed dont-land-on-v4.x labels Nov 11, 2016

MylesBorins mentioned this pull request Nov 11, 2016

Backport removing let in for loop declaration #9553

Closed

3 tasks

MylesBorins added dont-land-on-v4.x and removed lts-watch-v4.x labels Nov 18, 2016

MylesBorins mentioned this pull request Nov 22, 2016

v4.7.0 proposal #9736

Merged

addaleax deleted the fix-stream-unpipe-regression branch December 6, 2016 16:03


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

Uh oh!

Conversation

addaleax commented Oct 18, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

evanlucas left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 18, 2016

Uh oh!

niels4 commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Oct 18, 2016

Uh oh!

lpinca Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

addaleax Oct 18, 2016

Choose a reason for hiding this comment

Uh oh!

lpinca left a comment

Choose a reason for hiding this comment

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Oct 19, 2016

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

MylesBorins commented Oct 19, 2016

Uh oh!

MylesBorins commented Oct 19, 2016

Uh oh!

MylesBorins commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

niels4 commented Oct 18, 2016 •

edited

Loading

MylesBorins commented Oct 19, 2016 •

edited

Loading