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

Clean up orphan ReadableStream remaining during parsing response (#648) #649

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

HaidongPang
Copy link

No description provided.

@HaidongPang HaidongPang force-pushed the FixOrphanStream branch 2 times, most recently from 0dd3d9d to 96a6244 Compare November 7, 2024 08:56
@gregory
Copy link

gregory commented Nov 8, 2024

@HaidongPang i was just looking into something similar, it appears that the response is cloned before being passed to each afterResponse hooks which will create ReadableStream branches, which will force the runtime to buffer the entire body of the stream in memory multiple times without certainty that it'll ever be consumed...

@sindresorhus Could you elaborate on why you felt that you had to do that?

Thanks for your amazing work btw 🙏

@HaidongPang
Copy link
Author

@gregory

I suspect that @sindresorhus does not want the ReadableStream of response to be consumed in the afterResponse hooks, which would result in incomplete response data being returned.

In short, you can view the response object that is exactly the same as the original response in afterResponse hooks, but you should not modify the original response.

@gregory
Copy link

gregory commented Nov 8, 2024

@gregory

I suspect that @sindresorhus does not want the ReadableStream of response to be consumed in the afterResponse hooks, which would result in incomplete response data being returned.

In short, you can view the response object that is exactly the same as the original response in afterResponse hooks, but you should not modify the original response.

I understand, but that would be the responsibility of the consumer.
Ie: in the hook:

(req, {context}, res) => {
const headers = res.headers() 
const clone = res.clone()
}

@sholladay
Copy link
Collaborator

sholladay commented Nov 8, 2024

We could leave cloning to the consumer, but it's not very ergonomic and definitely a footgun of its own.

One possible solution might be to override the body methods of the response given to hooks, so they first clone the response before consuming the stream.

@gregory
Copy link

gregory commented Nov 8, 2024

We could leave cloning to the consumer, but it's not very ergonomic and definitely a footgun of its own.

One possible solution might be to override the body methods of the response given to hooks, so they first clone the response before consuming the stream.

that's a good idea.

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