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>

diff --git a/src/net/http/server.go b/src/net/http/server.go
index 439efa0c7530681b68149eaaf6d7ba40ad5c438f..b452f643bd3ff554aece7e555e0c6e8ca7a74c28 100644 (file)
--- 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: