Enable full duplex for http1 connections

[withinboredom]

There are two things here:

1. Per https://github.com/dunglas/frankenphp/pull/686#issuecomment-2018378525, we can let caddy handle the duplexity of http1. So the Caddyfile is updated to include this by default.

• [x] check the documentation for caddyfile references and update if necessary

2. An empty body was actually impossible to "read" (file_get_contents('php://input') didn't even work), thus causing things like #690 to be unable to work for http1 connections.

• [x] triple check for other cases like this

kudos: @francislavoie

fixes: #690

[francislavoie]

Might need to wait for Caddy v2.8.0 due to https://github.com/caddyserver/caddy/pull/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.

[withinboredom]

Also keep in mind that some HTTP clients may not support duplex for HTTP/1.1 properly.

I'm a bit skeptical of this; IIRC on the nginx implementation, it always reads the entire body (and always had -- hence the client_max_body_size directive). I'm not 100% sure as http1 is pretty ancient at this point, though still used. There may also be some magic via output buffers to provide an illusion of duplex in nginx... but anyway, I suspect the vast majority of HTTP clients (e.g., curl and friends) probably support it out-of-the-box. There may be custom implementations out there that don't support it, but they are probably an epic minority.

(edit to add: might just be wishful thinking as well. I currently have no evidence of this.)

[dunglas]

I would like to be in sync with Caddy regarding the defaults. As it looks like it's breaking a lot of Laravel apps, maybe Caddy should also consider enabling this feature by default anyway (cc @mholt @francislavoie). Otherwise, maybe should we just document how to enable this feature to fix the apps using SSE?

[francislavoie]

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.

[dunglas]

I agree. @withinboredom can we stick to Caddy and Go defaults please?

[withinboredom]

Updated documentation about enable_full_duplex and removed it from the default caddyfile.

[withinboredom]

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.

[dunglas]

No, good idea!

[withinboredom]

So, two notes:

1. I spotted a segfault on one of the test runs. It looks unrelated to this PR, but there's likely a nil pointer somewhere causing it. I'll take a look tomorrow, most likely. I'm still pretty sick but nearly recovered, though my throat is ragged af and I still have a lingering cough.
2. it looks like the static builder is (nearly) completely broken atm, not sure why. Would adding --debug to the builder also build it in debug mode or will it just allow us to see the errors? If it also builds it in debug mode, we should probably open an issue to just allow seeing the full build output. For CI purposes, hiding the build output is rather pointless.

Neither of these issues seems like a blocker to merge. If you agree @dunglas, give me a 👍 or merge away!

[dunglas]

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.

[withinboredom]

shouldn't we wait for the next release of Caddy to merge this?

Yeah, good call!

Do you have more informations about the segfault you get?

I've been trying to reproduce it. No luck so far.

[withinboredom]

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.

[dunglas]

I merged support for Caddy 2.8 (#827), we should be able to proceed with this one!

[dunglas]

Thank you @withinboredom! Good stuff as usual!!

[withinboredom]

❤️