Skip to content

Commit

Permalink
http2: make Transport.IdleConnTimeout consider wall (not monotonic) time
Browse files Browse the repository at this point in the history
This is the http2 version of CL 204797.

Updates golang/go#29308 (fixes once bundled into std)

Change-Id: I7cd97d38c941e9a8a62808e23b6533c72760f003
Reviewed-on: https://go-review.googlesource.com/c/net/+/208798
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Bryan C. Mills <bcmills@google.com>
  • Loading branch information
bradfitz committed Nov 26, 2019
1 parent ffdde10 commit ef20fe5
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 1 deletion.
16 changes: 15 additions & 1 deletion http2/transport.go
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,7 @@ type ClientConn struct {
br *bufio.Reader
fr *Framer
lastActive time.Time
lastIdle time.Time // time last idle
// Settings from peer: (also guarded by mu)
maxFrameSize uint32
maxConcurrentStreams uint32
Expand Down Expand Up @@ -736,7 +737,8 @@ func (cc *ClientConn) idleStateLocked() (st clientConnIdleState) {
}

st.canTakeNewRequest = cc.goAway == nil && !cc.closed && !cc.closing && maxConcurrentOkay &&
int64(cc.nextStreamID)+2*int64(cc.pendingRequests) < math.MaxInt32
int64(cc.nextStreamID)+2*int64(cc.pendingRequests) < math.MaxInt32 &&
!cc.tooIdleLocked()
st.freshConn = cc.nextStreamID == 1 && st.canTakeNewRequest
return
}
Expand All @@ -746,6 +748,16 @@ func (cc *ClientConn) canTakeNewRequestLocked() bool {
return st.canTakeNewRequest
}

// tooIdleLocked reports whether this connection has been been sitting idle
// for too much wall time.
func (cc *ClientConn) tooIdleLocked() bool {
// The Round(0) strips the monontonic clock reading so the
// times are compared based on their wall time. We don't want
// to reuse a connection that's been sitting idle during
// VM/laptop suspend if monotonic time was also frozen.
return cc.idleTimeout != 0 && !cc.lastIdle.IsZero() && time.Since(cc.lastIdle.Round(0)) > cc.idleTimeout
}

// onIdleTimeout is called from a time.AfterFunc goroutine. It will
// only be called when we're idle, but because we're coming from a new
// goroutine, there could be a new request coming in at the same time,
Expand Down Expand Up @@ -1150,6 +1162,7 @@ func (cc *ClientConn) awaitOpenSlotForRequest(req *http.Request) error {
}
return errClientConnUnusable
}
cc.lastIdle = time.Time{}
if int64(len(cc.streams))+1 <= int64(cc.maxConcurrentStreams) {
if waitingForConn != nil {
close(waitingForConn)
Expand Down Expand Up @@ -1638,6 +1651,7 @@ func (cc *ClientConn) streamByID(id uint32, andRemove bool) *clientStream {
delete(cc.streams, id)
if len(cc.streams) == 0 && cc.idleTimer != nil {
cc.idleTimer.Reset(cc.idleTimeout)
cc.lastIdle = time.Now()
}
close(cs.done)
// Wake up checkResetOrDone via clientStream.awaitFlowControl and
Expand Down
38 changes: 38 additions & 0 deletions http2/transport_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4483,3 +4483,41 @@ func testTransportBodyLargerThanSpecifiedContentLength(t *testing.T, body *chunk
t.Fatalf("expected %v, got %v", errReqBodyTooLong, err)
}
}

func TestClientConnTooIdle(t *testing.T) {
tests := []struct {
cc func() *ClientConn
want bool
}{
{
func() *ClientConn {
return &ClientConn{idleTimeout: 5 * time.Second, lastIdle: time.Now().Add(-10 * time.Second)}
},
true,
},
{
func() *ClientConn {
return &ClientConn{idleTimeout: 5 * time.Second, lastIdle: time.Time{}}
},
false,
},
{
func() *ClientConn {
return &ClientConn{idleTimeout: 60 * time.Second, lastIdle: time.Now().Add(-10 * time.Second)}
},
false,
},
{
func() *ClientConn {
return &ClientConn{idleTimeout: 0, lastIdle: time.Now().Add(-10 * time.Second)}
},
false,
},
}
for i, tt := range tests {
got := tt.cc().tooIdleLocked()
if got != tt.want {
t.Errorf("%d. got %v; want %v", i, got, tt.want)
}
}
}

0 comments on commit ef20fe5

Please sign in to comment.