core: throw away subchannel references after round_robin is shutdown (grpc#8132)

voidzcy · web-flow · commit 368c43aec4bb · 2021-04-30T15:42:44.000-07:00
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.
diff --git a/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java b/core/src/main/java/io/grpc/util/RoundRobinLoadBalancer.java
@@ -166,6 +166,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
@@ -292,6 +292,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);

Original file line number	Diff line number	Diff line change
`@@ -166,6 +166,7 @@ public void shutdown() {`
`166`	`166`	`for (Subchannel subchannel : getSubchannels()) {`
`167`	`167`	`shutdownSubchannel(subchannel);`
`168`	`168`	`}`
	`169`	`+ subchannels.clear();`
`169`	`170`	`}`
`170`	`171`
`171`	`172`	`private static final Status EMPTY_OK = Status.OK.withDescription("no subchannels ready");`