caddyhttp: Make use of http.ResponseController

[francislavoie]

One part of #5288, should fix #5431 (if built with Go 1.21+).

Adds a new enable_full_duplex server global option to opt-in (only effectual for Go 1.21+, with a go:build constraint). I think it needs to be opt-in because it could cause deadlocks if the client doesn't know how to handle a connection in that mode, but it seems necessary to turn on for certain kinds of apps. Maybe this should be an HTTP handler (maybe as a new option for https://caddyserver.com/docs/caddyfile/directives/request_body) which would allow using matchers to conditionally enable it instead of enabling it for all connections to the server 🤔 But I marked this as experimental so we can adjust later if needed.

The other main aspect of this is dropping caddyhttp.HTTPInterfaces because it's now made redundant by http.ResponseController which gives a way to use these optional interfaces without type assertions. Simplifies code in most cases.

Also syncs the reverseproxy implementation with stdlib's which now uses ResponseController as well; see golang/go@2449bbb for the original diff that I ported over.

And finally, minor improvement, io.Copy() already tries io.ReaderFrom so we don't need to do that ourselves in responsewriter.go.

[francislavoie]

Linter doesn't like this new pattern. Opened an issue: timakin/bodyclose#52

Also the Go stdlib code we ported ignores the err returned by m.flush(), so I need to suppress it to keep the code the same 🤷‍♂️

[mholt]

Thanks, this definitely looks more elegant than what we were doing before.

I imagine there aren't external repos using our HTTPInterfaces type?

[mholt]

LGTM now. Thank you for contributing this!

[francislavoie]

I'd just like @WeidiDeng to give his 👍 on this as well in case I missed something.

[WeidiDeng]

I'd just like @WeidiDeng to give his 👍 on this as well in case I missed something.

There is something about encode that I don't like, can it be type asserted just like before to avoid wrapping a caddyhttp.ResponseWriterWrapper?

[francislavoie]

Hmm. I'm not sure what we would type assert at all. We already know it's a ResponseWriter. Do we even need wrapping now that response controller lets us call those optional interfaces in a unified way? Does our wrapper add any value, or is it necessary for it to work properly?

If you have a suggestion, feel free to commit it 🤔 I'm not sure I follow.

[WeidiDeng]

I mean like this:

if rww, ok := wrappedRW.(*caddyhttp.ResponseWriterWrapper); ok {
  rw.ResponseWriter = rww
} else {
  rw.ResponseWriter = &caddyhttp.ResponseWriterWrapper{ResponseWriter: wrappedRW}
}

[francislavoie]

Aaah I see, so don't re-wrap if it's already wrapped. Makes sense.

[francislavoie]

Thanks, pushed that fix. Do you notice anything else?

[mholt]

I'll probably merge this in a few minutes to a few hours (before I have to take off for some family stuff today) so I can tag 2.7. Thank you for working on this!

[mholt]

And @WeidiDeng thank you for your invaluable review and contributions 🙏