commit 683eed671ee0581a820ad8d89533de858f0438bc
author Chengyuan Zhang <chengyuanzhang@google.com> Sat May 30 01:23:37 2020 +0000
committer GitHub <noreply@github.com> Fri May 29 18:23:37 2020 -0700
tree 0474e0e308aabbab002fb058d98cc812ae5347cd
parent 0201c5a9a7551c736d6f63476b145eca4f17c3b6

core: turn Channel into TF if resolver gives empty addresses with policy selection cannot handle it (#7073)

The original service config error handling design was unclear about the case when an updated resolution result with valid service config and empty address is returned to Channel, and the selected LB policy does not accept empty addresses. Existing implementation silently triggers resolver backoff after LB policy is changed, while leaving Channel being CONNECTING state, if there was an old service config resolved. This change converge the behaviors of whether the received service config is the first one or an update to a previous config, in both cases, Channel goes into TRANSIENT_FAILURE if the selected LB policy cannot handle empty addresses.

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
index 5b164c1..05dee80 100644
--- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
+++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
@@ -1369,7 +1369,6 @@
               "Resolved address: {0}, config={1}",
               servers,
               resolutionResult.getAttributes());
-          ResolutionState lastResolutionStateCopy = lastResolutionState;
 
           if (lastResolutionState != ResolutionState.SUCCESS) {
             channelLogger.log(ChannelLogLevel.INFO, "Address resolved: {0}", servers);
@@ -1457,14 +1456,7 @@
                     .build());
 
             if (!handleResult.isOk()) {
-              if (servers.isEmpty() && lastResolutionStateCopy == ResolutionState.SUCCESS) {
-                // lb doesn't expose that it needs address or not, because for some LB it is not
-                // deterministic. Assuming lb needs address if LB returns error when the address is
-                // empty and it is not the first resolution.
-                scheduleExponentialBackOffInSyncContext();
-              } else {
-                handleErrorInSyncContext(handleResult.augmentDescription(resolver + " was used"));
-              }
+              handleErrorInSyncContext(handleResult.augmentDescription(resolver + " was used"));
             }
           }
         }

core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java

diff --git a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java
index f81a4c5..7e61213 100644
--- a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java
+++ b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java
@@ -274,17 +274,19 @@
     assertThat(channel.getState(true)).isEqualTo(ConnectivityState.IDLE);
 
     reset(mockLoadBalancer);
-    Map<String, ?> ignoredServiceConfig =
-        parseJson("{\"loadBalancingConfig\": [{\"round_robin\": {}}]}");
-    nameResolverFactory.nextRawServiceConfig.set(ignoredServiceConfig);
     nameResolverFactory.servers.clear();
 
     // 2nd resolution
     nameResolverFactory.allResolved();
 
-    // 2nd service config without address should be ignored
+    // 2nd service config without addresses
+    ArgumentCaptor<Status> statusCaptor = ArgumentCaptor.forClass(Status.class);
     verify(mockLoadBalancer, never()).handleResolvedAddresses(any(ResolvedAddresses.class));
-    verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class));
+    verify(mockLoadBalancer).handleNameResolutionError(statusCaptor.capture());
+    assertThat(statusCaptor.getValue().getCode()).isEqualTo(Status.Code.UNAVAILABLE);
+    assertThat(statusCaptor.getValue().getDescription())
+        .contains("NameResolver returned no usable address.");
+    assertThat(channel.getState(true)).isEqualTo(ConnectivityState.TRANSIENT_FAILURE);
     assertWithMessage("Empty address should schedule NameResolver retry")
         .that(getNameResolverRefresh())
         .isNotNull();