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

fix: large requests with Caddy #187

Merged
merged 3 commits into from
Aug 16, 2023
Merged

fix: large requests with Caddy #187

merged 3 commits into from
Aug 16, 2023

Conversation

dunglas
Copy link
Owner

@dunglas dunglas commented Aug 16, 2023

This fix ensures that the request body buffer is full (if possible) before copying it to C memory.
Thanks to @francislavoie for pointing me to caddyserver/caddy#5461, which revealed the bug.

Close #182.

@dunglas dunglas marked this pull request as draft August 16, 2023 09:41
@dunglas dunglas marked this pull request as ready for review August 16, 2023 14:45
@dunglas dunglas changed the title test: large requests with Caddy fix: large requests with Caddy Aug 16, 2023
@francislavoie
Copy link
Contributor

This means you're buffering the whole request in memory? Or will it properly stream?

@dunglas
Copy link
Owner Author

dunglas commented Aug 16, 2023

No, it still streams (we have a test for that). We don't buffer the whole request but wait for the buffer provided by PHP to be full (which is a bit larger than the buffer you use in Caddy by default).

PHP considers that the request has been totally read if its buffer isn't full, which isn't the Go behavior (which is more fine-grained btw), so we have to wait for the PHP buffer to be full before calling PHP again.

@francislavoie
Copy link
Contributor

Oh I see, so you just had to pump r.Body.Read until it reaches countBytes (or EOF) which is PHP's buffer instead of only calling r.Body.Read once which would return too few bytes, and PHP will call go_read_post again if it wants more. I think I get it. 👍

@dunglas
Copy link
Owner Author

dunglas commented Aug 16, 2023

@francislavoie Yes exactly, Go's approach is better (you don't have to wait), but we have no choice here, PHP forces our hand.

@dunglas dunglas merged commit e0f2323 into main Aug 16, 2023
@dunglas dunglas deleted the test/182-caddy branch August 16, 2023 15:52
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.

Bug: Request data truncated > 3963 bytes
2 participants