Skip to content

Conversation

snyamathi
Copy link
Contributor

@snyamathi snyamathi commented Aug 15, 2025

This relates to...

Rationale

The request/stream registered with the finalization registry is mixed up between the clone and the original request.

Changes

This PR just changes which of the two tee'd streams is associated

edit: updated with changes from @tsctx how we're fixing the stream registration is moved to a better location #4419

Features

N/A

Bug Fixes

See changes above

Breaking Changes and Deprecations

N/A

Status

@snyamathi snyamathi changed the title Patch 1 fix: wrong stream canceled up after cloning Aug 15, 2025
@Uzlopak Uzlopak requested a review from Copilot August 15, 2025 20:48
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Fixes a bug where the wrong stream was being registered with the finalization registry when cloning Response objects. This was causing premature cancellation of the original response's stream when the cloned response was garbage collected.

  • Changed the stream registration from out1 to out2 in the clone body logic
  • Added a test to verify that garbage collection of a clone doesn't affect the original response

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
lib/web/fetch/body.js Fixed stream registry to register the correct stream (out2 instead of out1) for the cloned instance
test/fetch/response.js Added test to verify that garbage collection of cloned response doesn't break the original response

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.

@snyamathi snyamathi changed the title fix: wrong stream canceled up after cloning fix: fix wrong stream canceled up after cloning (v6) Aug 16, 2025
Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

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

RSLGTM

@Uzlopak
Copy link
Contributor

Uzlopak commented Aug 18, 2025

Merging as the failed CI is unrelated.

@Uzlopak Uzlopak merged commit 44c23e5 into nodejs:v6.x Aug 18, 2025
27 of 28 checks passed
@snyamathi snyamathi deleted the patch-1 branch August 18, 2025 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants