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

[HaidongPang]

No description provided.

[gregory]

@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]

@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]

@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]

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]

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.

[HaidongPang]

@gregory
Will these two patches be considered for merging in future versions?
Related to :#650

[gregory]

@gregory Will these two patches be considered for merging in future versions? Related to :#650

I'm sorry I'm not sure how automatically generating an unused arraybuffer will solve the problem?
I thought @sholladay 's suggestion made sense.

[sholladay]

Closing in favor of #661.

After careful review, I determined that we don't need to clone the response at all in this particular case. 😀