Skip to content

stream: sync stream unpipe resume - #31191

Closed
ronag wants to merge 4 commits into
nodejs:masterfrom
nxtedition:stream-readable-unpipe-resume
Closed

stream: sync stream unpipe resume#31191
ronag wants to merge 4 commits into
nodejs:masterfrom
nxtedition:stream-readable-unpipe-resume

Conversation

@ronag

@ronagronag commented Jan 5, 2020

Copy link
Copy Markdown
Member

pipe() ondata should not control flow state if cleaned up.

Fixes: #31190

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

pipe() ondata should not control flow state if cleaned up.
Fixes: nodejs#31190
@nodejs-github-botnodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jan 5, 2020
Comment threadtest/parallel/test-stream-readable-unpipe-resume.js Outdated
@ronag

ronag commented Jan 5, 2020

Copy link
Copy Markdown
MemberAuthor

The problem here is that we call unpipe() and then resume(). resume() will schedule a resume_ in the next tick. However, before that a 'data' event is emitted causing ondata to explicitly pause the stream, which stops resume_ from re-flowing the stream.

@addaleaxaddaleax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM if CI (and linter) are green

Comment threadtest/parallel/test-stream-readable-unpipe-resume.js Outdated
@ronag
ronagforce-pushed the stream-readable-unpipe-resume branch from 801fa99 to 786fcbbCompareJanuary 5, 2020 11:06
@ronag
ronagforce-pushed the stream-readable-unpipe-resume branch from 786fcbb to c9cd445CompareJanuary 5, 2020 11:07
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@BridgeARBridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 6, 2020

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@addaleax

Copy link
Copy Markdown
Member

Landed in b9e6c1b :)

@addaleaxaddaleax closed this Jan 7, 2020
addaleax pushed a commit that referenced this pull request Jan 7, 2020
pipe() ondata should not control flow state if cleaned up.
Fixes: #31190
PR-URL: #31191
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
pipe() ondata should not control flow state if cleaned up.
Fixes: #31190
PR-URL: #31191
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
@codebyterecodebytere mentioned this pull request Jan 16, 2020
@codebytere

Copy link
Copy Markdown
Member

@ronag this looks like it'll need a manual backport to v12.x, if we think it should land there. I'll add the label but feel free to swap it!

@ronag

Copy link
Copy Markdown
MemberAuthor

@codebytere: #32264

MylesBorins pushed a commit to MylesBorins/node that referenced this pull request Apr 1, 2020
pipe() ondata should not control flow state if cleaned up.
Fixes: nodejs#31190
Backport-PR-URL: nodejs#32264
PR-URL: nodejs#31191
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
ronag added a commit to nxtedition/node that referenced this pull request Apr 1, 2020
pipe() ondata should not control flow state if cleaned up.
Fixes: nodejs#31190
PR-URL: nodejs#31191
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: nodejs#32264
MylesBorins pushed a commit that referenced this pull request Apr 1, 2020
pipe() ondata should not control flow state if cleaned up.
Fixes: #31190
Backport-PR-URL: #32264
PR-URL: #31191
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Backport-PR-URL: #32264
@codebyterecodebytere mentioned this pull request Apr 3, 2020
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.streamIssues and PRs related to the stream subsystem.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

unpipe() and resume() in _transform()

9 participants

@ronag@nodejs-github-bot@addaleax@codebytere@mcollina@Trott@lpinca@MylesBorins@BridgeAR