Fix http3 servers dying after reload (#4654)

go.mod

@@ -16,7 +16,7 @@ require (
 	github.com/google/uuid v1.3.0
 	github.com/klauspost/compress v1.15.0
 	github.com/klauspost/cpuid/v2 v2.0.11
-	github.com/lucas-clemente/quic-go v0.25.0
+	github.com/lucas-clemente/quic-go v0.26.0
 	github.com/mholt/acmez v1.0.2
 	github.com/naoina/go-stringutil v0.1.0 // indirect
 	github.com/naoina/toml v0.1.1

go.sum

@@ -664,8 +664,8 @@ github.com/libdns/libdns v0.2.1 h1:Wu59T7wSHRgtA0cfxC+n1c/e+O3upJGWytknkmFEDis=
 github.com/libdns/libdns v0.2.1/go.mod h1:yQCXzk1lEZmmCPa857bnk4TsOiqYasqpyOEeSObbb40=
 github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20190605223551-bc2310a04743/go.mod h1:qklhhLq1aX+mtWk9cPHPzaBjWImj5ULL6C7HFJtXQMM=
 github.com/lightstep/lightstep-tracer-go v0.18.1/go.mod h1:jlF1pusYV4pidLvZ+XD0UBX0ZE6WURAspgAczcDHrL4=
-github.com/lucas-clemente/quic-go v0.25.0 h1:K+X9Gvd7JXsOHtU0N2icZ2Nw3rx82uBej3mP4CLgibc=
+github.com/lucas-clemente/quic-go v0.26.0 h1:ALBQXr9UJ8A1LyzvceX4jd9QFsHvlI0RR6BkV16o00A=
-github.com/lucas-clemente/quic-go v0.25.0/go.mod h1:YtzP8bxRVCBlO77yRanE264+fY/T2U9ZlW1AaHOsMOg=
+github.com/lucas-clemente/quic-go v0.26.0/go.mod h1:AzgQoPda7N+3IqMMMkywBKggIFo2KT6pfnlrQ2QieeI=
 github.com/lunixbochs/vtclean v1.0.0/go.mod h1:pHhQNgMf3btfWnGBVipUOjRYhoOsdGqdm/+2c2E2WMI=
 github.com/lyft/protoc-gen-validate v0.0.13/go.mod h1:XbGvPuh87YZc5TdIa2/I4pLk0QoUACkjt2znoq26NVQ=
 github.com/magiconair/properties v1.8.0/go.mod h1:PppfXfuXeibc/6YijjN8zIbojt8czPbwD3XqdrwzmxQ=
@@ -675,13 +675,12 @@ github.com/manifoldco/promptui v0.9.0 h1:3V4HzJk1TtXW1MTZMP7mdlwbBpIinw3HztaIlYt
 github.com/manifoldco/promptui v0.9.0/go.mod h1:ka04sppxSGFAtxX0qhlYQjISsg9mR4GWtQEhdbn6Pgg=
 github.com/marten-seemann/qpack v0.2.1 h1:jvTsT/HpCn2UZJdP+UUB53FfUUgeOyG5K1ns0OJOGVs=
 github.com/marten-seemann/qpack v0.2.1/go.mod h1:F7Gl5L1jIgN1D11ucXefiuJS9UMVP2opoCp2jDKb7wc=
-github.com/marten-seemann/qtls-go1-15 v0.1.4/go.mod h1:GyFwywLKkRt+6mfU99csTEY1joMZz5vmB1WNZH3P81I=
+github.com/marten-seemann/qtls-go1-16 v0.1.5 h1:o9JrYPPco/Nukd/HpOHMHZoBDXQqoNtUCmny98/1uqQ=
-github.com/marten-seemann/qtls-go1-16 v0.1.4 h1:xbHbOGGhrenVtII6Co8akhLEdrawwB2iHl5yhJRpnco=
+github.com/marten-seemann/qtls-go1-16 v0.1.5/go.mod h1:gNpI2Ol+lRS3WwSOtIUUtRwZEQMXjYK+dQSBFbethAk=
-github.com/marten-seemann/qtls-go1-16 v0.1.4/go.mod h1:gNpI2Ol+lRS3WwSOtIUUtRwZEQMXjYK+dQSBFbethAk=
+github.com/marten-seemann/qtls-go1-17 v0.1.1 h1:DQjHPq+aOzUeh9/lixAGunn6rIOQyWChPSI4+hgW7jc=
-github.com/marten-seemann/qtls-go1-17 v0.1.0 h1:P9ggrs5xtwiqXv/FHNwntmuLMNq3KaSIG93AtAZ48xk=
+github.com/marten-seemann/qtls-go1-17 v0.1.1/go.mod h1:C2ekUKcDdz9SDWxec1N/MvcXBpaX9l3Nx67XaR84L5s=
-github.com/marten-seemann/qtls-go1-17 v0.1.0/go.mod h1:fz4HIxByo+LlWcreM4CZOYNuz3taBQ8rN2X6FqvaWo8=
+github.com/marten-seemann/qtls-go1-18 v0.1.1 h1:qp7p7XXUFL7fpBvSS1sWD+uSqPvzNQK43DH+/qEkj0Y=
-github.com/marten-seemann/qtls-go1-18 v0.1.0-beta.1 h1:EnzzN9fPUkUck/1CuY1FlzBaIYMoiBsdwTNmNGkwUUM=
+github.com/marten-seemann/qtls-go1-18 v0.1.1/go.mod h1:mJttiymBAByA49mhlNZZGrH5u1uXYZJ+RW28Py7f4m4=
-github.com/marten-seemann/qtls-go1-18 v0.1.0-beta.1/go.mod h1:PUhIQk19LoFt2174H4+an8TYvWOGjb/hHwphBeaDHwI=
 github.com/mattn/go-colorable v0.0.9/go.mod h1:9vuHe8Xs5qXnSaW/c/ABM9alt+Vo+STaOChaDxuIBZU=
 github.com/mattn/go-colorable v0.1.1/go.mod h1:FuOcm+DKB9mbwrcAfNl7/TZVBZ6rcnceauSikq3lYCQ=
 github.com/mattn/go-colorable v0.1.2/go.mod h1:U0ppj6V5qS13XJ6of8GYAs25YV2eR4EVcfRqFIhoBtE=

listeners.go

@@ -15,6 +15,9 @@
 package caddy
 
 import (
+	"context"
+	"crypto/tls"
+	"errors"
 	"fmt"
 	"net"
 	"os"
@@ -24,6 +27,9 @@ import (
 	"sync/atomic"
 	"syscall"
 	"time"
+
+	"github.com/lucas-clemente/quic-go"
+	"github.com/lucas-clemente/quic-go/http3"
 )
 
 // Listen is like net.Listen, except Caddy's listeners can overlap
@@ -79,6 +85,27 @@ func ListenPacket(network, addr string) (net.PacketConn, error) {
 	return &fakeClosePacketConn{sharedPacketConn: sharedPc.(*sharedPacketConn)}, nil
 }
 
+// ListenQUIC returns a quic.EarlyListener suitable for use in a Caddy module.
+// Note that the context passed to Accept is currently ignored, so using
+// a context other than context.Background is meaningless.
+func ListenQUIC(addr string, tlsConf *tls.Config) (quic.EarlyListener, error) {
+	lnKey := "quic/" + addr
+
+	sharedEl, _, err := listenerPool.LoadOrNew(lnKey, func() (Destructor, error) {
+		el, err := quic.ListenAddrEarly(addr, http3.ConfigureTLSConfig(tlsConf), &quic.Config{})
+		if err != nil {
+			return nil, err
+		}
+		return &sharedQuicListener{EarlyListener: el, key: lnKey}, nil
+	})
+
+	ctx, cancel := context.WithCancel(context.Background())
+	return &fakeCloseQuicListener{
+		sharedQuicListener: sharedEl.(*sharedQuicListener),
+		context:            ctx, contextCancel: cancel,
+	}, err
+}
+
 // fakeCloseListener is a private wrapper over a listener that
 // is shared. The state of fakeCloseListener is not shared.
 // This allows one user of a socket to "close" the listener
@@ -95,7 +122,7 @@ type fakeCloseListener struct {
 func (fcl *fakeCloseListener) Accept() (net.Conn, error) {
 	// if the listener is already "closed", return error
 	if atomic.LoadInt32(&fcl.closed) == 1 {
-		return nil, fcl.fakeClosedErr()
+		return nil, fakeClosedErr(fcl)
 	}
 
 	// call underlying accept
@@ -119,7 +146,7 @@ func (fcl *fakeCloseListener) Accept() (net.Conn, error) {
 		_ = fcl.sharedListener.clearDeadline()
 
 		if netErr, ok := err.(net.Error); ok && netErr.Timeout() {
-			return nil, fcl.fakeClosedErr()
+			return nil, fakeClosedErr(fcl)
 		}
 	}
 
@@ -145,14 +172,47 @@ func (fcl *fakeCloseListener) Close() error {
 	return nil
 }
 
+type fakeCloseQuicListener struct {
+	closed              int32 // accessed atomically; belongs to this struct only
+	*sharedQuicListener       // embedded, so we also become a quic.EarlyListener
+	context             context.Context
+	contextCancel       context.CancelFunc
+}
+
+// Currently Accept ignores the passed context, however a situation where
+// someone would need a hotswappable QUIC-only (not http3, since it uses context.Background here)
+// server on which Accept would be called with non-empty contexts
+// (mind that the default net listeners' Accept doesn't take a context argument)
+// sounds way too rare for us to sacrifice efficiency here.
+func (fcql *fakeCloseQuicListener) Accept(_ context.Context) (quic.EarlySession, error) {
+	conn, err := fcql.sharedQuicListener.Accept(fcql.context)
+	if err == nil {
+		return conn, nil
+	}
+
+	// if the listener is "closed", return a fake closed error instead
+	if atomic.LoadInt32(&fcql.closed) == 1 && errors.Is(err, context.Canceled) {
+		return nil, fakeClosedErr(fcql)
+	}
+	return nil, err
+}
+
+func (fcql *fakeCloseQuicListener) Close() error {
+	if atomic.CompareAndSwapInt32(&fcql.closed, 0, 1) {
+		fcql.contextCancel()
+		_, _ = listenerPool.Delete(fcql.sharedQuicListener.key)
+	}
+	return nil
+}
+
 // fakeClosedErr returns an error value that is not temporary
 // nor a timeout, suitable for making the caller think the
 // listener is actually closed
-func (fcl *fakeCloseListener) fakeClosedErr() error {
+func fakeClosedErr(l interface{ Addr() net.Addr }) error {
 	return &net.OpError{
 		Op:   "accept",
-		Net:  fcl.Addr().Network(),
+		Net:  l.Addr().Network(),
-		Addr: fcl.Addr(),
+		Addr: l.Addr(),
 		Err:  errFakeClosed,
 	}
 }
@@ -244,6 +304,17 @@ func (sl *sharedListener) Destruct() error {
 	return sl.Listener.Close()
 }
 
+// sharedQuicListener is like sharedListener, but for quic.EarlyListeners.
+type sharedQuicListener struct {
+	quic.EarlyListener
+	key string
+}
+
+// Destruct closes the underlying QUIC listener.
+func (sql *sharedQuicListener) Destruct() error {
+	return sql.EarlyListener.Close()
+}
+
 // sharedPacketConn is like sharedListener, but for net.PacketConns.
 type sharedPacketConn struct {
 	net.PacketConn

modules/caddyhttp/app.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"crypto/tls"
 	"fmt"
-	"net"
 	"net/http"
 	"strconv"
 	"time"
@@ -116,9 +115,8 @@ type App struct {
 	// affect functionality.
 	Servers map[string]*Server `json:"servers,omitempty"`
 
-	servers     []*http.Server
+	servers   []*http.Server
-	h3servers   []*http3.Server
+	h3servers []*http3.Server
-	h3listeners []net.PacketConn
 
 	ctx    caddy.Context
 	logger *zap.Logger
@@ -353,9 +351,9 @@ func (app *App) Start() error {
 						app.logger.Info("enabling experimental HTTP/3 listener",
 							zap.String("addr", hostport),
 						)
-						h3ln, err := caddy.ListenPacket("udp", hostport)
+						h3ln, err := caddy.ListenQUIC(hostport, tlsCfg)
 						if err != nil {
-							return fmt.Errorf("getting HTTP/3 UDP listener: %v", err)
+							return fmt.Errorf("getting HTTP/3 QUIC listener: %v", err)
 						}
 						h3srv := &http3.Server{
 							Server: &http.Server{
@@ -366,9 +364,8 @@ func (app *App) Start() error {
 							},
 						}
 						//nolint:errcheck
-						go h3srv.Serve(h3ln)
+						go h3srv.ServeListener(h3ln)
 						app.h3servers = append(app.h3servers, h3srv)
-						app.h3listeners = append(app.h3listeners, h3ln)
 						srv.h3server = h3srv
 					}
 					/////////
@@ -426,13 +423,6 @@ func (app *App) Stop() error {
 		}
 	}
 
-	// close the http3 servers; it's unclear whether the bug reported in
-	// https://github.com/caddyserver/caddy/pull/2727#issuecomment-526856566
-	// was ever truly fixed, since it seemed racey/nondeterministic; but
-	// recent tests in 2020 were unable to replicate the issue again after
-	// repeated attempts (the bug manifested after a config reload; i.e.
-	// reusing a http3 server or listener was problematic), but it seems
-	// to be working fine now
 	for _, s := range app.h3servers {
 		// TODO: CloseGracefully, once implemented upstream
 		// (see https://github.com/lucas-clemente/quic-go/issues/2103)
@@ -441,17 +431,6 @@ func (app *App) Stop() error {
 			return err
 		}
 	}
-
-	// closing an http3.Server does not close their underlying listeners
-	// since apparently the listener can be used both by servers and
-	// clients at the same time; so we need to manually call Close()
-	// on the underlying h3 listeners (see lucas-clemente/quic-go#2103)
-	for _, pc := range app.h3listeners {
-		err := pc.Close()
-		if err != nil {
-			return err
-		}
-	}
 	return nil
 }