Use a body stream for worker requests

[ostenbom]

Reading the entire request body to memory as a string is less efficient as being able to use stream primitives. This should increase performance of large request bodies for example file uploads.

[ostenbom]

@augustoccesar - noticed this when debugging some performance problems, WDYT?

[augustoccesar]

@augustoccesar - noticed this when debugging some performance problems, WDYT?

Yeah, this makes a lot of sense.
I was concerned about it when did the change for processing the body on #263, but thought maybe could live with this drawback, but guess not.

I will take a look if this re-introduces the issue that we were having with the body. If it does, I think we are probably better just reverting #263 to rely on the try_into of the Request instead of doing the manual body processing.

The issue was the "misunderstanding" if there was a body to process or not for non-standard requests. I'm not sure if the issue was with is_end_stream (which the try_into have internally as well) or if it was another check.
I will try this PR and come back soon!

[augustoccesar]

Yeah, this brings back the issue that we were trying to solve with #263 😞

I will investigate here if can get both things working!

[augustoccesar]

For context: the issue is that when doing DELETE request with a body, the body does not reach the destination if done in the way that is done in this PR or with the try_into that was done before #263 . And it seems to be related to having it as a stream. Somewhere it ignores/discard it.
But if read the body and build the JsValue from a string instead, it does arrive.

Granted, DELETE requests shouldn't really have bodies, but we do have some "non-conforming" endpoints at the moment.

Funny enough, it does not happen if running the worker locally, only once it is deployed to Cloudflare.
Might be worth to update the worker and see if that solves as well, since it has been a while since the workers-rs have been bumped because they did some breaking changes on how the output is structured.

[augustoccesar]

@ostenbom What do you think of #269?

[ostenbom]

Yeah @augustoccesar that's what I was going to suggest - even though it's non standard, my understanding is that it's not expressly prohibited? I would even leave it there, not even try to remove it if that works.

[augustoccesar]

Yeah @augustoccesar that's what I was going to suggest - even though it's non standard, my understanding is that it's not expressly prohibited? I would even leave it there, not even try to remove it if that works.

Yeah, it isn't prohibited, but seems discouraged and that some services might just reject the request.

Although request message framing is independent of the method used, content received in a DELETE request has no generally defined semantics, cannot alter the meaning or target of the request, and might lead some implementations to reject the request and close the connection because of its potential as a request smuggling attack (Section 11.2 of [HTTP/1.1]). A client SHOULD NOT generate content in a DELETE request unless it is made directly to an origin server that has previously indicated, in or out of band, that such a request has a purpose and will be adequately supported. An origin server SHOULD NOT rely on private agreements to receive content, since participants in HTTP communication are often unaware of intermediaries along the request chain.
-- https://www.rfc-editor.org/rfc/rfc9110.html#section-9.3.5-6

I think we will go with reverting it (#270). For our internal case, it got fixed (changed the endpoint) today, so there isn't really the need for the workaround from our perspective.

This would go back to letting the conversion be handled by the workers-rs. Then if is worth bringing up that this should be valid, maybe we open an issue there.

[augustoccesar]

The changes where we started to read the entire body has now been reverted 💪
#270