Skip to content

Use a body stream for worker requests#268

Open
ostenbom wants to merge 1 commit intomentimeter:mainfrom
ostenbom:oliver/worker-request-streams
Open

Use a body stream for worker requests#268
ostenbom wants to merge 1 commit intomentimeter:mainfrom
ostenbom:oliver/worker-request-streams

Conversation

@ostenbom
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Contributor Author

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

@augustoccesar
Copy link
Copy Markdown
Member

@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 augustoccesar self-requested a review March 27, 2026 10:52
@augustoccesar
Copy link
Copy Markdown
Member

augustoccesar commented Mar 27, 2026

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
Copy link
Copy Markdown
Member

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 added a commit that referenced this pull request Mar 30, 2026
### Description
On #263 we changed the conversion of the request from the internal try_into from workers-rs for a
manual one to be able to proxy DELETE requests with body.
This caused the performance issues reported and with a proposed fix on #268.

The changes on #268 however re-introduced the original issue that #263 was attempting to fix, as in
it's core, it is what the `try_into` does.

As a middle ground, this PR proposes a workaround for the DELETE with body without compromising the
performance of other requests.
augustoccesar added a commit that referenced this pull request Mar 30, 2026
### Description
On #263 we changed the conversion of the request from the internal try_into from workers-rs for a
manual one to be able to proxy DELETE requests with body.
This caused the performance issues reported and with a proposed fix on #268.

The changes on #268 however re-introduced the original issue that #263 was attempting to fix, as in
it's core, it is what the `try_into` does.

As a middle ground, this PR proposes a workaround for the DELETE with body without compromising the
performance of other requests.
@augustoccesar
Copy link
Copy Markdown
Member

augustoccesar commented Mar 30, 2026

@ostenbom What do you think of #269?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants