Revert 51255#51491

mcollina · 2024-01-16T16:01:48Z

Reverts #51255
Fixes#51486

Adds a regression test.

This reverts commit 4d3923a.

Signed-off-by: Matteo Collina <[email protected]>

nodejs-github-bot · 2024-01-16T16:07:56Z

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

mcollina · 2024-01-16T17:15:35Z

Can we fast-track this?

github-actions · 2024-01-16T17:15:52Z

Fast-track has been requested by @mcollina. Please 👍 to approve.

nodejs-github-bot · 2024-01-17T09:40:21Z

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

RafaelGSS · 2024-01-18T13:26:23Z

@nodejs/tsc @nodejs/build I was planning to create a proposal yesterday with this revert but apparently some of our machines are suffering. Is that ok to force land it? I'm planning to create the proposal asap and release it tomorrow.

Considering it's a clean revert + a test case it wouldn't be a problem I assume.

cc: @nodejs/releasers

aduh95 · 2024-01-18T14:02:53Z

apparently some of our machines are suffering

The issue seems to be the backlog for the osx CI is growing faster than the CI is eating it. Would it be possible to disable that job until the queue is gone instead?

nodejs-github-bot · 2024-01-18T14:03:31Z

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

BethGriggs · 2024-01-18T15:01:00Z

The issue seems to be the backlog for the osx CI is growing faster than the CI is eating it. Would it be possible to disable that job until the queue is gone instead?

All the osx11-x64 machines are offline https://ci.nodejs.org/label/osx11-x64/

nodejs-github-bot · 2024-01-18T15:53:59Z

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

mhdawson · 2024-01-18T21:44:55Z

Just a reminder that in a case like this opening an issue in the build repo may make it more visible that help is needed, I went ahead and did that here - nodejs/build#3612

nodejs-github-bot · 2024-01-19T09:54:31Z

Commit Queue failed

