FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

stream: emit 'pause' on unpipe by ronag · Pull Request #32476 · nodejs/node · GitHub

/ node Public

stream: emit 'pause' on unpipe - #32476

Closed
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-unpipe-pause
ConversationCommits (1)ChecksFiles changed
Closed

stream: emit 'pause' on unpipe#32476
ronag wants to merge 1 commit into
nodejs:masterfrom
nxtedition:stream-unpipe-pause

Conversation

ronag commented Mar 24, 2020

Copy link
Copy Markdown
Member

unpipe should use pause() instead of mutating
state.flowing directly so that pausing side
effects such as emitting 'pause' are properly
performed.

Fixes: #32470

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

stream: emit 'pause' on unpipe
6fce298 
unpipe should use pause() instead of mutating
state.flowing directly so that pausing side
effects such as emitting 'pause' are properly
performed.

Fixes: nodejs#32470
nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Mar 24, 2020
ronag requested review from addaleax and mcollina March 25, 2020 12:04
addaleax approved these changes Mar 25, 2020
View reviewed changes

addaleax 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think this might also fix #32291?

mcollina approved these changes Mar 25, 2020
View reviewed changes

mcollina 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

lgtm

bricss mentioned this pull request Mar 25, 2020
Merged

nodejs-github-bot commented Mar 26, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30087/

MylesBorins commented Mar 26, 2020

Copy link
Copy Markdown
Contributor

Should we fast track this and get it included in v13.12.0? I don't know if it should so this should not count as 1 of the 2 +1s

MylesBorins added the fast-track PRs that do not need to wait for 48 hours to land. label Mar 26, 2020

ronag commented Mar 26, 2020 •
edited
Loading

Copy link
Copy Markdown
Member Author

This doesn't have green CI yet. Maybe flaky test (test.parallel/test-tls-root-certificates)? I'll do a rerun/resume once it's complete (osx always takes time).

I seem to be encountering quite a lot of flakiness nowadays.

MylesBorins commented Mar 26, 2020

Copy link
Copy Markdown
Contributor

test.parallel/test-tls-root-certificates is failing in the js suite for a variety of PRs and seems to be flaky on master right now. Unsure if this is an infrastructure issue or not.

https://github.com/nodejs/build/issues/2252

ronag commented Mar 26, 2020

Copy link
Copy Markdown
Member Author

@MylesBorins Does that mean I can land this on master without green CI or should I keep trying until green CI is achieved?

MylesBorins commented Mar 26, 2020

Copy link
Copy Markdown
Contributor

@ronag kicked off CI again. We should try and get green CI

nodejs-github-bot commented Mar 26, 2020

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/30094/

MylesBorins pushed a commit that referenced this pull request Mar 26, 2020
stream: emit 'pause' on unpipe
0d0f151 
unpipe should use pause() instead of mutating
state.flowing directly so that pausing side
effects such as emitting 'pause' are properly
performed.

Fixes: #32470

PR-URL: #32476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>

MylesBorins commented Mar 26, 2020

Copy link
Copy Markdown
Contributor

We did it!!!

Landed in 0d0f151

MylesBorins pushed a commit that referenced this pull request Mar 26, 2020
stream: emit 'pause' on unpipe
56da8df 
unpipe should use pause() instead of mutating
state.flowing directly so that pausing side
effects such as emitting 'pause' are properly
performed.

Fixes: #32470

PR-URL: #32476
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

mcollina mcollina approved these changes
addaleax addaleax approved these changes
+1 more reviewer
hassaanp hassaanp approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fast-track PRs that do not need to wait for 48 hours to land. stream Issues and PRs related to the stream subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Readable Pause event not triggered when unpipe causes the pause

7 participants

Back | FazBrowse Home | New Git URL