From c9b5e7f77b8aac7334d81c552c583af81ba1c400 Mon Sep 17 00:00:00 2001 From: Artem Mikheev <30644072+renbou@users.noreply.github.com> Date: Wed, 23 Mar 2022 02:47:57 +0300 Subject: [PATCH] Fix http3 servers dying after reload (#4654) --- go.mod | 2 +- go.sum | 17 ++++----- listeners.go | 81 +++++++++++++++++++++++++++++++++++++--- modules/caddyhttp/app.go | 31 +++------------ 4 files changed, 90 insertions(+), 41 deletions(-) diff --git a/go.mod b/go.mod index a7cc1a89..425e8705 100644 --- a/go.mod +++ b/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 diff --git a/go.sum b/go.sum index e6ba33dc..aef242da 100644 --- a/go.sum +++ b/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.25.0/go.mod h1:YtzP8bxRVCBlO77yRanE264+fY/T2U9ZlW1AaHOsMOg= +github.com/lucas-clemente/quic-go v0.26.0 h1:ALBQXr9UJ8A1LyzvceX4jd9QFsHvlI0RR6BkV16o00A= +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.4 h1:xbHbOGGhrenVtII6Co8akhLEdrawwB2iHl5yhJRpnco= -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.0 h1:P9ggrs5xtwiqXv/FHNwntmuLMNq3KaSIG93AtAZ48xk= -github.com/marten-seemann/qtls-go1-17 v0.1.0/go.mod h1:fz4HIxByo+LlWcreM4CZOYNuz3taBQ8rN2X6FqvaWo8= -github.com/marten-seemann/qtls-go1-18 v0.1.0-beta.1 h1:EnzzN9fPUkUck/1CuY1FlzBaIYMoiBsdwTNmNGkwUUM= -github.com/marten-seemann/qtls-go1-18 v0.1.0-beta.1/go.mod h1:PUhIQk19LoFt2174H4+an8TYvWOGjb/hHwphBeaDHwI= +github.com/marten-seemann/qtls-go1-16 v0.1.5 h1:o9JrYPPco/Nukd/HpOHMHZoBDXQqoNtUCmny98/1uqQ= +github.com/marten-seemann/qtls-go1-16 v0.1.5/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.1/go.mod h1:C2ekUKcDdz9SDWxec1N/MvcXBpaX9l3Nx67XaR84L5s= +github.com/marten-seemann/qtls-go1-18 v0.1.1 h1:qp7p7XXUFL7fpBvSS1sWD+uSqPvzNQK43DH+/qEkj0Y= +github.com/marten-seemann/qtls-go1-18 v0.1.1/go.mod h1:mJttiymBAByA49mhlNZZGrH5u1uXYZJ+RW28Py7f4m4= 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= diff --git a/listeners.go b/listeners.go index c23ff783..181db75a 100644 --- a/listeners.go +++ b/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(), - Addr: fcl.Addr(), + Net: l.Addr().Network(), + 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 diff --git a/modules/caddyhttp/app.go b/modules/caddyhttp/app.go index 64cc5401..d81f5563 100644 --- a/modules/caddyhttp/app.go +++ b/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 - h3servers []*http3.Server - h3listeners []net.PacketConn + servers []*http.Server + h3servers []*http3.Server 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 }