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 reading post bodies #686

Merged
merged 1 commit into from
Mar 24, 2024
Merged

fix reading post bodies #686

merged 1 commit into from
Mar 24, 2024

Conversation

withinboredom
Copy link
Collaborator

Now that golang/go#15527 is supposedly fixed, this condition should be no-longer needed. Further, if php didn't request enough bytes, this condition would be hit. It appears PHP requests chunks ~2mb in size at a time.

Fixes #122

Now that golang/go#15527 is supposedly fixed, this condition should be no-longer needed. Further, if php didn't request enough bytes, this condition would be hit. It appears PHP requests chunks ~2mb in size at a time.
@dunglas
Copy link
Owner

dunglas commented Mar 24, 2024

Would it be possible to add a test?

@withinboredom
Copy link
Collaborator Author

withinboredom commented Mar 24, 2024

Tested with the reproducer and 70mb file:

Laravel.and.17.more.pages.-.Personal.-.Microsoft.Edge.2024-03-24.11-20-19.mp4

@withinboredom
Copy link
Collaborator Author

Would it be possible to add a test?

Yes... but do we want to? That would mean putting large test file in the repo.

@withinboredom withinboredom merged commit d973206 into main Mar 24, 2024
41 checks passed
@withinboredom withinboredom deleted the fix/post-read branch March 24, 2024 11:18
@dunglas
Copy link
Owner

dunglas commented Mar 24, 2024

@withinboredom not necessarily, we can tweak this test for instance: https://github.com/dunglas/frankenphp/blob/main/testdata/large-request.php

@withinboredom
Copy link
Collaborator Author

on it!

@dunglas
Copy link
Owner

dunglas commented Mar 24, 2024

Thank you

withinboredom added a commit that referenced this pull request Mar 24, 2024
This makes the file size 6mb-ish, more than the 2mb batching that PHP does. I verified this fails on 0e163a0 (main prior to #686).
@francislavoie
Copy link
Contributor

Re golang/go#15527, that's an opt-in fix, EnableDuplex() needs to be called to fix it. We added support for that in Caddy recently https://caddyserver.com/docs/caddyfile/options#enable-full-duplex. But I mean, if you've tested it then 👍 I suppose!

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.

None yet

3 participants