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

core: Refactor, improve listener logic #5089

Merged
merged 3 commits into from
Sep 28, 2022
Merged

core: Refactor, improve listener logic #5089

merged 3 commits into from
Sep 28, 2022

Conversation

mholt
Copy link
Member

@mholt mholt commented Sep 27, 2022

Deprecate:

  • caddy.Listen
  • caddy.ListenTimeout
  • caddy.ListenPacket

Prefer caddy.NetworkAddress.Listen() instead.

Change:

  • caddy.ListenQUIC (hopefully to remove later)

  • caddy.ListenerFunc signature (add context and ListenConfig)

  • Don't emit Alt-Svc header advertising h3 over HTTP/3

  • Use quic.ListenEarly instead of quic.ListenEarlyAddr; this gives us more flexibility (e.g. possibility of HTTP/3 over UDS) but also introduces a new issue:
    Listening on Unix socket? quic-go/quic-go#3560 (comment)

  • Unlink unix socket before and after use

I'm not sure that ListenAll() will be useful. You'd think it would be, except the HTTP app, at least, sets up each listener one-by-one. That could probably be refactored and simplified with ListenAll(), but I haven't gotten there yet. EDIT: I haven't refactored the http app to use ListenAll, but I did find it to be quite convenient in the layer4 module so I think I'll at least keep it (experimentally) for now.

(I'm keeping it for now because I wrote it and then realized that wouldn't be suitable for this refactor. I kept it because I already wrote it. I might delete it before merging this PR though.)

This took several days. I've done fairly extensive testing on this, but also went through many phases of "oops, I broke it" eventually followed by "duh, that was stupid" or "argh, that's annoying" and found fixes or workarounds for all the problems I noticed.

I have NO idea if this works on Windows or Mac yet. I can only run tests on Linux here, so we'll see what the CI says. I did my best to at least ensure it builds on Windows.

This change was mostly motivated by HTTP/3 and Unix sockets and the interop there. HTTP/3 has to be disabled if a site binds to unix sockets and other HTTP versions are also enabled, because a unix socket can't be both STREAM and DGRAM. But if you enable only HTTP/3 and bind to a unix socket, it should work. However, I couldn't test this because no client I know of (Firefox, curl --http3, etc) supports HTTP/3 over unix sockets. And frankly, I don't blame them, because I can't think of why that'd ever be useful or necessary. Nonetheless, the code in this PR should enable that use case, and the program runs with a config like that, but I haven't verified with a client.

Overall the refactor is mostly a welcome change, although it does deprecate some Listen* functions in the caddy package. I doubt these are used by any plugins other than my layer4 app, so I'm not too worried about deprecating and eventually removing them.

Some things are frustrating about this though. Socket reuse is now dependent on OS and network type; so like, TCP sockets are reused differently on Windows and Linux, and different still for Unix sockets regardless of OS.

QUIC is a slight pain point still because although the listener is basically just a net.PacketConn, API constraints and an implementation choice in quic-go make the Close behavior of listeners a little tricky/surprising. Actually, quic-go's implementation kind of makes sense in general, and would seem like a good idea, but in our case it does add a bit of bulk to the code.

Ideally, we'd get rid of ListenQUIC and just have our serveHTTP() method wrap the returned listener in a quic.EarlyListener. And maybe there's a way to do that already, I just haven't discovered it yet. We might need to wrap the EarlyListener with our own concrete type, but it'd still likely be cleaner than what we have now.

This change also doesn't emit the Alt-Svc header on HTTP/3. It doesn't make sense to advertise h3 when the client is already using it. I just hope clients won't stop using it if the header disappears. I have noticed that Firefox likes to stop using HTTP/3 intermittently or randomly sometimes...

Anyway, there's a lot to unpack here. Overall I think this implementation of listeners is more reliable, flexible, and correct than what we had before, even if a little more complicated. I think some of the rough edges can be smoothed out with some cooperation and clever tinkering.

Please test this out. It'll probably get merged either way but the more field use it gets, the better! I'll deploy it to the Caddy website, for example, before tagging a release.

Deprecate:
- caddy.Listen
- caddy.ListenTimeout
- caddy.ListenPacket

Prefer caddy.NetworkAddress.Listen() instead.

Change:
- caddy.ListenQUIC (hopefully to remove later)
- caddy.ListenerFunc signature (add context and ListenConfig)

- Don't emit Alt-Svc header advertising h3 over HTTP/3

- Use quic.ListenEarly instead of quic.ListenEarlyAddr; this gives us
more flexibility (e.g. possibility of HTTP/3 over UDS) but also
introduces a new issue:
quic-go/quic-go#3560 (comment)

- Unlink unix socket before and after use
@mholt mholt added the under review 🧐 Review is pending before merging label Sep 27, 2022
@mholt mholt added this to the v2.6.2 milestone Sep 27, 2022
@mholt mholt removed the under review 🧐 Review is pending before merging label Sep 28, 2022
@mholt
Copy link
Member Author

mholt commented Sep 28, 2022

Merging this in so it can hopefully more easily get some field use before we deploy.

@mholt mholt merged commit e3e8aab into master Sep 28, 2022
@mholt mholt deleted the listeners branch September 28, 2022 19:35
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.

1 participant