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.

Sign up for GitHub

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

Jump to bottom

stream: make sure _destroy is called on the tail #53213

Merged
merged 5 commits into from
Jun 10, 2024
Merged

stream: make sure _destroy is called on the tail #53213

merged 5 commits into from
Jun 10, 2024

Conversation

jakecastelli
Copy link
Contributor

@jakecastelli jakecastelli commented May 30, 2024 •
edited
Loading

Fixes: #51987 (attempt to).

Please let me know if I am on the right track as I could be missing some important stuff here. If the fix is on the right track I will add a test.

EDIT: I have added a test.

@jakecastelli jakecastelli marked this pull request as ready for review May 30, 2024 08:05
@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/streams

@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label May 30, 2024
@ronag
Copy link
Member

ronag commented May 30, 2024

Do you think you could make a test?

@ronag
Copy link
Member

ronag commented May 30, 2024

I will have a look as soon as I have time

@ronag ronag requested a review from mcollina May 30, 2024 20:54
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. label May 30, 2024
@ronag
Copy link
Member

ronag commented May 30, 2024

Good find and fix!

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 30, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@jakecastelli @ronag
Update lib/internal/streams/compose.js
5d93185 
Co-authored-by: Robert Nagy <ronagy@icloud.com>
@ronag ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 31, 2024
mcollina
mcollina approved these changes May 31, 2024
View reviewed changes
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

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 31, 2024
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

mcollina
mcollina reviewed Jun 4, 2024
View reviewed changes
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.

Can you avoid the use of timeouts in the test? They are usually pretty brittle to maintain.

test/parallel/test-stream-compose.js Outdated Show resolved Hide resolved
@jakecastelli
Copy link
Contributor Author

jakecastelli commented Jun 4, 2024 •
edited
Loading

Can you avoid the use of timeouts in the test? They are usually pretty brittle to maintain.

Do you mean avoiding using setTimeout? maybe I can use setImmediate or process.nextTick instead?

@jakecastelli
Copy link
Contributor Author

jakecastelli commented Jun 4, 2024 •
edited
Loading

@mcollina I had a deeper look, I think I understood what you mean, the issue here is that _destroy is not called on the tail stream node, no matter if its a slow process or not.

I have removed the timeouts as they are not needed.

Although a quick question - does setImmediate usually cause less issue?

@mcollina
Copy link
Member

mcollina commented Jun 4, 2024

Yes, it's more predictable.

@jakecastelli
Copy link
Contributor Author

since the test has changed, shall we kick start another CI? 👀

@mcollina mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 8, 2024
@nodejs-github-bot
Copy link
Collaborator

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

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

