From 8e9ceb5c3f74bc25cd37511e1590e45f17cffd30 Mon Sep 17 00:00:00 2001
From: Chengyuan Zhang <chengyuanzhang@google.com>
Date: Mon, 6 Apr 2020 12:08:04 -0700
Subject: [PATCH] core: keep round_robin lb subchannel in TRANSIENT_FAILURE
 until becoming READY (#6657)

Make each subchannel created by RR stay in TRANSIENT_FAILURE state until READY. That is, each subchannel ignores consequent non-READY states after TRANSIENT_FAILURE.
---
 .../io/grpc/util/RoundRobinLoadBalancer.java  |  8 +++-
 .../grpc/util/RoundRobinLoadBalancerTest.java | 48 +++++++++++++++++--
 2 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java
index c8cb2084b3b..1ac1f7ca012 100644
--- a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java
+++ b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java
@@ -142,7 +142,13 @@ private void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo
     if (stateInfo.getState() == IDLE) {
       subchannel.requestConnection();
     }
-    getSubchannelStateInfoRef(subchannel).value = stateInfo;
+    Ref<ConnectivityStateInfo> subchannelStateRef = getSubchannelStateInfoRef(subchannel);
+    if (subchannelStateRef.value.getState().equals(TRANSIENT_FAILURE)) {
+      if (stateInfo.getState().equals(CONNECTING) || stateInfo.getState().equals(IDLE)) {
+        return;
+      }
+    }
+    subchannelStateRef.value = stateInfo;
     updateBalancingState();
   }
 
diff --git a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java
index 4346accf709..bab636f2c36 100644
--- a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java
+++ b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java
@@ -279,21 +279,61 @@ public void pickAfterStateChange() throws Exception {
     Status error = Status.UNKNOWN.withDescription("¯\\_(ツ)_//¯");
     deliverSubchannelState(subchannel,
         ConnectivityStateInfo.forTransientFailure(error));
-    assertThat(subchannelStateInfo.value).isEqualTo(
-        ConnectivityStateInfo.forTransientFailure(error));
+    assertThat(subchannelStateInfo.value.getState()).isEqualTo(TRANSIENT_FAILURE);
+    assertThat(subchannelStateInfo.value.getStatus()).isEqualTo(error);
     inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
     assertThat(pickerCaptor.getValue()).isInstanceOf(EmptyPicker.class);
 
     deliverSubchannelState(subchannel,
         ConnectivityStateInfo.forNonError(IDLE));
-    assertThat(subchannelStateInfo.value).isEqualTo(
-        ConnectivityStateInfo.forNonError(IDLE));
+    assertThat(subchannelStateInfo.value.getState()).isEqualTo(TRANSIENT_FAILURE);
+    assertThat(subchannelStateInfo.value.getStatus()).isEqualTo(error);
 
     verify(subchannel, times(2)).requestConnection();
     verify(mockHelper, times(3)).createSubchannel(any(CreateSubchannelArgs.class));
     verifyNoMoreInteractions(mockHelper);
   }
 
+  @Test
+  public void stayTransientFailureUntilReady() {
+    InOrder inOrder = inOrder(mockHelper);
+    loadBalancer.handleResolvedAddresses(
+        ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(Attributes.EMPTY)
+            .build());
+
+    inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), isA(EmptyPicker.class));
+
+    // Simulate state transitions for each subchannel individually.
+    for (Subchannel sc : loadBalancer.getSubchannels()) {
+      Status error = Status.UNKNOWN.withDescription("connection broken");
+      deliverSubchannelState(
+          sc,
+          ConnectivityStateInfo.forTransientFailure(error));
+      deliverSubchannelState(
+          sc,
+          ConnectivityStateInfo.forNonError(IDLE));
+      deliverSubchannelState(
+          sc,
+          ConnectivityStateInfo.forNonError(CONNECTING));
+      Ref<ConnectivityStateInfo> scStateInfo = sc.getAttributes().get(
+          STATE_INFO);
+      assertThat(scStateInfo.value.getState()).isEqualTo(TRANSIENT_FAILURE);
+      assertThat(scStateInfo.value.getStatus()).isEqualTo(error);
+    }
+    inOrder.verify(mockHelper).updateBalancingState(eq(TRANSIENT_FAILURE), isA(EmptyPicker.class));
+    inOrder.verifyNoMoreInteractions();
+
+    Subchannel subchannel = loadBalancer.getSubchannels().iterator().next();
+    deliverSubchannelState(subchannel, ConnectivityStateInfo.forNonError(READY));
+    Ref<ConnectivityStateInfo> subchannelStateInfo = subchannel.getAttributes().get(
+        STATE_INFO);
+    assertThat(subchannelStateInfo.value).isEqualTo(ConnectivityStateInfo.forNonError(READY));
+    inOrder.verify(mockHelper).updateBalancingState(eq(READY), isA(ReadyPicker.class));
+
+    verify(mockHelper, times(3)).createSubchannel(any(CreateSubchannelArgs.class));
+    verifyNoMoreInteractions(mockHelper);
+  }
+
   @Test
   public void pickerRoundRobin() throws Exception {
     Subchannel subchannel = mock(Subchannel.class);