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 <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Chris Broadfoot <cbro@golang.org>

diff --git a/src/net/http/serve_test.go b/src/net/http/serve_test.go
index 139ce3eafc7e1ff4a5de2d3ea35afe296a9dd015..13e5f283e4c4d6dbc8285e70ed64885583aed927 100644 (file)
--- 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 7b2b4b2f42307245e887f9e3a82f390c6b3e69d7..89574a8b36e7e9300df967816621f2b59b35da0f 100644 (file)
--- 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