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.

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

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

Closed

Conversation

MoonBall
Copy link
Member

@MoonBall MoonBall commented Jan 20, 2018

Refs: #12746

After a readable stream pipes more than one writable streams, I calls readable.unpipe() to remove all piped writable stream. I find that only the first piped writable stream calls the cleanup(), the others don't call cleanup().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

stream

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 20, 2018
@@ -758,8 +758,10 @@ Readable.prototype.unpipe = function(dest) {
state.pipesCount = 0;
state.flowing = false;

for (var i = 0; i < len; i++)
for (var i = 0; i < len; i++) {
unpipeInfo = { hasUnpiped: false };
Copy link
Member

@lpinca lpinca Jan 20, 2018

Choose a reason for hiding this comment

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

For other reviewers, we need a new object becausedest sets unpipeInfo.hasUnpiped to true when 'unpipe' is emitted.

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, doesn't this make the hasUnpiped guard useless? I mean, no matter what, dest will always find hasUnpiped false.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found the PR which adds the hasUnpiped. you can see it.

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 hasUnpiped solved the situation that a readable stram piped the same writable stream more than one time. This change has no influence to it.

Copy link
Member

Choose a reason for hiding this comment

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

Ok yes, it seems correct as this is for the "remove all" case.

@@ -758,8 +758,10 @@ Readable.prototype.unpipe = function(dest) {
state.pipesCount = 0;
state.flowing = false;

for (var i = 0; i < len; i++)
for (var i = 0; i < len; i++) {
unpipeInfo = { hasUnpiped: false };
dests[i].emit('unpipe', this, unpipeInfo);
Copy link
Member

Choose a reason for hiding this comment

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

Tiny nit: I'd just create the new object here, without using the above assignment.

Copy link
Member Author

@MoonBall MoonBall Jan 20, 2018

Choose a reason for hiding this comment

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

There are three situations to use unpipeInfo. The above assignment can be used in two situations. I’m also fine to assign it just before using it.

@@ -758,8 +758,12 @@ Readable.prototype.unpipe = function(dest) {
state.pipesCount = 0;
state.flowing = false;

for (var i = 0; i < len; i++)
// use the above assignment
dests[0].emit('unpipe', this, unpipeInfo);
Copy link
Member

Choose a reason for hiding this comment

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

What I meant in my previous comment was:

for (var i = 0; i < len; i++)
  dests[i].emit('unpipe', this, { hasUnpiped: false });

Copy link
Member Author

Choose a reason for hiding this comment

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

ok~

@MoonBall MoonBall force-pushed the fix-unpiping-all-piped-streams branch from 08153b1 to 3500d5f Compare January 20, 2018 08:26
@targos
Copy link
Member

targos commented Jan 20, 2018

/cc @nodejs/streams

Copy link
Member

@apapirovski apapirovski left a comment

Choose a reason for hiding this comment

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

The fix here seems good although I would prefer a test that accounts for more stuff within cleanup().

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

@MoonBall
Copy link
Member Author

MoonBall commented Jan 20, 2018

@apapirovski I added more test contents.


const unpipeChecker = common.mustCall(() => {
assert.strictEqual(
dest.listenerCount('unpipe'), 1,
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I'd simplify this as

assert.deepStrictEqual(dest.listeners('unpipe'), [unpipeChecker]);

Copy link
Member Author

Choose a reason for hiding this comment

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

ok~

srcCheckEventNames.forEach((eventName) => {
assert.strictEqual(
source.listenerCount(eventName), 0,
`source's '${eventName}' event listeners don't be cleaned up`
Copy link
Member

Choose a reason for hiding this comment

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

Nit: replace "don't be cleaned up" with "not removed", same below.

Copy link
Member Author

Choose a reason for hiding this comment

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

ok~

@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 1, 2018
@BridgeAR
Copy link
Member

BridgeAR commented Feb 1, 2018

@addaleax
Copy link
Member

addaleax commented Feb 4, 2018

@MoonBall I think with this we can remove unpipeInfo altogether?

@MoonBall
Copy link
Member Author

MoonBall commented Feb 4, 2018

@addaleax The #12746 will be reverted If we remove unpipeInfo altogether. This PR didn't change the intention of #12746. It just fixed that calling unpipe() doesn't remove all piped stream.

mcollina pushed a commit that referenced this pull request Feb 5, 2018
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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
@mcollina
Copy link
Member

mcollina commented Feb 5, 2018

Landed as a52b7a2.

@mcollina mcollina closed this Feb 5, 2018
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
MylesBorins pushed a commit that referenced this pull request Feb 21, 2018
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 <[email protected]>
Reviewed-By: Anatoli Papirovski <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.