From ee11e856d3c26669c8669fed728ecf22ea4f60e0 Mon Sep 17 00:00:00 2001
From: Chao Xu <xuchao@google.com>
Date: Fri, 21 Feb 2020 12:38:08 -0800
Subject: [PATCH] address Michael's comments

---
 http2/transport.go      | 27 ++++++++++-----------------
 http2/transport_test.go |  6 ++++--
 2 files changed, 14 insertions(+), 19 deletions(-)

diff --git a/http2/transport.go b/http2/transport.go
index a8ed28438..22914645e 100644
--- a/http2/transport.go
+++ b/http2/transport.go
@@ -110,7 +110,7 @@ type Transport struct {
 
 	// PingTimeout is the timeout after which the connection will be closed
 	// if a response to Ping is not received.
-	// Defaults to 1s.
+	// Defaults to 15s.
 	PingTimeout time.Duration
 
 	// ReadIdleTimeout is the timeout after which a health check using ping
@@ -144,17 +144,9 @@ func (t *Transport) disableCompression() bool {
 	return t.DisableCompression || (t.t1 != nil && t.t1.DisableCompression)
 }
 
-func (t *Transport) readIdleTimeout() time.Duration {
-	to := t.ReadIdleTimeout
-	if to == 0 {
-		to = 60 * time.Second
-	}
-	return to
-}
-
 func (t *Transport) pingTimeout() time.Duration {
 	if t.PingTimeout == 0 {
-		return 1 * time.Second
+		return 15 * time.Second
 	}
 	return t.PingTimeout
 
@@ -708,8 +700,8 @@ func (cc *ClientConn) healthCheck() {
 	// We don't need to periodically ping in the health check, because the readLoop of ClientConn will
 	// trigger the healthCheck again if there is no frame received.
 	ctx, cancel := context.WithTimeout(context.Background(), pingTimeout)
+	defer cancel()
 	err := cc.Ping(ctx)
-	cancel()
 	if err != nil {
 		cc.closeForLostPing()
 		cc.t.connPool().MarkDead(cc)
@@ -905,7 +897,7 @@ func (cc *ClientConn) Close() error {
 
 // closes the client connection immediately. In-flight requests are interrupted.
 func (cc *ClientConn) closeForLostPing() error {
-	err := errors.New("http2: client connection force closed because ping frame was not answered")
+	err := errors.New("http2: client connection force closed because ping frame was not acknowledged")
 	return cc.closeForError(err)
 }
 
@@ -1761,15 +1753,16 @@ func (rl *clientConnReadLoop) run() error {
 	rl.closeWhenIdle = cc.t.disableKeepAlives() || cc.singleUse
 	gotReply := false // ever saw a HEADERS reply
 	gotSettings := false
-	to := cc.t.readIdleTimeout()
+	readIdleTimeout := cc.t.ReadIdleTimeout
 	var t *time.Timer
-	if to != 0 {
-		t = time.AfterFunc(to, cc.healthCheck)
+	if readIdleTimeout != 0 {
+		t = time.AfterFunc(readIdleTimeout, cc.healthCheck)
+		defer t.Stop()
 	}
 	for {
 		f, err := cc.fr.ReadFrame()
-		if to != 0 {
-			t.Reset(to)
+		if readIdleTimeout != 0 {
+			t.Reset(readIdleTimeout)
 		}
 		if err != nil {
 			cc.vlogf("http2: Transport readFrame error on conn %p: (%T) %v", cc, err, err)
diff --git a/http2/transport_test.go b/http2/transport_test.go
index 734fb7f2a..e2c562855 100644
--- a/http2/transport_test.go
+++ b/http2/transport_test.go
@@ -3254,8 +3254,8 @@ func TestTransportCloseAfterLostPing(t *testing.T) {
 		defer close(clientDone)
 		req, _ := http.NewRequest("GET", "https://dummy.tld/", nil)
 		_, err := ct.tr.RoundTrip(req)
-		if err == nil || !strings.Contains(err.Error(), "ping frame was not answered") {
-			return fmt.Errorf("expected to get error about \"ping frame was not answered\", got %v", err)
+		if err == nil || !strings.Contains(err.Error(), "ping frame was not acknowledged") {
+			return fmt.Errorf("expected to get error about \"ping frame was not acknowledged\", got %v", err)
 		}
 		return nil
 	}
@@ -3270,6 +3270,8 @@ func TestTransportCloseAfterLostPing(t *testing.T) {
 func TestTransportPingWhenReading(t *testing.T) {
 	testTransportPingWhenReading(t, 50*time.Millisecond, 110*time.Millisecond, 20)
 	testTransportPingWhenReading(t, 100*time.Millisecond, 50*time.Millisecond, 0)
+	// 0 means no ping/pong health check.
+	testTransportPingWhenReading(t, 0, 50*time.Millisecond, 0)
 }
 
 func testTransportPingWhenReading(t *testing.T, readIdleTimeout, serverResponseInterval time.Duration, expectedPingCount int) {