-
-
Notifications
You must be signed in to change notification settings - Fork 194
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
Enable full duplex for http1 connections #692
Conversation
Might need to wait for Caddy v2.8.0 due to caddyserver/caddy#6102 Also keep in mind that some HTTP clients may not support duplex for HTTP/1.1 properly. I don't know the specifics of this, but that's the reason it's not enabled by default by Go. I haven't played around with this much at all, we just added the config flag to enable it for users to try out as a possible solution for those edgecases. |
I'm a bit skeptical of this; IIRC on the nginx implementation, it always reads the entire body (and always had -- hence the (edit to add: might just be wishful thinking as well. I currently have no evidence of this.) |
I would like to be in sync with Caddy regarding the defaults. |
I would be willing to enable it by default, but we need to make an evidence based decision. We don't know if it would break anything. Plus if it does break things it would probably do so quite obscurely; users would not think to try disabling duplex. Also I kinda rather follow the Go defaults rather than flip them, that becomes confusing. |
I agree. @withinboredom can we stick to Caddy and Go defaults please? |
Updated documentation about |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I added some tiny docs improvements.
Hmm. Would you be opposed to a PR that cancels builds if another commit is added to a PR? I used the GitHub ui to merge your suggestions which kicked off a bunch of builds. It'd probably be more efficient to only build the last commit and cancel the others. |
No, good idea! |
So, two notes:
Neither of these issues seems like a blocker to merge. If you agree @dunglas, give me a 👍 or merge away! |
I hope you'll get better soon and wish you a speedy recovery. Take your time, there is no urgency at all in the pending PR. Regarding this one, shouldn't we wait for the next release of Caddy to merge this? LGTM otherwise! Do you have more informations about the segfault you get? I can try to track it. |
Yeah, good call!
I've been trying to reproduce it. No luck so far. |
Looking at https://github.com/dunglas/frankenphp/actions/runs/8622010633/job/23632166418 and the stack trace a bit closer, it looks like the nil is in the test library itself, not our code. |
I merged support for Caddy 2.8 (#827), we should be able to proceed with this one! |
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
Co-authored-by: Kévin Dunglas <kevin@dunglas.fr>
8761624
to
47cd636
Compare
Thank you @withinboredom! Good stuff as usual!! |
❤️ |
There are two things here:
file_get_contents('php://input')
didn't even work), thus causing things like Server-Sent-Events not working through HTTP/1.1 #690 to be unable to work for http1 connections.kudos: @francislavoie
fixes: #690