Skip to content

stream: fix cloned webstreams not being unref correctly#51526

Merged
nodejs-github-bot merged 1 commit intonodejs:mainfrom
tsctx:fix/webstreams-clone-hang
Jan 23, 2024
Merged

stream: fix cloned webstreams not being unref correctly#51526
nodejs-github-bot merged 1 commit intonodejs:mainfrom
tsctx:fix/webstreams-clone-hang

Conversation

@tsctx
Copy link
Member

@tsctx tsctx commented Jan 19, 2024 •
edited
Loading

Fixes #44985.
Fixed a problem where the process would not terminate if structuredClone was used for webstream and body was not consumed.

The issue with #51255 has been fixed and adjusted so that #51486 does not happen.

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. web streams labels Jan 19, 2024
mcollina
mcollina approved these changes Jan 19, 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 requested review from KhafraDev and jasnell January 19, 2024 16:01
@RafaelGSS RafaelGSS mentioned this pull request Jan 19, 2024
Merged
@tsctx tsctx force-pushed the fix/webstreams-clone-hang branch from 014f1ea to 0530d58 Compare January 20, 2024 13:49
tsctx
tsctx commented Jan 20, 2024
View reviewed changes
@H4ad
Copy link
Member

H4ad commented Jan 22, 2024

@mcollina Should we fast track in favor of #51530 (comment)?

@H4ad H4ad added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Jan 22, 2024
@richardlau
Copy link
Member

richardlau commented Jan 22, 2024

@mcollina Should we fast track in favor of #51530 (comment)?

This doesn't need a fast track as it was opened >48 hours ago and has the required number of reviews. It does need a passing CI run, which I see has been requested.

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

nodejs-github-bot commented Jan 22, 2024

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

@H4ad
Copy link
Member

H4ad commented Jan 22, 2024

@nodejs/build The fedora machines are failling in the latest jobs:

11:40:02 ERROR: Error cloning remote repo 'origin'
11:40:02 hudson.plugins.git.GitException: Could not init /home/iojs/build/workspace/node-test-commit-linux
11:40:02 	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$5.execute(CliGitAPIImpl.java:1073)
11:40:02 	at org.jenkinsci.plugins.gitclient.CliGitAPIImpl$2.execute(CliGitAPIImpl.java:819)
11:40:02 	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$GitCommandMasterToSlaveCallable.call(RemoteGitImpl.java:170)
11:40:02 	at org.jenkinsci.plugins.gitclient.RemoteGitImpl$CommandInvocationHandler$GitCommandMasterToSlaveCallable.call(RemoteGitImpl.java:161)
11:40:02 	at hudson.remoting.UserRequest.perform(UserRequest.java:211)
11:40:02 	at hudson.remoting.UserRequest.perform(UserRequest.java:54)
11:40:02 	at hudson.remoting.Request$2.run(Request.java:377)
11:40:02 	at hudson.remoting.InterceptingExecutorService.lambda$wrap$0(InterceptingExecutorService.java:78)
11:40:02 	at java.base/java.util.concurrent.FutureTask.run(FutureTask.java:264)
11:40:02 	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136)
11:40:02 	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635)
11:40:02 	at hudson.remoting.Engine$1.lambda$newThread$0(Engine.java:125)
11:40:02 	at java.base/java.lang.Thread.run(Thread.java:833)
11:40:02 	Suppressed: hudson.remoting.Channel$CallSiteStackTrace: Remote call to JNLP4-connect connection from

@targos
Copy link
Member

targos commented Jan 22, 2024

Looks like the git clone is corrupt on test-digitalocean-fedora38-x64-2. I wiped out the workspace.

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 22, 2024

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

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 23, 2024

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

@mcollina mcollina added the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 23, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jan 23, 2024
@nodejs-github-bot nodejs-github-bot merged commit 66556f5 into nodejs:main Jan 23, 2024
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Jan 23, 2024

Landed in 66556f5

@tsctx tsctx deleted the fix/webstreams-clone-hang branch January 23, 2024 06:38
@GulajavaMinistudio GulajavaMinistudio mentioned this pull request Jan 23, 2024
Merged
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 2, 2024
@tsctx @marco-ippolito
stream: fix cloned webstreams not being unref correctly
0f63ee9 
PR-URL: nodejs#51526
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
targos pushed a commit that referenced this pull request Feb 15, 2024
@tsctx @targos
stream: fix cloned webstreams not being unref correctly
438b7fd 
PR-URL: #51526
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Feb 19, 2024
@tsctx @marco-ippolito
stream: fix cloned webstreams not being unref correctly
76ff37e 
PR-URL: nodejs#51526
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
@marco-ippolito marco-ippolito mentioned this pull request Mar 1, 2024
Merged
@tsctx tsctx mentioned this pull request Mar 2, 2024
Closed
@tsctx tsctx mentioned this pull request Mar 15, 2024
Closed
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@tsctx @richardlau
stream: fix cloned webstreams not being unref correctly
3e18eb1 
PR-URL: #51526
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@tsctx @richardlau
stream: fix cloned webstreams not being unref correctly
d0a6f35 
PR-URL: #51526
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
@AriPerkkio AriPerkkio mentioned this pull request Apr 15, 2024
Closed
@nflaig nflaig mentioned this pull request Apr 15, 2024
Closed
@AriPerkkio AriPerkkio mentioned this pull request May 4, 2024
Closed
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

@H4ad H4ad H4ad approved these changes

@jasnell jasnell Awaiting requested review from jasnell

@KhafraDev KhafraDev Awaiting requested review from KhafraDev

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. web streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bug: using structuredClone with ReadableStream prevents process from exiting

6 participants

@tsctx @H4ad @richardlau @nodejs-github-bot @targos @mcollina