core/util: Go to TRANSIENT_FAILURE on initial resolution error for Gr…

…acefulSwitchLoadBalancer (grpc#6598)

Go to TRANSIENT_FAILURE immediately instead of NOOP if GracefulSwitchLoadBalancer receives resolution error before switching to any delegate.

In most natural usecase, the `gracefulSwitchLb` is a child balancer of some `parentLb`, and the `gracefulSwitchLb` switches to a new `delegateLb` when `parentLb.handleResolvedAddressGroup()`. If the  `parentLb` receives a resolution error before receiving any resolved addresses, it should go to  TRANSIENT_FAILURE. In this case, it will be more convenient if the initial  `gracefulSwitchLb` can go to TRANSIENT_FAILURE directly.

core/src/main/java/io/grpc/util/GracefulSwitchLoadBalancer.java

@@ -32,13 +32,39 @@
  * A load balancer that gracefully swaps to a new lb policy. If the channel is currently in a state
  * other than READY, the new policy will be swapped into place immediately.  Otherwise, the channel
  * will keep using the old policy until the new policy reports READY or the old policy exits READY.
+ *
+ * <p>The balancer must {@link #switchTo(Factory) switch to} a policy prior to {@link
+ * LoadBalancer#handleResolvedAddresses(ResolvedAddresses) handling resolved addresses} for the
+ * first time.
  */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/5999")
 @NotThreadSafe // Must be accessed in SynchronizationContext
 public final class GracefulSwitchLoadBalancer extends ForwardingLoadBalancer {
-  private static final LoadBalancer NOOP_BALANCER = new LoadBalancer() {
+  private final LoadBalancer defaultBalancer = new LoadBalancer() {
     @Override
-    public void handleNameResolutionError(Status error) {}
+    public void handleResolvedAddresses(ResolvedAddresses resolvedAddresses) {
+      //  Most LB policies using this class will receive child policy configuration within the
+      //  service config, so they are naturally calling switchTo() just before
+      //  handleResolvedAddresses(), within their own handleResolvedAddresses(). If switchTo() is
+      //  not called immediately after construction that does open up potential for bugs in the
+      //  parent policies, where they fail to call switchTo(). So we will use the exception to try
+      //  to notice those bugs quickly, as it will fail very loudly.
+      throw new IllegalStateException(
+          "GracefulSwitchLoadBalancer must switch to a load balancing policy before handling"
+              + " ResolvedAddresses");
+    }
+
+    @Override
+    public void handleNameResolutionError(final Status error) {
+      helper.updateBalancingState(
+          ConnectivityState.TRANSIENT_FAILURE,
+          new SubchannelPicker() {
+            @Override
+            public PickResult pickSubchannel(PickSubchannelArgs args) {
+              return PickResult.withError(error);
+            }
+          });
+    }
 
     @Override
     public void shutdown() {}
@@ -64,9 +90,9 @@ public String toString() {
   // The current fields are guaranteed to be set after the initial swapTo().
   // The pending fields are cleared when it becomes current.
   @Nullable private LoadBalancer.Factory currentBalancerFactory;
-  private LoadBalancer currentLb = NOOP_BALANCER;
+  private LoadBalancer currentLb = defaultBalancer;
   @Nullable private LoadBalancer.Factory pendingBalancerFactory;
-  private LoadBalancer pendingLb = NOOP_BALANCER;
+  private LoadBalancer pendingLb = defaultBalancer;
   private ConnectivityState pendingState;
   private SubchannelPicker pendingPicker;
 
@@ -87,7 +113,7 @@ public void switchTo(LoadBalancer.Factory newBalancerFactory) {
       return;
     }
     pendingLb.shutdown();
-    pendingLb = NOOP_BALANCER;
+    pendingLb = defaultBalancer;
     pendingBalancerFactory = null;
     pendingState = ConnectivityState.CONNECTING;
     pendingPicker = BUFFER_PICKER;
@@ -115,7 +141,7 @@ public void updateBalancingState(ConnectivityState newState, SubchannelPicker ne
           }
         } else if (lb == currentLb) {
           currentLbIsReady = newState == ConnectivityState.READY;
-          if (!currentLbIsReady && pendingLb != NOOP_BALANCER) {
+          if (!currentLbIsReady && pendingLb != defaultBalancer) {
             swap(); // current policy exits READY, so swap
           } else {
             helper.updateBalancingState(newState, newPicker);
@@ -138,13 +164,13 @@ private void swap() {
     currentLb.shutdown();
     currentLb = pendingLb;
     currentBalancerFactory = pendingBalancerFactory;
-    pendingLb = NOOP_BALANCER;
+    pendingLb = defaultBalancer;
     pendingBalancerFactory = null;
   }
 
   @Override
   protected LoadBalancer delegate() {
-    return pendingLb == NOOP_BALANCER ? currentLb : pendingLb;
+    return pendingLb == defaultBalancer ? currentLb : pendingLb;
   }
 
   @Override

core/src/test/java/io/grpc/util/GracefulSwitchLoadBalancerTest.java

@@ -19,7 +19,9 @@
 import static com.google.common.truth.Truth.assertThat;
 import static io.grpc.ConnectivityState.CONNECTING;
 import static io.grpc.ConnectivityState.READY;
+import static io.grpc.ConnectivityState.TRANSIENT_FAILURE;
 import static io.grpc.util.GracefulSwitchLoadBalancer.BUFFER_PICKER;
+import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
@@ -33,6 +35,7 @@
 import io.grpc.LoadBalancer;
 import io.grpc.LoadBalancer.CreateSubchannelArgs;
 import io.grpc.LoadBalancer.Helper;
+import io.grpc.LoadBalancer.PickSubchannelArgs;
 import io.grpc.LoadBalancer.ResolvedAddresses;
 import io.grpc.LoadBalancer.Subchannel;
 import io.grpc.LoadBalancer.SubchannelPicker;
@@ -51,6 +54,7 @@
 import org.junit.rules.ExpectedException;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
+import org.mockito.ArgumentCaptor;
 import org.mockito.InOrder;
 
 /**
@@ -471,6 +475,16 @@ public int hashCode() {
     verifyNoMoreInteractions(lb0, lb1);
   }
 
+  @Test
+  public void transientFailureOnInitialResolutionError() {
+    gracefulSwitchLb.handleNameResolutionError(Status.DATA_LOSS);
+    ArgumentCaptor<SubchannelPicker> pickerCaptor = ArgumentCaptor.forClass(null);
+    verify(mockHelper).updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture());
+    SubchannelPicker picker = pickerCaptor.getValue();
+    assertThat(picker.pickSubchannel(mock(PickSubchannelArgs.class)).getStatus().getCode())
+        .isEqualTo(Status.Code.DATA_LOSS);
+  }
+
   @Deprecated
   @Test
   public void handleSubchannelState_shouldThrow() {