net/http: allocate CloseNotifier channel lazily

The CloseNotifier interface is deprecated. We can defer allocating the
backing channel until the first use of CloseNotifier.

goos: linux
goarch: amd64
pkg: net/http
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                   │   before    │               after                │
                   │   sec/op    │   sec/op     vs base               │
Server-8             160.8µ ± 2%   160.1µ ± 1%       ~ (p=0.353 n=10)
CloseNotifier/h1-8   222.1µ ± 4%   226.4µ ± 7%       ~ (p=0.143 n=10)
geomean              189.0µ        190.4µ       +0.75%

                   │    before    │                after                │
                   │     B/op     │     B/op      vs base               │
Server-8             2.292Ki ± 0%   2.199Ki ± 0%  -4.07% (p=0.000 n=10)
CloseNotifier/h1-8   3.224Ki ± 0%   3.241Ki ± 0%  +0.51% (p=0.000 n=10)
geomean              2.718Ki        2.669Ki       -1.80%

                   │   before   │                after                │
                   │ allocs/op  │ allocs/op   vs base                 │
Server-8             21.00 ± 0%   20.00 ± 0%  -4.76% (p=0.000 n=10)
CloseNotifier/h1-8   50.00 ± 0%   50.00 ± 0%       ~ (p=1.000 n=10) ¹
geomean              32.40        31.62       -2.41%
¹ all samples are equal

Change-Id: I3f35d56b8356fb660589b7708a023e4480f32067
GitHub-Last-Rev: c75696b9b8498ae03a4ad9527b9b7c8337415456
GitHub-Pull-Request: golang/go#71163
Reviewed-on: https://go-review.googlesource.com/c/go/+/640598
Reviewed-by: Damien Neil <dneil@google.com>
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Auto-Submit: Damien Neil <dneil@google.com>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>

1 files changed, 28 insertions, 9 deletions

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 439efa0c75..b452f643bd 100644
--- a/src/net/http/server.go
+++ b/src/net/http/server.go
@@ -261,7 +261,7 @@ type conn struct {
 
 	// rwc is the underlying network connection.
 	// This is never wrapped by other types and is the value given out
-	// to CloseNotifier callers. It is usually of type *net.TCPConn or
+	// to [Hijacker] callers. It is usually of type *net.TCPConn or
 	// *tls.Conn.
 	rwc net.Conn
 
@@ -486,11 +486,12 @@ type response struct {
 	clenBuf   [10]byte
 	statusBuf [3]byte
 
+	// lazyCloseNotifyMu protects closeNotifyCh and closeNotifyTriggered.
+	lazyCloseNotifyMu sync.Mutex
 	// closeNotifyCh is the channel returned by CloseNotify.
-	// TODO(bradfitz): this is currently (for Go 1.8) always
-	// non-nil. Make this lazily-created again as it used to be?
-	closeNotifyCh  chan bool
-	didCloseNotify atomic.Bool // atomic (only false->true winner should send)
+	closeNotifyCh chan bool
+	// closeNotifyTriggered tracks prior closeNotify calls.
+	closeNotifyTriggered bool
 }
 
 func (c *response) SetReadDeadline(deadline time.Time) error {
@@ -761,9 +762,8 @@ func (cr *connReader) handleReadError(_ error) {
 
 // may be called from multiple goroutines.
 func (cr *connReader) closeNotify() {
-	res := cr.conn.curReq.Load()
-	if res != nil && !res.didCloseNotify.Swap(true) {
-		res.closeNotifyCh <- true
+	if res := cr.conn.curReq.Load(); res != nil {
+		res.closeNotify()
 	}
 }
 
@@ -1078,7 +1078,6 @@ func (c *conn) readRequest(ctx context.Context) (w *response, err error) {
 		reqBody:       req.Body,
 		handlerHeader: make(Header),
 		contentLength: -1,
-		closeNotifyCh: make(chan bool, 1),
 
 		// We populate these ahead of time so we're not
 		// reading from req.Header after their Handler starts
@@ -2228,12 +2227,32 @@ func (w *response) Hijack() (rwc net.Conn, buf *bufio.ReadWriter, err error) {
 }
 
 func (w *response) CloseNotify() <-chan bool {
+	w.lazyCloseNotifyMu.Lock()
+	defer w.lazyCloseNotifyMu.Unlock()
 	if w.handlerDone.Load() {
 		panic("net/http: CloseNotify called after ServeHTTP finished")
 	}
+	if w.closeNotifyCh == nil {
+		w.closeNotifyCh = make(chan bool, 1)
+		if w.closeNotifyTriggered {
+			w.closeNotifyCh <- true // action prior closeNotify call
+		}
+	}
 	return w.closeNotifyCh
 }
 
+func (w *response) closeNotify() {
+	w.lazyCloseNotifyMu.Lock()
+	defer w.lazyCloseNotifyMu.Unlock()
+	if w.closeNotifyTriggered {
+		return // already triggered
+	}
+	w.closeNotifyTriggered = true
+	if w.closeNotifyCh != nil {
+		w.closeNotifyCh <- true
+	}
+}
+
 func registerOnHitEOF(rc io.ReadCloser, fn func()) {
 	switch v := rc.(type) {
 	case *expectContinueReader: