From 30b9d59a766858a4b51148e47edb3af2766ab617 Mon Sep 17 00:00:00 2001
From: Menghan Li <menghanl@google.com>
Date: Fri, 20 May 2022 15:17:29 -0700
Subject: [PATCH] client/SubConn: do not recreate addrConn if UpdateAddresses
 is called with the same addresses (#5373)

---
 clientconn.go      | 25 ++++++++++++++++++++++---
 clientconn_test.go | 16 +++++++++-------
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/clientconn.go b/clientconn.go
index ea9836d28b3..3fe28732b80 100644
--- a/clientconn.go
+++ b/clientconn.go
@@ -801,16 +801,31 @@ func (ac *addrConn) connect() error {
 	return nil
 }
 
+func equalAddresses(a, b []resolver.Address) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for i, v := range a {
+		if !v.Equal(b[i]) {
+			return false
+		}
+	}
+	return true
+}
+
 // tryUpdateAddrs tries to update ac.addrs with the new addresses list.
 //
-// If ac is Connecting, it returns false. The caller should tear down the ac and
-// create a new one. Note that the backoff will be reset when this happens.
-//
 // If ac is TransientFailure, it updates ac.addrs and returns true. The updated
 // addresses will be picked up by retry in the next iteration after backoff.
 //
 // If ac is Shutdown or Idle, it updates ac.addrs and returns true.
 //
+// If the addresses is the same as the old list, it does nothing and returns
+// true.
+//
+// If ac is Connecting, it returns false. The caller should tear down the ac and
+// create a new one. Note that the backoff will be reset when this happens.
+//
 // If ac is Ready, it checks whether current connected address of ac is in the
 // new addrs list.
 //  - If true, it updates ac.addrs and returns true. The ac will keep using
@@ -827,6 +842,10 @@ func (ac *addrConn) tryUpdateAddrs(addrs []resolver.Address) bool {
 		return true
 	}
 
+	if equalAddresses(ac.addrs, addrs) {
+		return true
+	}
+
 	if ac.state == connectivity.Connecting {
 		return false
 	}
diff --git a/clientconn_test.go b/clientconn_test.go
index 21f70c8f251..9f32999709f 100644
--- a/clientconn_test.go
+++ b/clientconn_test.go
@@ -854,9 +854,10 @@ func (s) TestBackoffCancel(t *testing.T) {
 	}
 }
 
-// UpdateAddresses should cause the next reconnect to begin from the top of the
-// list if the connection is not READY.
-func (s) TestUpdateAddresses_RetryFromFirstAddr(t *testing.T) {
+// TestUpdateAddresses_NoopIfCalledWithSameAddresses tests that UpdateAddresses
+// should be noop if UpdateAddresses is called with the same list of addresses,
+// even when the SubConn is in Connecting and doesn't have a current address.
+func (s) TestUpdateAddresses_NoopIfCalledWithSameAddresses(t *testing.T) {
 	lis1, err := net.Listen("tcp", "localhost:0")
 	if err != nil {
 		t.Fatalf("Error while listening. Err: %v", err)
@@ -1008,19 +1009,20 @@ func (s) TestUpdateAddresses_RetryFromFirstAddr(t *testing.T) {
 	}
 	client.mu.Unlock()
 
+	// Call UpdateAddresses with the same list of addresses, it should be a noop
+	// (even when the SubConn is Connecting, and doesn't have a curAddr).
 	ac.acbw.UpdateAddresses(addrsList)
 
 	// We've called tryUpdateAddrs - now let's make server2 close the
-	// connection and check that it goes back to server1 instead of continuing
-	// to server3 or trying server2 again.
+	// connection and check that it continues to server3.
 	close(closeServer2)
 
 	select {
 	case <-server1ContactedSecondTime:
+		t.Fatal("server1 was contacted a second time, but it should have continued to server 3")
 	case <-server2ContactedSecondTime:
-		t.Fatal("server2 was contacted a second time, but it after tryUpdateAddrs it should have re-started the list and tried server1")
+		t.Fatal("server2 was contacted a second time, but it should have continued to server 3")
 	case <-server3Contacted:
-		t.Fatal("server3 was contacted, but after tryUpdateAddrs it should have re-started the list and tried server1")
 	case <-timeout:
 		t.Fatal("timed out waiting for any server to be contacted after tryUpdateAddrs")
 	}