From e389c56ea2ee55a0ef4bc813a1d1ed5130e51c86 Mon Sep 17 00:00:00 2001
From: Chengyuan Zhang <chengyuanzhang@google.com>
Date: Mon, 10 May 2021 12:12:45 -0700
Subject: [PATCH] core: throw away subchannel references after round_robin is
 shutdown (#8132) (#8155)

Triggering balancing state updates after already being shutdown can be confusing for the upstream of round_robin. In cases of the callers not managing round_robin's lifecycle (e.g., not ignoring updates after it shuts down round_robin, which it should), it can make problem very bad, especially with the behavior that round_robin is actually propagating TRANSIENT_FAILURE with a picker that buffers RPCs.

This change only polishes round_robin by always preserving its invariant. Callers/LBs should not rely on this and should still manage the balancing updates from its downstream correctly based on the downstream's lifetime.
---
 .../io/grpc/util/RoundRobinLoadBalancer.java  |  1 +
 .../grpc/util/RoundRobinLoadBalancerTest.java | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java
index 179755cbf8e..fe174d030af 100644
--- a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java
+++ b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java
@@ -163,6 +163,7 @@ public void shutdown() {
     for (Subchannel subchannel : getSubchannels()) {
       shutdownSubchannel(subchannel);
     }
+    subchannels.clear();
   }
 
   private static final Status EMPTY_OK = Status.OK.withDescription("no subchannels ready");
diff --git a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java
index e9f4ff9b439..8a9e573dd14 100644
--- a/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java
+++ b/core/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java
@@ -290,6 +290,25 @@ public void pickAfterStateChange() throws Exception {
     verifyNoMoreInteractions(mockHelper);
   }
 
+  @Test
+  public void ignoreShutdownSubchannelStateChange() {
+    InOrder inOrder = inOrder(mockHelper);
+    loadBalancer.handleResolvedAddresses(
+        ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(Attributes.EMPTY)
+            .build());
+    inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), isA(EmptyPicker.class));
+
+    loadBalancer.shutdown();
+    for (Subchannel sc : loadBalancer.getSubchannels()) {
+      verify(sc).shutdown();
+      // When the subchannel is being shut down, a SHUTDOWN connectivity state is delivered
+      // back to the subchannel state listener.
+      deliverSubchannelState(sc, ConnectivityStateInfo.forNonError(SHUTDOWN));
+    }
+
+    inOrder.verifyNoMoreInteractions();
+  }
+
   @Test
   public void stayTransientFailureUntilReady() {
     InOrder inOrder = inOrder(mockHelper);