From c80e0d374ba3caf8ee32c6fe4a5474fa33928086 Mon Sep 17 00:00:00 2001 From: Brad Fitzpatrick Date: Tue, 26 Jul 2016 23:44:00 +0200 Subject: net/http: fix data race with concurrent use of Server.Serve Fixes #16505 Change-Id: I0afabcc8b1be3a5dbee59946b0c44d4c00a28d71 Reviewed-on: https://go-review.googlesource.com/25280 Run-TryBot: Brad Fitzpatrick TryBot-Result: Gobot Gobot Reviewed-by: Chris Broadfoot --- src/net/http/serve_test.go | 11 +++++++++++ src/net/http/server.go | 38 +++++++++++++++++++++++++++++--------- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go index 139ce3eafc..13e5f283e4 100644 --- a/src/net/http/serve_test.go +++ b/src/net/http/serve_test.go @@ -4716,3 +4716,14 @@ func BenchmarkCloseNotifier(b *testing.B) { } b.StopTimer() } + +// Verify this doesn't race (Issue 16505) +func TestConcurrentServerServe(t *testing.T) { + for i := 0; i < 100; i++ { + ln1 := &oneConnListener{conn: nil} + ln2 := &oneConnListener{conn: nil} + srv := Server{} + go func() { srv.Serve(ln1) }() + go func() { srv.Serve(ln2) }() + } +} diff --git a/src/net/http/server.go b/src/net/http/server.go index 7b2b4b2f42..89574a8b36 100644 --- a/src/net/http/server.go +++ b/src/net/http/server.go @@ -2129,8 +2129,8 @@ type Server struct { ErrorLog *log.Logger disableKeepAlives int32 // accessed atomically. - nextProtoOnce sync.Once // guards initialization of TLSNextProto in Serve - nextProtoErr error + nextProtoOnce sync.Once // guards setupHTTP2_* init + nextProtoErr error // result of http2.ConfigureServer if used } // A ConnState represents the state of a client connection to a server. @@ -2260,10 +2260,8 @@ func (srv *Server) Serve(l net.Listener) error { } var tempDelay time.Duration // how long to sleep on accept failure - if srv.shouldConfigureHTTP2ForServe() { - if err := srv.setupHTTP2(); err != nil { - return err - } + if err := srv.setupHTTP2_Serve(); err != nil { + return err } // TODO: allow changing base context? can't imagine concrete @@ -2408,7 +2406,7 @@ func (srv *Server) ListenAndServeTLS(certFile, keyFile string) error { // Setup HTTP/2 before srv.Serve, to initialize srv.TLSConfig // before we clone it and create the TLS Listener. - if err := srv.setupHTTP2(); err != nil { + if err := srv.setupHTTP2_ListenAndServeTLS(); err != nil { return err } @@ -2436,14 +2434,36 @@ func (srv *Server) ListenAndServeTLS(certFile, keyFile string) error { return srv.Serve(tlsListener) } -func (srv *Server) setupHTTP2() error { +// setupHTTP2_ListenAndServeTLS conditionally configures HTTP/2 on +// srv and returns whether there was an error setting it up. If it is +// not configured for policy reasons, nil is returned. +func (srv *Server) setupHTTP2_ListenAndServeTLS() error { srv.nextProtoOnce.Do(srv.onceSetNextProtoDefaults) return srv.nextProtoErr } +// setupHTTP2_Serve is called from (*Server).Serve and conditionally +// configures HTTP/2 on srv using a more conservative policy than +// setupHTTP2_ListenAndServeTLS because Serve may be called +// concurrently. +// +// The tests named TestTransportAutomaticHTTP2* and +// TestConcurrentServerServe in server_test.go demonstrate some +// of the supported use cases and motivations. +func (srv *Server) setupHTTP2_Serve() error { + srv.nextProtoOnce.Do(srv.onceSetNextProtoDefaults_Serve) + return srv.nextProtoErr +} + +func (srv *Server) onceSetNextProtoDefaults_Serve() { + if srv.shouldConfigureHTTP2ForServe() { + srv.onceSetNextProtoDefaults() + } +} + // onceSetNextProtoDefaults configures HTTP/2, if the user hasn't // configured otherwise. (by setting srv.TLSNextProto non-nil) -// It must only be called via srv.nextProtoOnce (use srv.setupHTTP2). +// It must only be called via srv.nextProtoOnce (use srv.setupHTTP2_*). func (srv *Server) onceSetNextProtoDefaults() { if strings.Contains(os.Getenv("GODEBUG"), "http2server=0") { return -- cgit v1.2.3-54-g00ecf