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

streams: fix cloned webstreams not being unref'd #51255

Merged
merged 2 commits into from
Dec 24, 2023

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Dec 22, 2023

When cloning a ReadableStream and WritableStream, both use an internal MessageChannel to communicate with the original stream. Those, however, previously were not unref'd which would lead to the process not exiting if the stream was not fully consumed.

Alternative for: #51131

Fixes: #44985

Semver-major because this can change whether a worker thread stays alive solely because of the existence of the cloned streams vs. needing something else to keep it alive (see the modified test in this PR for instance).

When cloning a `ReadableStream` and `WritableStream`, both use an
internal `MessageChannel` to communicate with the original stream.
Those, however, previously were not unref'd which would lead to the
process not exiting if the stream was not fully consumed.

Fixes: nodejs#44985
@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 22, 2023
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Dec 22, 2023
@tsctx
Copy link
Member

tsctx commented Dec 22, 2023

Thank you!

@jasnell jasnell added the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2023
@jasnell jasnell added request-ci Add this label to start a Jenkins CI on a PR. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Dec 22, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 22, 2023
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Dec 22, 2023

Copy link
Member

@KhafraDev KhafraDev left a comment

Choose a reason for hiding this comment

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

this will hopefully fix a few issues in fetch.

@jasnell jasnell requested a review from anonrig December 22, 2023 15:56
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, but I would really consider this a bugfix rather than a semver-major change.

How about we tag it as "baking for lts" instead and wait if we actually break folks?

@jasnell jasnell added semver-major PRs that contain breaking changes and should be released in the next major version. baking-for-lts PRs that need to wait before landing in a LTS release. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed semver-major PRs that contain breaking changes and should be released in the next major version. needs-ci PRs that need a full CI run. labels Dec 23, 2023
@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 Dec 24, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51255
✔  Done loading data for nodejs/node/pull/51255
----------------------------------- PR info ------------------------------------
Title      streams: fix cloned webstreams not being unref'd (#51255)
Author     James M Snell  (@jasnell)
Branch     jasnell:fixup-webstreams-clone-hang -> nodejs:main
Labels     baking-for-lts, author ready, web streams
Commits    2
 - stream: fix cloned webstreams not being unref'd
 - Update test/parallel/test-whatwg-webstreams-transfer.js
Committers 2
 - James M Snell 
 - GitHub 
PR-URL: https://github.com/nodejs/node/pull/51255
Fixes: https://github.com/nodejs/node/issues/44985
Reviewed-By: Matthew Aitken 
Reviewed-By: Matteo Collina 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51255
Fixes: https://github.com/nodejs/node/issues/44985
Reviewed-By: Matthew Aitken 
Reviewed-By: Matteo Collina 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Fri, 22 Dec 2023 04:21:41 GMT
   ✔  Approvals: 2
   ✔  - Matthew Aitken (@KhafraDev): https://github.com/nodejs/node/pull/51255#pullrequestreview-1794662152
   ✔  - Matteo Collina (@mcollina) (TSC): https://github.com/nodejs/node/pull/51255#pullrequestreview-1795456690
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-12-22T08:53:12Z: https://ci.nodejs.org/job/node-test-pull-request/56461/
   ⚠  Commits were pushed after the last Full PR CI run:
   ⚠  - Update test/parallel/test-whatwg-webstreams-transfer.js
- Querying data for job/node-test-pull-request/56461/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  Aborted `git node land` session in /home/runner/work/node/node/.ncu
https://github.com/nodejs/node/actions/runs/7312493372

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

@nodejs-github-bot
Copy link
Collaborator

@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 Dec 24, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 24, 2023
@nodejs-github-bot nodejs-github-bot merged commit 4d3923a into nodejs:main Dec 24, 2023
71 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 4d3923a

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
When cloning a `ReadableStream` and `WritableStream`, both use an
internal `MessageChannel` to communicate with the original stream.
Those, however, previously were not unref'd which would lead to the
process not exiting if the stream was not fully consumed.

Fixes: #44985
PR-URL: #51255
Reviewed-By: Matthew Aitken <maitken033380023@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
@richardlau richardlau added dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. and removed baking-for-lts PRs that need to wait before landing in a LTS release. labels Jan 16, 2024
@richardlau
Copy link
Member

This is being reverted by #51491 so adding dont-land labels.

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. dont-land-on-v18.x PRs that should not land on the v18.x-staging branch and should not be released in v18.x. dont-land-on-v20.x PRs that should not land on the v20.x-staging branch and should not be released in v20.x. web streams
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: using structuredClone with ReadableStream prevents process from exiting
8 participants