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: resume stream on drain #41848

Merged
merged 2 commits into from Feb 6, 2022
Merged

Conversation

ronag
Copy link
Member

@ronag ronag commented Feb 4, 2022

Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: #41785

Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: nodejs#41785
@ronag ronag requested a review from mcollina February 4, 2022 09:38
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@ronag ronag requested a review from benjamingr February 4, 2022 09:38
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Feb 4, 2022
@ronag ronag added request-ci Add this label to start a Jenkins CI on a PR. stream Issues and PRs related to the stream subsystem. and removed needs-ci PRs that need a full CI run. labels Feb 4, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022
@nodejs-github-bot

This comment was marked as outdated.

@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 4, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Feb 4, 2022
@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 5, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 5, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41848
✔  Done loading data for nodejs/node/pull/41848
----------------------------------- PR info ------------------------------------
Title      stream: resume stream on drain (#41848)
Author     Robert Nagy  (@ronag)
Branch     ronag:pipe-resume -> nodejs:master
Labels     stream, author ready, lts-watch-v14.x, lts-watch-v16.x
Commits    2
 - stream: resume stream on drain
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 04 Feb 2022 09:38:27 GMT
   ✔  Approvals: 3
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/41848#pullrequestreview-872871213
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41848#pullrequestreview-872917307
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/41848#pullrequestreview-873588555
   ✖  This PR needs to wait 23 more hours to land
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-04T09:54:15Z: https://ci.nodejs.org/job/node-test-pull-request/42344/
- Querying data for job/node-test-pull-request/42344/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1799092796

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 5, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41848
✔  Done loading data for nodejs/node/pull/41848
----------------------------------- PR info ------------------------------------
Title      stream: resume stream on drain (#41848)
Author     Robert Nagy  (@ronag)
Branch     ronag:pipe-resume -> nodejs:master
Labels     stream, author ready, commit-queue-failed, lts-watch-v14.x, lts-watch-v16.x
Commits    2
 - stream: resume stream on drain
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 04 Feb 2022 09:38:27 GMT
   ✔  Approvals: 3
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/41848#pullrequestreview-872871213
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41848#pullrequestreview-872917307
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/41848#pullrequestreview-873588555
   ✖  This PR needs to wait 23 more hours to land
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-05T10:30:48Z: https://ci.nodejs.org/job/node-test-pull-request/42344/
- Querying data for job/node-test-pull-request/42344/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1799113734

@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41848
✔  Done loading data for nodejs/node/pull/41848
----------------------------------- PR info ------------------------------------
Title      stream: resume stream on drain (#41848)
Author     Robert Nagy  (@ronag)
Branch     ronag:pipe-resume -> nodejs:master
Labels     stream, author ready, commit-queue-failed, lts-watch-v14.x, lts-watch-v16.x
Commits    2
 - stream: resume stream on drain
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 04 Feb 2022 09:38:27 GMT
   ✔  Approvals: 3
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/41848#pullrequestreview-872871213
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41848#pullrequestreview-872917307
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/41848#pullrequestreview-873588555
   ✖  This PR needs to wait 23 more hours to land
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-05T10:31:11Z: https://ci.nodejs.org/job/node-test-pull-request/42344/
- Querying data for job/node-test-pull-request/42344/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1799130209

@mcollina mcollina removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 5, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41848
✔  Done loading data for nodejs/node/pull/41848
----------------------------------- PR info ------------------------------------
Title      stream: resume stream on drain (#41848)
Author     Robert Nagy  (@ronag)
Branch     ronag:pipe-resume -> nodejs:master
Labels     stream, author ready, lts-watch-v14.x, lts-watch-v16.x
Commits    2
 - stream: resume stream on drain
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 04 Feb 2022 09:38:27 GMT
   ✔  Approvals: 4
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/41848#pullrequestreview-872871213
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41848#pullrequestreview-872917307
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/41848#pullrequestreview-873588555
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41848#pullrequestreview-873948143
   ✖  This PR needs to wait 9 more minutes to land
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-05T10:31:29Z: https://ci.nodejs.org/job/node-test-pull-request/42344/
- Querying data for job/node-test-pull-request/42344/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/1801919364

@mcollina mcollina added commit-queue Add this label to land a pull request using GitHub Actions. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 6, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/41848
✔  Done loading data for nodejs/node/pull/41848
----------------------------------- PR info ------------------------------------
Title      stream: resume stream on drain (#41848)
Author     Robert Nagy  (@ronag)
Branch     ronag:pipe-resume -> nodejs:master
Labels     stream, author ready, lts-watch-v14.x, lts-watch-v16.x
Commits    2
 - stream: resume stream on drain
 - fixup
Committers 1
 - Robert Nagy 
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/41848
Fixes: https://github.com/nodejs/node/issues/41785
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Matteo Collina 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 04 Feb 2022 09:38:27 GMT
   ✔  Approvals: 4
   ✔  - Benjamin Gruenbaum (@benjamingr): https://github.com/nodejs/node/pull/41848#pullrequestreview-872871213
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/41848#pullrequestreview-872917307
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/41848#pullrequestreview-873588555
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/41848#pullrequestreview-873948143
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2022-02-06T09:28:47Z: https://ci.nodejs.org/job/node-test-pull-request/42344/
- Querying data for job/node-test-pull-request/42344/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 41848
From https://github.com/nodejs/node
 * branch                  refs/pull/41848/merge -> FETCH_HEAD
✔  Fetched commits as 1c7a74e6f769..4916f0cde3fb
--------------------------------------------------------------------------------
[master 7e509e5088] stream: resume stream on drain
 Author: Robert Nagy 
 Date: Fri Feb 4 10:36:58 2022 +0100
 2 files changed, 18 insertions(+), 2 deletions(-)
[master f73beb6c3d] fixup
 Author: Robert Nagy 
 Date: Fri Feb 4 10:47:25 2022 +0100
 1 file changed, 1 insertion(+), 2 deletions(-)
   ✔  Patches applied
There are 2 commits in the PR. Attempting autorebase.
Rebasing (2/4)

Executing: git node land --amend --yes
⚠ Found Fixes: #41785, skipping..
--------------------------------- New Message ----------------------------------
stream: resume stream on drain

Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: #41785

PR-URL: #41848
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD dfe7ed6c58] stream: resume stream on drain
Author: Robert Nagy ronagy@icloud.com
Date: Fri Feb 4 10:36:58 2022 +0100
2 files changed, 18 insertions(+), 2 deletions(-)
Rebasing (3/4)
Rebasing (4/4)

Executing: git node land --amend --yes
--------------------------------- New Message ----------------------------------
fixup

PR-URL: #41848
Fixes: #41785
Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com
Reviewed-By: Matteo Collina matteo.collina@gmail.com
Reviewed-By: Luigi Pinca luigipinca@gmail.com
Reviewed-By: James M Snell jasnell@gmail.com

[detached HEAD 16ebaf30d7] fixup
Author: Robert Nagy ronagy@icloud.com
Date: Fri Feb 4 10:47:25 2022 +0100
1 file changed, 1 insertion(+), 2 deletions(-)

Successfully rebased and updated refs/heads/master.

ℹ Use --fixupAll option, squash the PR manually or land the PR from the command line.

https://github.com/nodejs/node/actions/runs/1801962481

@nodejs-github-bot nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Feb 6, 2022
@benjamingr benjamingr added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Feb 6, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 6, 2022
@nodejs-github-bot nodejs-github-bot merged commit 224b78f into nodejs:master Feb 6, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in 224b78f

ruyadorno pushed a commit that referenced this pull request Feb 8, 2022
Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: #41785

PR-URL: #41848
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 2, 2022
Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: #41785

PR-URL: #41848
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 3, 2022
Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: #41785

PR-URL: #41848
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Mar 4, 2022
Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: nodejs#41785

PR-URL: nodejs#41848
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit to danielleadams/node that referenced this pull request Mar 4, 2022
Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: nodejs#41785

PR-URL: nodejs#41848
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 8, 2022
Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: #41785

PR-URL: #41848
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
danielleadams pushed a commit that referenced this pull request Mar 14, 2022
Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: #41785

PR-URL: #41848
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
juanarbol pushed a commit that referenced this pull request Apr 28, 2022
Previously we would just resume "flowing" the stream without
reseting the "paused" state. Fixes this by properly using
pause/resume methods for .pipe.

Fixes: #41785

PR-URL: #41848
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@juanarbol juanarbol mentioned this pull request Apr 28, 2022
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. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Readable.pipe() behaves inconsistently resuming (or not) the source
7 participants