Advertise multiple listeners via Alt-Svc and improve perf of SetQuicHeaders

[renbou]

PR to resolve #3350.
Looking at the profile of Caddy with this commit used, SetQuicHeaders seems to work 5 times faster now, and pretty much consists of a single slice append.

[codecov]

Codecov Report

Merging #3352 (3a5c660) into master (b7e93b5) will increase coverage by 0.04%.
The diff coverage is 95.45%.

@@            Coverage Diff             @@
##           master    #3352      +/-   ##
==========================================
+ Coverage   85.52%   85.57%   +0.04%     
==========================================
  Files         135      135              
  Lines        9837     9860      +23     
==========================================
+ Hits         8413     8437      +24     
  Misses       1046     1046              
+ Partials      378      377       -1     

Impacted Files	Coverage Δ
http3/server.go	73.24% <95.45%> (+3.06%)	⬆️
internal/utils/rand.go	62.50% <0.00%> (-12.50%)	⬇️
http3/frames.go	78.08% <0.00%> (-0.30%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7e93b5...3a5c660. Read the comment docs.

[marten-seemann]

There is a valid use case for using a listener running on top of a net.PacketConn that is not a net.UDPConn, and which might not have a port to begin with. I'm wondering if we should just log the error, and skip addresses for which we can't determine the port. We could then extract the port on addListener.
wdyt?

[marten-seemann]

I'm aware this wasn't introduced by this PR, but is this correct? Can't we just do a strconv.Atoi here?

[renbou]

Well, there's technically a test which supplies ":https" as the port, and it is valid to run http3.Server.ListenAndServe with an address like ":https" given (which should launch a udp listener on 0.0.0.0:443). I don't really find it meaningful to support this and would rather have the function simply split by ':' and then do an Atoi, so that SetQuicHeaders doesn't return an error. But since it's already supported and SetQuicHeaders returns an error, doesn't make sense to change the API now 😕

[renbou]

There is a valid use case for using a listener running on top of a net.PacketConn that is not a net.UDPConn, and which might not have a port to begin with. I'm wondering if we should just log the error, and skip addresses for which we can't determine the port. We could then extract the port on addListener. wdyt?

What do you mean by "extract the port on addListener"? We're already extracting the port during addListener. Hmm, I could technically see someone using the http3 server on a unix socket, I think in the case where an address doesn't have a port, we can use Server.Addr (and if Server.Port then that would be used instead by default). And if Server.Addr isn't set then we should probably log the error and return it during SetQuicHeaders, yeah. I don't think advertising 443 by default is a good idea since 443 only really makes sense in terms of http1.1/2 over tls, but http3 can run on any port.

[marten-seemann]

I wasn't suggesting to use 443 by default. Sorry for the confusion.

Let me rephrase: We could get rid of the altSvcGenerationErr. When an error occurs in generateAltSvcHeader, we just log that error (even better: we only log it once, by running the port extraction logic in addListener), and skip that listener. That would make it possible to use (some) listeners that don't have port numbers, and still use SetQUICHeaders.

[renbou]

Ah, okay, this makes sense now. Though I think in this case, if no other ports are found from the listeners, I think we should try using Server.Addr as a last resort, and if it fails, then we could set an error and return it in SetQUICHeaders. I don't think there's a way to make SetQUICHeaders work nicely without returning an error at all, and that would also change existing API. Though now that I think about it, since in this case we would only return an error when no port is available for us to return valid headers with in SetQUICHeaders, we can still remove altSvcGenerationErr and instead return an error specifying that no ports are available (when altSvcHeader == "", for example). I'll implement this so it's easier to understand what I mean

[marten-seemann]

Thank you!