From: Alexander Yastrebov Date: Thu, 7 Mar 2024 10:28:46 +0000 (+0000) Subject: net/http: remove persistConn reference from wantConn X-Git-Tag: go1.23rc1~963 X-Git-Url: http://www.git.cypherpunks.su/?a=commitdiff_plain;h=8590f0aef30d1e4d242af97ae15266facb26493b;p=gostls13.git net/http: remove persistConn reference from wantConn Transport getConn creates wantConn w, tries to obtain idle connection for it based on the w.key and, when there is no idle connection, puts wantConn into idleConnWait wantConnQueue. Then getConn dials connection for w in a goroutine and blocks. After dial succeeds getConn unblocks and returns connection to the caller. At this point w is stored in the idleConnWait and will not be evicted until another wantConn with the same w.key is requested or alive connection returned into the idle pool which may not happen e.g. if server closes the connection. The problem is that even after tryDeliver succeeds w references persistConn wrapper that allocates bufio.Reader and bufio.Writer and prevents them from being garbage collected. To fix the problem this change removes persistConn and error references from wantConn and delivers them via channel to getConn. This way wantConn could be kept in wantConnQueues arbitrary long. Fixes #43966 Fixes #50798 Change-Id: I77942552f7db04c225fb40d770b3101a8cfe655d GitHub-Last-Rev: 027a0833f98b23ddadb3ec7ee4f2e62653bc7705 GitHub-Pull-Request: golang/go#62227 Reviewed-on: https://go-review.googlesource.com/c/go/+/522095 Reviewed-by: Damien Neil LUCI-TryBot-Result: Go LUCI Reviewed-by: Michael Knyszek Auto-Submit: Damien Neil --- diff --git a/src/net/http/transport.go b/src/net/http/transport.go index 75934f00de..828da01247 100644 --- a/src/net/http/transport.go +++ b/src/net/http/transport.go @@ -957,7 +957,7 @@ func (t *Transport) tryPutIdleConn(pconn *persistConn) error { // Loop over the waiting list until we find a w that isn't done already, and hand it pconn. for q.len() > 0 { w := q.popFront() - if w.tryDeliver(pconn, nil) { + if w.tryDeliver(pconn, nil, time.Time{}) { done = true break } @@ -969,7 +969,7 @@ func (t *Transport) tryPutIdleConn(pconn *persistConn) error { // list unconditionally, for any future clients too. for q.len() > 0 { w := q.popFront() - w.tryDeliver(pconn, nil) + w.tryDeliver(pconn, nil, time.Time{}) } } if q.len() == 0 { @@ -1073,7 +1073,7 @@ func (t *Transport) queueForIdleConn(w *wantConn) (delivered bool) { list = list[:len(list)-1] continue } - delivered = w.tryDeliver(pconn, nil) + delivered = w.tryDeliver(pconn, nil, pconn.idleAt) if delivered { if pconn.alt != nil { // HTTP/2: multiple clients can share pconn. @@ -1207,9 +1207,8 @@ func (t *Transport) dial(ctx context.Context, network, addr string) (net.Conn, e // These three options are racing against each other and use // wantConn to coordinate and agree about the winning outcome. type wantConn struct { - cm connectMethod - key connectMethodKey // cm.key() - ready chan struct{} // closed when pc, err pair is delivered + cm connectMethod + key connectMethodKey // cm.key() // hooks for testing to know when dials are done // beforeDial is called in the getConn goroutine when the dial is queued. @@ -1217,45 +1216,51 @@ type wantConn struct { beforeDial func() afterDial func() - mu sync.Mutex // protects ctx, pc, err, close(ready) - ctx context.Context // context for dial, cleared after delivered or canceled - pc *persistConn - err error + mu sync.Mutex // protects ctx, done and sending of the result + ctx context.Context // context for dial, cleared after delivered or canceled + done bool // true after delivered or canceled + result chan connOrError // channel to deliver connection or error +} + +type connOrError struct { + pc *persistConn + err error + idleAt time.Time } // waiting reports whether w is still waiting for an answer (connection or error). func (w *wantConn) waiting() bool { - select { - case <-w.ready: - return false - default: - return true - } + w.mu.Lock() + defer w.mu.Unlock() + + return !w.done } // getCtxForDial returns context for dial or nil if connection was delivered or canceled. func (w *wantConn) getCtxForDial() context.Context { w.mu.Lock() defer w.mu.Unlock() + return w.ctx } // tryDeliver attempts to deliver pc, err to w and reports whether it succeeded. -func (w *wantConn) tryDeliver(pc *persistConn, err error) bool { +func (w *wantConn) tryDeliver(pc *persistConn, err error, idleAt time.Time) bool { w.mu.Lock() defer w.mu.Unlock() - if w.pc != nil || w.err != nil { + if w.done { return false } - - w.ctx = nil - w.pc = pc - w.err = err - if w.pc == nil && w.err == nil { + if (pc == nil) == (err == nil) { panic("net/http: internal error: misuse of tryDeliver") } - close(w.ready) + w.ctx = nil + w.done = true + + w.result <- connOrError{pc: pc, err: err, idleAt: idleAt} + close(w.result) + return true } @@ -1263,13 +1268,16 @@ func (w *wantConn) tryDeliver(pc *persistConn, err error) bool { // If a connection has been delivered already, cancel returns it with t.putOrCloseIdleConn. func (w *wantConn) cancel(t *Transport, err error) { w.mu.Lock() - if w.pc == nil && w.err == nil { - close(w.ready) // catch misbehavior in future delivery + var pc *persistConn + if w.done { + if r, ok := <-w.result; ok { + pc = r.pc + } + } else { + close(w.result) } - pc := w.pc w.ctx = nil - w.pc = nil - w.err = err + w.done = true w.mu.Unlock() if pc != nil { @@ -1359,7 +1367,7 @@ func (t *Transport) customDialTLS(ctx context.Context, network, addr string) (co // specified in the connectMethod. This includes doing a proxy CONNECT // and/or setting up TLS. If this doesn't return an error, the persistConn // is ready to write requests to. -func (t *Transport) getConn(treq *transportRequest, cm connectMethod) (pc *persistConn, err error) { +func (t *Transport) getConn(treq *transportRequest, cm connectMethod) (_ *persistConn, err error) { req := treq.Request trace := treq.trace ctx := req.Context() @@ -1371,7 +1379,7 @@ func (t *Transport) getConn(treq *transportRequest, cm connectMethod) (pc *persi cm: cm, key: cm.key(), ctx: ctx, - ready: make(chan struct{}, 1), + result: make(chan connOrError, 1), beforeDial: testHookPrePendingDial, afterDial: testHookPostPendingDial, } @@ -1381,38 +1389,41 @@ func (t *Transport) getConn(treq *transportRequest, cm connectMethod) (pc *persi } }() + var cancelc chan error + // Queue for idle connection. if delivered := t.queueForIdleConn(w); delivered { - pc := w.pc - // Trace only for HTTP/1. - // HTTP/2 calls trace.GotConn itself. - if pc.alt == nil && trace != nil && trace.GotConn != nil { - trace.GotConn(pc.gotIdleConnTrace(pc.idleAt)) - } // set request canceler to some non-nil function so we // can detect whether it was cleared between now and when // we enter roundTrip t.setReqCanceler(treq.cancelKey, func(error) {}) - return pc, nil - } - - cancelc := make(chan error, 1) - t.setReqCanceler(treq.cancelKey, func(err error) { cancelc <- err }) + } else { + cancelc = make(chan error, 1) + t.setReqCanceler(treq.cancelKey, func(err error) { cancelc <- err }) - // Queue for permission to dial. - t.queueForDial(w) + // Queue for permission to dial. + t.queueForDial(w) + } // Wait for completion or cancellation. select { - case <-w.ready: + case r := <-w.result: // Trace success but only for HTTP/1. // HTTP/2 calls trace.GotConn itself. - if w.pc != nil && w.pc.alt == nil && trace != nil && trace.GotConn != nil { - trace.GotConn(httptrace.GotConnInfo{Conn: w.pc.conn, Reused: w.pc.isReused()}) + if r.pc != nil && r.pc.alt == nil && trace != nil && trace.GotConn != nil { + info := httptrace.GotConnInfo{ + Conn: r.pc.conn, + Reused: r.pc.isReused(), + } + if !r.idleAt.IsZero() { + info.WasIdle = true + info.IdleTime = time.Since(r.idleAt) + } + trace.GotConn(info) } - if w.err != nil { + if r.err != nil { // If the request has been canceled, that's probably - // what caused w.err; if so, prefer to return the + // what caused r.err; if so, prefer to return the // cancellation error (see golang.org/issue/16049). select { case <-req.Cancel: @@ -1428,7 +1439,7 @@ func (t *Transport) getConn(treq *transportRequest, cm connectMethod) (pc *persi // return below } } - return w.pc, w.err + return r.pc, r.err case <-req.Cancel: return nil, errRequestCanceledConn case <-req.Context().Done(): @@ -1483,7 +1494,7 @@ func (t *Transport) dialConnFor(w *wantConn) { } pc, err := t.dialConn(ctx, w.cm) - delivered := w.tryDeliver(pc, err) + delivered := w.tryDeliver(pc, err, time.Time{}) if err == nil && (!delivered || pc.alt != nil) { // pconn was not passed to w, // or it is HTTP/2 and can be shared. @@ -2007,18 +2018,6 @@ func (pc *persistConn) isReused() bool { return r } -func (pc *persistConn) gotIdleConnTrace(idleAt time.Time) (t httptrace.GotConnInfo) { - pc.mu.Lock() - defer pc.mu.Unlock() - t.Reused = pc.reused - t.Conn = pc.conn - t.WasIdle = true - if !idleAt.IsZero() { - t.IdleTime = time.Since(idleAt) - } - return -} - func (pc *persistConn) cancelRequest(err error) { pc.mu.Lock() defer pc.mu.Unlock()