Skip to content

Commit

Permalink
net/http: optimize memory usage after hijacking and handler exits
Browse files Browse the repository at this point in the history
Avoid holding on to the "connReader" after hijacking. This allows all of
the "conn", "response" and "Request" to be garbage collected after the
ServeHTTP handler exits.

Use a weak-ref on the "response" to allow CloseNotify to work _before_
the handler has exited (i.e. while the "response" is still referenced in
"conn.serve"). This aligns with the documentation of CloseNotifier:
> After the Handler has returned, there is no guarantee that the channel
> receives a value.

goos: linux
goarch: amd64
pkg: net/http
cpu: Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
                          │ lazy-notify  │               reader                │
                          │    sec/op    │    sec/op     vs base               │
Server-8                    507.0µ ±  1%   499.5µ ±  2%  -1.48% (p=0.002 n=10)
ServerHijack-8              44.09µ ± 29%   44.56µ ± 27%       ~ (p=0.529 n=10)
ServerHijackMemoryUsage-8   1.181m ± 15%   1.104m ±  9%       ~ (p=0.143 n=10)
geomean                     297.7µ         290.7µ        -2.36%

                          │ lazy-notify  │               reader                │
                          │     B/op     │     B/op      vs base               │
Server-8                    2.185Ki ± 0%   2.188Ki ± 0%  +0.13% (p=0.014 n=10)
ServerHijack-8              12.03Ki ± 0%   12.09Ki ± 0%  +0.45% (p=0.000 n=10)
ServerHijackMemoryUsage-8   11.51Ki ± 0%   11.57Ki ± 0%  +0.50% (p=0.000 n=10)
geomean                     6.714Ki        6.738Ki       +0.36%

                          │ lazy-notify │               reader                │
                          │  allocs/op  │ allocs/op   vs base                 │
Server-8                     20.00 ± 0%   20.00 ± 0%       ~ (p=1.000 n=10) ¹
ServerHijack-8               49.00 ± 0%   51.00 ± 0%  +4.08% (p=0.000 n=10)
ServerHijackMemoryUsage-8    45.00 ± 0%   47.00 ± 0%  +4.44% (p=0.000 n=10)
geomean                      35.33        36.33       +2.82%
¹ all samples are equal

                          │  lazy-notify  │                reader                 │
                          │ retained_B/op │ retained_B/op  vs base                │
ServerHijackMemoryUsage-8    11.339k ± 0%     8.714k ± 0%  -23.15% (p=0.000 n=10)
  • Loading branch information
das7pad committed Dec 10, 2024
1 parent 8550264 commit 9f49d26
Showing 1 changed file with 50 additions and 2 deletions.
52 changes: 50 additions & 2 deletions src/net/http/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ import (
"sync/atomic"
"time"
_ "unsafe" // for linkname
"weak"

"golang.org/x/net/http/httpguts"
)
Expand Down Expand Up @@ -314,7 +315,7 @@ func (c *conn) hijacked() bool {
}

// c.mu must be held.
func (c *conn) hijackLocked() (rwc net.Conn, buf *bufio.ReadWriter, err error) {
func (c *conn) hijackLocked(w *response) (rwc net.Conn, buf *bufio.ReadWriter, err error) {
if c.hijackedv {
return nil, nil, ErrHijacked
}
Expand All @@ -329,6 +330,9 @@ func (c *conn) hijackLocked() (rwc net.Conn, buf *bufio.ReadWriter, err error) {
return nil, nil, fmt.Errorf("unexpected Peek failure reading buffered byte: %v", err)
}
}

ccn := &connCloseNotify{rwc: rwc, weakResponse: weak.Make(w)}
ccn.attach(c.bufr)
c.bufw.Reset(rwc)
buf = bufio.NewReadWriter(c.bufr, c.bufw)

Expand Down Expand Up @@ -650,6 +654,50 @@ type readResult struct {
b byte // byte read, if n == 1
}

// connCloseNotify is a minimal version of [connReader] for use by [Hijacker]
// to notify any [CloseNotifier] on read errors before the handler exists. It
// allows the hijacked [conn], [response] and [Request] to get garbage
// collected after the handler exits.
type connCloseNotify struct {
// rwc is the underlying network connection.
rwc net.Conn
// weakResponse points at the [response] until the handler exits.
weakResponse weak.Pointer[response]
// previousBufferContents holds on to previously buffered bytes of the read buffer.
previousBufferContents []byte
}

func (c *connCloseNotify) Read(p []byte) (n int, err error) {
if c.previousBufferContents != nil {
n = copy(p, c.previousBufferContents)
c.previousBufferContents = nil
return
}
n, err = c.rwc.Read(p)
if err != nil {
if w := c.weakResponse.Value(); w != nil {
w.cancelCtx()
w.closeNotify()
}
}
return
}

// attach preserves the buffered bytes while attaching to the given reader.
func (c *connCloseNotify) attach(r *bufio.Reader) (err error) {
n := r.Buffered()
if n != 0 {
if c.previousBufferContents, err = r.Peek(n); err != nil {
return
}
}
r.Reset(c)
if n != 0 {
_, err = r.Peek(n)
}
return
}

// connReader is the io.Reader wrapper used by *conn. It combines a
// selectively-activated io.LimitedReader (to bound request header
// read sizes) with support for selectively keeping an io.Reader.Read
Expand Down Expand Up @@ -2244,7 +2292,7 @@ func (w *response) Hijack() (rwc net.Conn, buf *bufio.ReadWriter, err error) {

// Release the bufioWriter that writes to the chunk writer, it is not
// used after a connection has been hijacked.
rwc, buf, err = c.hijackLocked()
rwc, buf, err = c.hijackLocked(w)
if err == nil {
putBufioWriter(w.w)
w.w = nil
Expand Down

0 comments on commit 9f49d26

Please sign in to comment.