post-review update

keepalive/keepalive.go

@@ -4,12 +4,15 @@ import (
 	"time"
 )
 
-// ClientParameters is used to set keepalive parameters for the client side.
+// ClientParameters is used to set keepalive parameters on the client-side.
+// These configure how the client will actively probe to notice when a connection broken
+// and to cause activity so intermediaries are aware the connection is still in use.
 type ClientParameters struct {
-	// After a duration of this time the client pings the server to see if the transport is still alive.
-	Time time.Duration
-	// After having pinged for keepalive check, the client waits for a duration of keepalive_timeout before closing the transport.
-	Timeout time.Duration
+	// After a duration of this time if the client doesn't see any activity it pings the server to see if the transport is still alive.
+	Time time.Duration // The current default value is infinity.
+	// After having pinged for keepalive check, the client waits for a duration of Timeout and if no activity is seen even after that
+	// the connection is closed.
+	Timeout time.Duration // The current default value is 20 seconds.
 	// If true, client runs keepalive checks even with no active RPCs.
 	PermitWithoutStream bool
 }

transport/http2_client.go

@@ -82,7 +82,7 @@ type http2Client struct {
 	// goAway is closed to notify the upper layer (i.e., addrConn.transportMonitor)
 	// that the server sent GoAway on this transport.
 	goAway chan struct{}
-	// awakenKeepalive is used to tell keepalive goroutine to reset keepalive timer.
+	// awakenKeepalive is used to wake up keepalive when after it has gone dormant.
 	awakenKeepalive chan struct{}
 
 	framer *framer
@@ -231,7 +231,7 @@ func newHTTP2Client(ctx context.Context, addr TargetInfo, opts ConnectOptions) (
 		kp:              kp,
 		statsHandler:    opts.StatsHandler,
 	}
-	// make sure awakenKeepalive can't be written upon.
+	// Make sure awakenKeepalive can't be written upon.
 	// keepalive routine will make it writable, if need be.
 	t.awakenKeepalive <- struct{}{}
 	if t.statsHandler != nil {
@@ -1147,7 +1147,7 @@ func (t *http2Client) keepalive() {
 	for {
 		select {
 		case <-timer.C:
-			if a := atomic.SwapUint32(&t.activity, 0); a == 1 {
+			if atomic.CompareAndSwapUint32(&t.activity, 1, 0) {
 				timer.Reset(t.kp.Time)
 				continue
 			}
@@ -1174,7 +1174,7 @@ func (t *http2Client) keepalive() {
 			timer.Reset(t.kp.Timeout)
 			select {
 			case <-timer.C:
-				if a := atomic.SwapUint32(&t.activity, 0); a == 1 {
+				if atomic.CompareAndSwapUint32(&t.activity, 1, 0) {
 					timer.Reset(t.kp.Time)
 					continue
 				}

transport/transport_test.go

@@ -292,6 +292,10 @@ func setUpWithNoPingServer(t *testing.T, copts ConnectOptions, done chan net.Con
 	}()
 	tr, err := NewClientTransport(context.Background(), TargetInfo{Addr: lis.Addr().String()}, copts)
 	if err != nil {
+		// Server clean-up.
+		if conn, ok := <-done; ok {
+			conn.Close()
+		}
 		t.Fatalf("Failed to dial: %v", err)
 	}
 	return tr