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>

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