- Loading data for nodejs/node/pull/51491 ✔ Done loading data for nodejs/node/pull/51491 ----------------------------------- PR info ------------------------------------ Title Revert 51255 (#51491) Author Matteo Collina (@mcollina) Branch mcollina:revert-51255 -> nodejs:main Labels fast-track, commit-queue-rebase Commits 2 - Revert "stream: fix cloned webstreams not being unref'd" - test: add regression test for 51586 Committers 1 - Matteo Collina PR-URL: https://github.com/nodejs/node/pull/51491 Reviewed-By: Vinícius Lourenço Claro Cardoso Reviewed-By: Marco Ippolito Reviewed-By: Matthew Aitken Reviewed-By: Rafael Gonzaga Reviewed-By: Moshe Atlow Reviewed-By: Franziska Hinkelmann Reviewed-By: Benjamin Gruenbaum ------------------------------ Generated metadata ------------------------------ PR-URL: https://github.com/nodejs/node/pull/51491 Reviewed-By: Vinícius Lourenço Claro Cardoso Reviewed-By: Marco Ippolito Reviewed-By: Matthew Aitken Reviewed-By: Rafael Gonzaga Reviewed-By: Moshe Atlow Reviewed-By: Franziska Hinkelmann Reviewed-By: Benjamin Gruenbaum -------------------------------------------------------------------------------- ℹ This PR was created on Tue, 16 Jan 2024 16:01:48 GMT ✔ Approvals: 7 ✔ - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/51491#pullrequestreview-1824043243 ✔ - Marco Ippolito (@marco-ippolito): https://github.com/nodejs/node/pull/51491#pullrequestreview-1824108380 ✔ - Matthew Aitken (@KhafraDev): https://github.com/nodejs/node/pull/51491#pullrequestreview-1824177345 ✔ - Rafael Gonzaga (@RafaelGSS) (TSC): https://github.com/nodejs/node/pull/51491#pullrequestreview-1824440897 ✔ - Moshe Atlow (@MoLow) (TSC): https://github.com/nodejs/node/pull/51491#pullrequestreview-1824680016 ✔ - Franziska Hinkelmann (@fhinkel): https://github.com/nodejs/node/pull/51491#pullrequestreview-1826347736 ✔ - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/51491#pullrequestreview-1830226110 ℹ This PR is being fast-tracked ✘ Last GitHub CI failed ℹ Last Full PR CI on 2024-01-18T15:53:59Z: https://ci.nodejs.org/job/node-test-pull-request/56836/ - Querying data for job/node-test-pull-request/56836/ ✔ Last Jenkins CI successful -------------------------------------------------------------------------------- ✔ Aborted `git node land` session in /home/runner/work/node/node/.ncu

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

nodejs-github-bot · 2024-01-19T10:39:32Z

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

nodejs-github-bot · 2024-01-19T11:47:08Z

Landed in a772973...fbf1fb3

This reverts commit 4d3923a. PR-URL: #51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Signed-off-by: Matteo Collina <[email protected]> PR-URL: #51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

This reverts commit 4d3923a. PR-URL: #51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Signed-off-by: Matteo Collina <[email protected]> PR-URL: #51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

This reverts commit 4d3923a. PR-URL: nodejs#51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

lib/internal/webstreams/readablestream.js


 [kTransferList](){
 const{ port1, port2 }=newMessageChannel();
-port1.unref();


This reverts commit 4d3923a. PR-URL: nodejs#51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

This reverts commit 4d3923a. PR-URL: nodejs#51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Signed-off-by: Matteo Collina <[email protected]> PR-URL: nodejs#51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

Signed-off-by: Matteo Collina <[email protected]> PR-URL: #51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

This reverts commit 4d3923a. PR-URL: #51491 Reviewed-By: Vinícius Lourenço Claro Cardoso <[email protected]> Reviewed-By: Marco Ippolito <[email protected]> Reviewed-By: Matthew Aitken <[email protected]> Reviewed-By: Rafael Gonzaga <[email protected]> Reviewed-By: Moshe Atlow <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>

mcollina added 2 commits January 16, 2024 16:45

Revert "stream: fix cloned webstreams not being unref'd"
d286f1e
This reverts commit 4d3923a.

test: add regression test for 51586
6a38f83
Signed-off-by: Matteo Collina <[email protected]>

mcollina requested review from RafaelGSS and jasnell January 16, 2024 16:01

nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jan 16, 2024

mcollina added commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. request-ci Add this label to start a Jenkins CI on a PR. and removed needs-ci PRs that need a full CI run. web streams labels Jan 16, 2024

github-actionsbot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 16, 2024

H4ad approved these changes Jan 16, 2024
View reviewed changes

richardlau mentioned this pull request Jan 16, 2024
streams: fix cloned webstreams not being unref'd #51255
Merged

marco-ippolito approved these changes Jan 16, 2024
View reviewed changes

KhafraDev approved these changes Jan 16, 2024
View reviewed changes

mcollina added the fast-track PRs that do not need to wait for 48 hours to land. label Jan 16, 2024

RafaelGSS approved these changes Jan 16, 2024
View reviewed changes

mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 16, 2024

MoLow approved these changes Jan 16, 2024
View reviewed changes

fhinkel approved these changes Jan 17, 2024
View reviewed changes

benjamingr approved these changes Jan 18, 2024
View reviewed changes

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2024

nodejs-github-bot added the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Jan 19, 2024

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 Jan 19, 2024

mcollina mentioned this pull request Jan 19, 2024
bug: using structuredClone with ReadableStream prevents process from exiting #44985
Closed

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 19, 2024

nodejs-github-bot closed this Jan 19, 2024

mcollina deleted the revert-51255 branch January 19, 2024 13:29

RafaelGSS mentioned this pull request Jan 19, 2024
v21.6.1 proposal #51530
Merged

Trott mentioned this pull request Jan 22, 2024
Regression in Node.js v21.6.0 #51486
Closed

Taahoyare reviewed Jan 31, 2024
View reviewed changes

lib/internal/webstreams/readablestream.js

[kTransferList](){
const{ port1, port2 }=newMessageChannel();
port1.unref();
This comment was marked as spam.
Sign in to view
This comment was marked as spam.
Sign in to view

richardlau mentioned this pull request Mar 25, 2024
v20.12.0 proposal #52212
Merged

marco-ippolito mentioned this pull request May 2, 2024
v20.13.0 proposal #52793
Merged

Uh oh!

Revert 51255#51491

Revert 51255 #51491

Uh oh!

Conversation

mcollina commented Jan 16, 2024

Uh oh!

nodejs-github-bot commented Jan 16, 2024

Uh oh!

mcollina commented Jan 16, 2024

Uh oh!

github-actionsbot commented Jan 16, 2024

Uh oh!

nodejs-github-bot commented Jan 17, 2024

Uh oh!

RafaelGSS commented Jan 18, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aduh95 commented Jan 18, 2024

Uh oh!

nodejs-github-bot commented Jan 18, 2024

Uh oh!

BethGriggs commented Jan 18, 2024

Uh oh!

nodejs-github-bot commented Jan 18, 2024

Uh oh!

mhdawson commented Jan 18, 2024

Uh oh!

nodejs-github-bot commented Jan 19, 2024

Uh oh!

nodejs-github-bot commented Jan 19, 2024

Uh oh!

nodejs-github-bot commented Jan 19, 2024

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

13 participants

RafaelGSS commented Jan 18, 2024•
edited
Loading