stream: fix y.pipe(x)+y.pipe(x)+y.unpipe(x) scenario #12746

addaleax · 2017-04-29T18:33:34Z

Fix the uncommon situation when a readable stream is piped twice into
the same destination stream, and then unpiped once.

Previously, the unpipe event handlers weren’t able to tell whether
they were corresponding to the “right” conceptual pipe that was being
removed; this fixes this by adding a counter to the unpipe event
handler and only removing a single piping destination at most.

Fixes: #12718

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
commit message follows commit guidelines

Affected core subsystem(s)

stream

addaleax · 2017-04-29T18:37:00Z

CI: https://ci.nodejs.org/job/node-test-commit/9508/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/753/
(Very recent master CITGM for comparison: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/752/. Also, I hope I did the right thing by checking the DISABLE_READABLE_STREAM box)

Edit: CI failures are infra-related, CITGM compares good to master

/cc @nodejs/streams

mscdex · 2017-04-29T19:02:54Z

lib/_stream_readable.js

    debug('onunpipe');
    if (readable === src) {
-      cleanup();
+      if (unpipeInfo.counter++ === 0) {


Nit: how about only incrementing inside to avoid incrementing when unnecessary? Or just use a boolean?

@mscdex I’m fine either way, updated this to switch to a boolean

benjamingr

Changes LGTM assuming CI and CITGM are green, we might want to mark this as semver-minor for safety reasons as it changes stream behavior.

addaleax · 2017-04-29T19:40:26Z

we might want to mark this as semver-minor for safety reasons as it changes stream behavior

Not sure, it isn’t really adding anything … if you think it’s a change that should be pointed out explicitly, then adding notable-change sounds better. But really, this is a very very niche use case that’s being fixed, even for streams…

calvinmetcalf · 2017-04-30T14:19:15Z

Wanna just discuss this at the face to face this week?

…

On Sat, Apr 29, 2017, 3:40 PM Anna Henningsen ***@***.***> wrote: we might want to mark this as semver-minor for safety reasons as it changes stream behavior Not sure, it isn’t really adding anything … if you think it’s a change that should be pointed out explicitly, then adding notable-change sounds better. But really, this is a very very niche use case that’s being fixed, even for streams… — You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub <#12746 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABE4nxSRpld7Miewyr6PQxWfxpH0wWcSks5r05JHgaJpZM4NMV5N> .

addaleax · 2017-04-30T14:22:22Z

@calvinmetcalf I mean, sure, if you think that that makes anything easier, we can talk in person … but it might be good to know what you’d like to discuss? 😄

calvinmetcalf · 2017-04-30T14:45:19Z

Whether this is semver minor or not

…

On Sun, Apr 30, 2017, 10:22 AM Anna Henningsen ***@***.***> wrote: @calvinmetcalf <https://github.com/calvinmetcalf> I mean, sure, if you think that that makes anything easier, we can talk in person … but it might be good to know what you’d like to discuss? 😄 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12746 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABE4n3-mSiFqWW2aSkM7sWLRHM6CgGAiks5r1Jk9gaJpZM4NMV5N> .

addaleax · 2017-04-30T14:54:17Z

Sure … but I don’t have a problem discussing it here, either. I know the streams WG has a good chance of being quite complete in Berlin, but there might be other people with thoughts on this that can’t make it there. (That doesn’t mean we can’t talk about it in person if we don’t come to a conclusion here).

I would say that this is semver-patch, because it fixes a bug; the current behaviour is definitely broken (or needs to be document as something that’s just fundamentally not supported), and this is a way to make the behaviour at least consistent in a way that would match my expectations as a streams user, and presumably @ovikholt’s expectations, too.

I would say that this is not semver-major because the edge case that is affected by this change would just be broken before, so there’s no reason to believe anybody relies on it.

I would say that this is not semver-minor because it doesn’t add a feature to the API. It does add another parameter to an event callback, but one that’s for internal use, that’s intentionally undocumented and that userland applications won’t really be able to use. I wish there would be an easier way to hide it, but I don’t see any (at least as long as we’re also talking about solutions that include all environments supported by readable-stream).

If we want to be careful, then landing it in current and waiting a reasonably long time before backporting it to LTS seems like a better approach than assigning it a semver level out of caution.

mcollina · 2017-05-01T14:53:05Z

I'm not convinced of this PR. I think y.pipe(x); y.pipe(x); should be discouraged, and I am wondering why it was supported in the first place. Maybe we should stop supporting piping the same stream multiple times to the same destination, but that would be a semver-major change.

Would you mind adding a test that does y.pipe(x); y.pipe(x); y.unpipe(x); y.unpipe(x); and check that everything is still in order?

addaleax · 2017-05-01T17:00:04Z

I'm not convinced of this PR. I think y.pipe(x); y.pipe(x); should be discouraged, and I am wondering why it was supported in the first place. Maybe we should stop supporting piping the same stream multiple times to the same destination, but that would be a semver-major change.

I agree, supporting that is odd (especially for non-object mode) and we should consider un-supporting it. Are you okay with this change as a bugfix, though?

Would you mind adding a test that does y.pipe(x); y.pipe(x); y.unpipe(x); y.unpipe(x); and check that everything is still in order?

Done!

mcollina · 2017-05-02T13:48:58Z

I agree, supporting that is odd (especially for non-object mode) and we should consider un-supporting it. Are you okay with this change as a bugfix, though?

Yes, I'm ok as being a backportable bugfix. We can aim to port this to lts as well.

mcollina

LGTM

mcollina · 2017-05-02T13:50:37Z

Just one nit, can you amend the commit and the title to the PR to y.pipe(x)+y.pipe(x)+y.unpipe(x), as otherwise I've read it wrongly the first time y.pipe(x).pipe(x).unpipe(x), which has a completely different meaning.

Fix the uncommon situation when a readable stream is piped twice into the same destination stream, and then unpiped once. Previously, the `unpipe` event handlers weren’t able to tell whether they were corresponding to the “right” conceptual pipe that was being removed; this fixes this by adding a counter to the `unpipe` event handler and only removing a single piping destination at most. Fixes: nodejs#12718

addaleax · 2017-05-02T13:54:08Z

Just one nit, can you amend the commit and the title to the PR to y.pipe(x)+y.pipe(x)+y.unpipe(x)

Done!

mcollina · 2017-05-02T15:02:10Z

Fantastic, you can go ahead and merge this for me. @calvinmetcalf have you got any other concerns?

calvinmetcalf · 2017-05-02T15:30:16Z

no I'm all set

…

On Tue, May 2, 2017 at 5:03 PM Matteo Collina ***@***.***> wrote: Fantastic, you can go ahead and merge this for me. @calvinmetcalf <https://github.com/calvinmetcalf> have you got any other concerns? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#12746 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABE4n-jc2r-U1O3O80P8yYh-TBj0Pameks5r10WvgaJpZM4NMV5N> .

addaleax · 2017-05-03T13:35:47Z

Landed in 6993eb0

This is streams code and a very edge-case-y thing, so it might be good to let it sit in Current for a while before backporting.

Fix the uncommon situation when a readable stream is piped twice into the same destination stream, and then unpiped once. Previously, the `unpipe` event handlers weren’t able to tell whether they were corresponding to the “right” conceptual pipe that was being removed; this fixes this by adding a counter to the `unpipe` event handler and only removing a single piping destination at most. Fixes: #12718 PR-URL: #12746 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Fix the uncommon situation when a readable stream is piped twice into the same destination stream, and then unpiped once. Previously, the `unpipe` event handlers weren’t able to tell whether they were corresponding to the “right” conceptual pipe that was being removed; this fixes this by adding a counter to the `unpipe` event handler and only removing a single piping destination at most. Fixes: nodejs#12718 PR-URL: nodejs#12746 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

Refs: nodejs#12746

This PR makes sure the object emitted as the 'unpipe' event in the destination stream is not shared between destination, as it would be muted. Refs: #12746 PR-URL: #18266 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

lpinca · 2018-04-16T09:02:11Z

@MylesBorins any reason to not backport this to v6.x?

MylesBorins · 2018-04-16T18:19:23Z

@lpinca I opted to not land this as @addaleax mentioned it was edgecasey and #18266 showed there was at least one regression.

More than happy to backport both if it makes sense.

addaleax · 2018-04-16T18:32:32Z

I think backporting both should be fine by now, it’s been almost a year. :)

lpinca · 2018-04-16T19:02:48Z

+1 for backporting both.

MylesBorins · 2018-04-16T19:50:35Z

@lpinca done 🎉

Will report back if there is any weirdness

Fix the uncommon situation when a readable stream is piped twice into the same destination stream, and then unpiped once. Previously, the `unpipe` event handlers weren’t able to tell whether they were corresponding to the “right” conceptual pipe that was being removed; this fixes this by adding a counter to the `unpipe` event handler and only removing a single piping destination at most. Fixes: #12718 PR-URL: #12746 Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

This PR makes sure the object emitted as the 'unpipe' event in the destination stream is not shared between destination, as it would be muted. Refs: #12746 PR-URL: #18266 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

This PR makes sure the object emitted as the 'unpipe' event in the destination stream is not shared between destination, as it would be muted. Refs: nodejs#12746 PR-URL: nodejs#18266 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net>

addaleax added lts-watch-v4.x stream Issues and PRs related to the stream subsystem. labels Apr 29, 2017

addaleax mentioned this pull request Apr 29, 2017

Piping twice to same stream then unpiping loses data #12718

Closed

mscdex reviewed Apr 29, 2017

View reviewed changes

jasnell requested a review from mcollina April 29, 2017 19:35

benjamingr approved these changes Apr 29, 2017

View reviewed changes

addaleax force-pushed the stream-pipe-same-destination-twice branch from 81ad497 to ca8f430 Compare April 29, 2017 19:42

addaleax force-pushed the stream-pipe-same-destination-twice branch from ca8f430 to 87cdba9 Compare May 1, 2017 17:00

mcollina approved these changes May 2, 2017

View reviewed changes

addaleax force-pushed the stream-pipe-same-destination-twice branch from 87cdba9 to 86b9243 Compare May 2, 2017 13:52

mcollina changed the title ~~stream: fix pipe(x)+pipe(x)+unpipe(x) scenario~~ stream: fix y.pipe(x)+y.pipe(x)+y.unpipe(x) scenario May 2, 2017

addaleax closed this May 3, 2017

addaleax deleted the stream-pipe-same-destination-twice branch May 3, 2017 13:35

jasnell mentioned this pull request May 11, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the baking-for-lts PRs that need to wait before landing in a LTS release. label Jun 18, 2017

MoonBall mentioned this pull request Jan 20, 2018

stream: fix not calling cleanup() when unpiping all streams. #18266

Closed

4 tasks

MoonBall pushed a commit to MoonBall/node that referenced this pull request Jan 20, 2018

stream: fix not calling cleanup() when unpiping all streams.

3500d5f

Refs: nodejs#12746

MylesBorins added dont-land-on-v6.x and removed baking-for-lts PRs that need to wait before landing in a LTS release. lts-watch-v4.x labels Apr 13, 2018

MylesBorins added land-on-v6.x and removed dont-land-on-v6.x labels Apr 16, 2018

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

stream: fix y.pipe(x)+y.pipe(x)+y.unpipe(x) scenario #12746

stream: fix y.pipe(x)+y.pipe(x)+y.unpipe(x) scenario #12746

addaleax commented Apr 29, 2017

addaleax commented Apr 29, 2017 •

edited

mscdex Apr 29, 2017

addaleax Apr 29, 2017

benjamingr left a comment

addaleax commented Apr 29, 2017

calvinmetcalf commented Apr 30, 2017 via email

addaleax commented Apr 30, 2017

calvinmetcalf commented Apr 30, 2017 via email

addaleax commented Apr 30, 2017

mcollina commented May 1, 2017

addaleax commented May 1, 2017

mcollina commented May 2, 2017

mcollina left a comment

mcollina commented May 2, 2017

addaleax commented May 2, 2017

mcollina commented May 2, 2017

calvinmetcalf commented May 2, 2017 via email

addaleax commented May 3, 2017

lpinca commented Apr 16, 2018 •

edited

MylesBorins commented Apr 16, 2018

addaleax commented Apr 16, 2018

lpinca commented Apr 16, 2018

MylesBorins commented Apr 16, 2018

stream: fix y.pipe(x)+y.pipe(x)+y.unpipe(x) scenario #12746

stream: fix y.pipe(x)+y.pipe(x)+y.unpipe(x) scenario #12746

Conversation

addaleax commented Apr 29, 2017

Checklist

Affected core subsystem(s)

addaleax commented Apr 29, 2017 • edited

mscdex Apr 29, 2017

Choose a reason for hiding this comment

addaleax Apr 29, 2017

Choose a reason for hiding this comment

benjamingr left a comment

Choose a reason for hiding this comment

addaleax commented Apr 29, 2017

calvinmetcalf commented Apr 30, 2017 via email

addaleax commented Apr 30, 2017

calvinmetcalf commented Apr 30, 2017 via email

addaleax commented Apr 30, 2017

mcollina commented May 1, 2017

addaleax commented May 1, 2017

mcollina commented May 2, 2017

mcollina left a comment

Choose a reason for hiding this comment

mcollina commented May 2, 2017

addaleax commented May 2, 2017

mcollina commented May 2, 2017

calvinmetcalf commented May 2, 2017 via email

addaleax commented May 3, 2017

lpinca commented Apr 16, 2018 • edited

MylesBorins commented Apr 16, 2018

addaleax commented Apr 16, 2018

lpinca commented Apr 16, 2018

MylesBorins commented Apr 16, 2018

addaleax commented Apr 29, 2017 •

edited

lpinca commented Apr 16, 2018 •

edited