Skip to content

Conversation

@cirospaciari
Copy link
Member

@cirospaciari cirospaciari commented Oct 7, 2025

What does this PR do?

This is the second step to fix cyclic reference issues with the stream please review #23313 first

How did you verify your code works?

Test + CI

@robobun
Copy link
Collaborator

robobun commented Oct 7, 2025

Updated 2:10 AM PT - Oct 7th, 2025

@cirospaciari, your commit d7f7ad8 has 1 failures in Build #28331 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 23319

That installs a local version of the PR into your bun-23319 executable, so you can run:

bun-23319 --bun

@cirospaciari cirospaciari force-pushed the ciro/fix-readable-stream-cycle-ref-step-2 branch from 11aea7a to b9daf01 Compare October 7, 2025 02:09
@cirospaciari cirospaciari changed the title WIP Response refactoring handling - change owner ship of the stream fix(Response) change owner ship of the stream to allow cyclic references Oct 7, 2025
@cirospaciari cirospaciari marked this pull request as ready for review October 7, 2025 02:55
stream.value.ensureStillAlive();
resp.detachReadableStream(globalThis);

stream.done(globalThis);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This subtly changes the semantics. now we only mark it as used if there is a ReadableStream attached. Previously, we unconditionally marked it as used. That is a bug.

### What does this PR do?
Just a refactoring isolating the usage of Request body this is the first
step necessary for #23327 please
review #23319 first
### How did you verify your code works?
CI
@Jarred-Sumner Jarred-Sumner merged commit d98eb1f into ciro/fix-readable-stream-cycle-ref Oct 7, 2025
4 of 6 checks passed
@Jarred-Sumner Jarred-Sumner deleted the ciro/fix-readable-stream-cycle-ref-step-2 branch October 7, 2025 09:10
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.

4 participants