remove pingPeriod. Rely on idleTimeout to ping periodically

Author: Chao Xu

http2/transport.go

@@ -108,23 +108,17 @@ type Transport struct {
 	// waiting for their turn.
 	StrictMaxConcurrentStreams bool
 
-	// PingPeriod controls how often pings are sent on idle connections to
-	// check the liveness of the connection. The connection will be closed
-	// if response is not received within PingTimeout.
-	// 0 means no periodic pings. Defaults to 0.
-	PingPeriod time.Duration
-
 	// PingTimeout is the timeout after which the connection will be closed
 	// if a response to Ping is not received.
-	// Defaults to PingPeriod/2
+	// Defaults to 1s.
 	PingTimeout time.Duration
 
-	// ReadIdleTimeout is the timeout after which the periodic ping for
-	// connection health check will begin if no frame is received on the
-	// connection.
-	// The health check will stop once there is a frame received on the
-	// connection.
-	// Defaults to 60s.
+	// ReadIdleTimeout is the timeout after which a health check using ping
+	// frame will be carried out if no frame is received on the connection.
+	// Note that a ping response will is considered a received frame, so if
+	// there is no other traffic on the connection, the health check will
+	// be performed every ReadIdleTimeout interval.
+	// Default to 0, which means no health check.
 	ReadIdleTimeout time.Duration
 
 	// t1, if non-nil, is the standard library Transport using
@@ -160,7 +154,7 @@ func (t *Transport) readIdleTimeout() time.Duration {
 
 func (t *Transport) pingTimeout() time.Duration {
 	if t.PingTimeout == 0 {
-		return t.PingPeriod / 2
+		return 1 * time.Second
 	}
 	return t.PingTimeout
 
@@ -274,9 +268,6 @@ type ClientConn struct {
 
 	wmu  sync.Mutex // held while writing; acquire AFTER mu if holding both
 	werr error      // first write error that has occurred
-
-	hmu               sync.Mutex    // guard the healthCheckStopCh
-	healthCheckStopCh chan struct{} // A close-only channel to stop the health check.
 }
 
 // clientStream is the state for a single HTTP/2 stream. One of these
@@ -712,51 +703,18 @@ func (t *Transport) newClientConn(c net.Conn, singleUse bool) (*ClientConn, erro
 	return cc, nil
 }
 
-func (cc *ClientConn) healthCheck(stop chan struct{}) {
-	pingPeriod := cc.t.PingPeriod
+func (cc *ClientConn) healthCheck() {
 	pingTimeout := cc.t.pingTimeout()
-	if pingPeriod == 0 || pingTimeout == 0 {
-		return
-	}
-	ticker := time.NewTicker(pingPeriod)
-	defer ticker.Stop()
-	for {
-		select {
-		case <-stop:
-			return
-		case <-ticker.C:
-			ctx, cancel := context.WithTimeout(context.Background(), pingTimeout)
-			err := cc.Ping(ctx)
-			cancel()
-			if err != nil {
-				cc.closeForLostPing()
-				cc.t.connPool().MarkDead(cc)
-				return
-			}
-		}
-	}
-}
-
-func (cc *ClientConn) startHealthCheck() {
-	cc.hmu.Lock()
-	defer cc.hmu.Unlock()
-	if cc.healthCheckStopCh != nil {
-		// a health check is already running
-		return
-	}
-	cc.healthCheckStopCh = make(chan struct{})
-	go cc.healthCheck(cc.healthCheckStopCh)
-}
-
-func (cc *ClientConn) stopHealthCheck() {
-	cc.hmu.Lock()
-	defer cc.hmu.Unlock()
-	if cc.healthCheckStopCh == nil {
-		// no health check running
+	// 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)
+	err := cc.Ping(ctx)
+	cancel()
+	if err != nil {
+		cc.closeForLostPing()
+		cc.t.connPool().MarkDead(cc)
 		return
 	}
-	close(cc.healthCheckStopCh)
-	cc.healthCheckStopCh = nil
 }
 
 func (cc *ClientConn) setGoAway(f *GoAwayFrame) {
@@ -1804,11 +1762,15 @@ func (rl *clientConnReadLoop) run() error {
 	gotReply := false // ever saw a HEADERS reply
 	gotSettings := false
 	to := cc.t.readIdleTimeout()
-	t := time.AfterFunc(to, cc.startHealthCheck)
+	var t *time.Timer
+	if to != 0 {
+		t = time.AfterFunc(to, cc.healthCheck)
+	}
 	for {
 		f, err := cc.fr.ReadFrame()
-		t.Reset(to)
-		cc.stopHealthCheck()
+		if to != 0 {
+			t.Reset(to)
+		}
 		if err != nil {
 			cc.vlogf("http2: Transport readFrame error on conn %p: (%T) %v", cc, err, err)
 		}

http2/transport_test.go

@@ -3247,16 +3247,15 @@ func TestTransportNoRaceOnRequestObjectAfterRequestComplete(t *testing.T) {
 func TestTransportCloseAfterLostPing(t *testing.T) {
 	clientDone := make(chan struct{})
 	ct := newClientTester(t)
-	ct.tr.PingPeriod = 1 * time.Second
 	ct.tr.PingTimeout = 1 * time.Second
 	ct.tr.ReadIdleTimeout = 1 * time.Second
 	ct.client = func() error {
 		defer ct.cc.(*net.TCPConn).CloseWrite()
 		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 is not responded") {
-			return fmt.Errorf("expected to get error about \"ping frame is not responded\", got %v", err)
+		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)
 		}
 		return nil
 	}
@@ -3269,16 +3268,15 @@ func TestTransportCloseAfterLostPing(t *testing.T) {
 }
 
 func TestTransportPingWhenReading(t *testing.T) {
-	testTransportPingWhenReading(t, 50*time.Millisecond, 100*time.Millisecond)
-	testTransportPingWhenReading(t, 100*time.Millisecond, 50*time.Millisecond)
+	testTransportPingWhenReading(t, 50*time.Millisecond, 110*time.Millisecond, 20)
+	testTransportPingWhenReading(t, 100*time.Millisecond, 50*time.Millisecond, 0)
 }
 
-func testTransportPingWhenReading(t *testing.T, readIdleTimeout, serverResponseInterval time.Duration) {
-	var pinged bool
+func testTransportPingWhenReading(t *testing.T, readIdleTimeout, serverResponseInterval time.Duration, expectedPingCount int) {
+	var pingCount int
 	clientBodyBytes := []byte("hello, this is client")
 	clientDone := make(chan struct{})
 	ct := newClientTester(t)
-	ct.tr.PingPeriod = 10 * time.Millisecond
 	ct.tr.PingTimeout = 10 * time.Millisecond
 	ct.tr.ReadIdleTimeout = readIdleTimeout
 	// guards the ct.fr.Write
@@ -3375,25 +3373,22 @@ func testTransportPingWhenReading(t *testing.T, readIdleTimeout, serverResponseI
 					}()
 				}
 			case *PingFrame:
-				pinged = true
-				if serverResponseInterval > readIdleTimeout {
-					wmu.Lock()
-					if err := ct.fr.WritePing(true, f.Data); err != nil {
-						wmu.Unlock()
-						return err
-					}
+				pingCount++
+				wmu.Lock()
+				if err := ct.fr.WritePing(true, f.Data); err != nil {
 					wmu.Unlock()
-				} else {
-					return fmt.Errorf("Unexpected ping frame: %v", f)
+					return err
 				}
+				wmu.Unlock()
 			default:
 				return fmt.Errorf("Unexpected client frame %v", f)
 			}
 		}
 	}
 	ct.run()
-	if serverResponseInterval > readIdleTimeout && !pinged {
-		t.Errorf("expect ping")
+	if e, a := expectedPingCount, pingCount; e != a {
+		t.Errorf("expected receiving %d pings, got %d pings", e, a)
+
 	}
 }