Commit Queue failed
- Loading data for nodejs/node/pull/53213
✔  Done loading data for nodejs/node/pull/53213
----------------------------------- PR info ------------------------------------
Title      stream: make sure _destroy is called on the tail (#53213)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     jakecastelli:fix-51987 -> nodejs:main
Labels     author ready, needs-ci
Commits    5
 - stream: make sure _destroy is called
 - fixup! add test
 - Update lib/internal/streams/compose.js
 - fixup! use common.platformTimeout
 - fixup! remove slow process as its not needed
Committers 2
 - jakecastelli <959672929@qq.com>
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/53213
Fixes: https://github.com/nodejs/node/issues/51987
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53213
Fixes: https://github.com/nodejs/node/issues/51987
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ⚠  Commits were pushed since the last approving review:
   ⚠  - fixup! use common.platformTimeout
   ⚠  - fixup! remove slow process as its not needed
   ℹ  This PR was created on Thu, 30 May 2024 08:05:30 GMT
   ✔  Approvals: 3
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/53213#pullrequestreview-2089357194
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/53213#pullrequestreview-2091025829
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/53213#pullrequestreview-2094796152
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-06-08T15:47:51Z: https://ci.nodejs.org/job/node-test-pull-request/59699/
- Querying data for job/node-test-pull-request/59699/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/9449666312

mcollina
mcollina approved these changes Jun 10, 2024
View reviewed changes
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

@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 Jun 10, 2024
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 10, 2024
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/53213
✔  Done loading data for nodejs/node/pull/53213
----------------------------------- PR info ------------------------------------
Title      stream: make sure _destroy is called on the tail (#53213)
   ⚠  Could not retrieve the email or name of the PR author's from user's GitHub profile!
Branch     jakecastelli:fix-51987 -> nodejs:main
Labels     author ready, needs-ci
Commits    5
 - stream: make sure _destroy is called
 - fixup! add test
 - Update lib/internal/streams/compose.js
 - fixup! use common.platformTimeout
 - fixup! remove slow process as its not needed
Committers 2
 - jakecastelli <959672929@qq.com>
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/53213
Fixes: https://github.com/nodejs/node/issues/51987
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/53213
Fixes: https://github.com/nodejs/node/issues/51987
Reviewed-By: Robert Nagy 
Reviewed-By: Matteo Collina 
Reviewed-By: Benjamin Gruenbaum 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 30 May 2024 08:05:30 GMT
   ✔  Approvals: 3
   ✔  - Robert Nagy (@ronag) (TSC): https://github.com/nodejs/node/pull/53213#pullrequestreview-2089357194
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/53213#pullrequestreview-2107988745
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/53213#pullrequestreview-2094796152
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2024-06-10T13:56:39Z: https://ci.nodejs.org/job/node-test-pull-request/59699/
- Querying data for job/node-test-pull-request/59699/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 53213
From https://github.com/nodejs/node
 * branch                  refs/pull/53213/merge -> FETCH_HEAD
✔  Fetched commits as 2dea6a4520b8..d8a6156f7cb8
--------------------------------------------------------------------------------
[main 707c3ff394] stream: make sure _destroy is called
 Author: jakecastelli <959672929@qq.com>
 Date: Thu May 30 17:21:15 2024 +0930
 1 file changed, 3 insertions(+), 3 deletions(-)
[main 2dc4090f69] fixup! add test
 Author: jakecastelli <959672929@qq.com>
 Date: Thu May 30 22:54:57 2024 +0930
 1 file changed, 49 insertions(+)
[main 7866fd2a22] Update lib/internal/streams/compose.js
 Author: jakecastelli <38635403+jakecastelli@users.noreply.github.com>
 Date: Fri May 31 08:55:59 2024 +0930
 1 file changed, 2 insertions(+), 1 deletion(-)
[main 008624d2da] fixup! use common.platformTimeout
 Author: jakecastelli <959672929@qq.com>
 Date: Tue Jun 4 20:49:49 2024 +0930
 1 file changed, 2 insertions(+), 2 deletions(-)
[main 5e225afa4d] fixup! remove slow process as its not needed
 Author: jakecastelli <959672929@qq.com>
 Date: Tue Jun 4 23:44:27 2024 +0930
 1 file changed, 13 insertions(+), 17 deletions(-)
   ✔  Patches applied
There are 5 commits in the PR. Attempting autorebase.
Rebasing (2/10)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

stream: make sure _destroy is called


PR-URL: #53213

Fixes: #51987

Reviewed-By: Robert Nagy ronagy@icloud.com

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com


[detached HEAD 2d4a067242] stream: make sure _destroy is called

Author: jakecastelli 959672929@qq.com

Date: Thu May 30 17:21:15 2024 +0930

1 file changed, 3 insertions(+), 3 deletions(-)

Rebasing (3/10)

Rebasing (4/10)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup! add test


PR-URL: #53213

Fixes: #51987

Reviewed-By: Robert Nagy ronagy@icloud.com

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com


[detached HEAD 20581668fb] fixup! add test

Author: jakecastelli 959672929@qq.com

Date: Thu May 30 22:54:57 2024 +0930

1 file changed, 49 insertions(+)

Rebasing (5/10)

Rebasing (6/10)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

Update lib/internal/streams/compose.js


Co-authored-by: Robert Nagy ronagy@icloud.com

PR-URL: #53213

Fixes: #51987

Reviewed-By: Robert Nagy ronagy@icloud.com

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com


[detached HEAD f51b69502a] Update lib/internal/streams/compose.js

Author: jakecastelli 38635403+jakecastelli@users.noreply.github.com

Date: Fri May 31 08:55:59 2024 +0930

1 file changed, 2 insertions(+), 1 deletion(-)

Rebasing (7/10)

Rebasing (8/10)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup! use common.platformTimeout


PR-URL: #53213

Fixes: #51987

Reviewed-By: Robert Nagy ronagy@icloud.com

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com


[detached HEAD a3ef6ef184] fixup! use common.platformTimeout

Author: jakecastelli 959672929@qq.com

Date: Tue Jun 4 20:49:49 2024 +0930

1 file changed, 2 insertions(+), 2 deletions(-)

Rebasing (9/10)

Rebasing (10/10)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fixup! remove slow process as its not needed


PR-URL: #53213

Fixes: #51987

Reviewed-By: Robert Nagy ronagy@icloud.com

Reviewed-By: Matteo Collina matteo.collina@gmail.com

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com


[detached HEAD 023c600074] fixup! remove slow process as its not needed

Author: jakecastelli 959672929@qq.com

Date: Tue Jun 4 23:44:27 2024 +0930

1 file changed, 13 insertions(+), 17 deletions(-)


Successfully rebased and updated refs/heads/main.


ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/9450518114

@mcollina mcollina 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 Jun 10, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 10, 2024
@nodejs-github-bot nodejs-github-bot merged commit 8e6901a into nodejs:main Jun 10, 2024
71 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 8e6901a

targos pushed a commit that referenced this pull request Jun 20, 2024
@jakecastelli @targos
stream: make sure _destroy is called
f4efb7f 
PR-URL: #53213
Fixes: #51987
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
EliphazBouye pushed a commit to EliphazBouye/node that referenced this pull request Jun 20, 2024
@jakecastelli @EliphazBouye
stream: make sure _destroy is called
46c84a6 
PR-URL: nodejs#53213
Fixes: nodejs#51987
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
bmeck pushed a commit to bmeck/node that referenced this pull request Jun 22, 2024
@jakecastelli @bmeck
stream: make sure _destroy is called
426e4d8 
PR-URL: nodejs#53213
Fixes: nodejs#51987
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@targos targos mentioned this pull request Jun 25, 2024
Merged
@Karliz24 Karliz24 mentioned this pull request Jul 9, 2024
Open
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@jakecastelli @marco-ippolito
stream: make sure _destroy is called
76cb4df 
PR-URL: #53213
Fixes: #51987
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
marco-ippolito pushed a commit that referenced this pull request Jul 19, 2024
@jakecastelli @marco-ippolito
stream: make sure _destroy is called
1b46ebb 
PR-URL: #53213
Fixes: #51987
Reviewed-By: Robert Nagy <ronagy@icloud.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
@marco-ippolito marco-ippolito mentioned this pull request Jul 19, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mcollina mcollina mcollina approved these changes

@benjamingr benjamingr benjamingr approved these changes

@ronag ronag ronag approved these changes

Assignees
No one assigned
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. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

stream.compose(...) doesn't destroy all active composed streams when it is destroyed
5 participants
@jakecastelli @nodejs-github-bot @ronag @mcollina @benjamingr