core: ManagedChannelImpl to always use RetryingNameResolver (#10328)

ManagedCahnnelImpl did not make sure to use a RetryingNameResolver if
authority was not overriden. This was not a problem for DNS name
resolution as the DNS name resolver factory explicitly returns a
RetryingNameResolver. For polling name resolvers that do not do this in
their factories (like the grpclb name resolver) this meant not having retry
at all.

3 files changed, 24 insertions, 15 deletions

diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
index 696a33885..d5d1f2419 100644
--- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
+++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
@@ -749,9 +749,6 @@ final class ManagedChannelImpl extends ManagedChannel implements
       String target, @Nullable final String overrideAuthority,
       NameResolver.Factory nameResolverFactory, NameResolver.Args nameResolverArgs) {
     NameResolver resolver = getNameResolver(target, nameResolverFactory, nameResolverArgs);
-    if (overrideAuthority == null) {
-      return resolver;
-    }
 
     // If the nameResolver is not already a RetryingNameResolver, then wrap it with it.
     // This helps guarantee that name resolution retry remains supported even as it has been
@@ -769,6 +766,10 @@ final class ManagedChannelImpl extends ManagedChannel implements
           nameResolverArgs.getSynchronizationContext());
     }
 
+    if (overrideAuthority == null) {
+      return usedNameResolver;
+    }
+
     return new ForwardingNameResolver(usedNameResolver) {
       @Override
       public String getServiceAuthority() {
diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java
index 4aa12973c..b63d53a6f 100644
--- a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java
+++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java
@@ -139,8 +139,9 @@ public class ManagedChannelImplGetNameResolverTest {
 
   private void testValidTarget(String target, String expectedUriString, URI expectedUri) {
     NameResolver.Factory nameResolverFactory = new FakeNameResolverFactory(expectedUri.getScheme());
-    FakeNameResolver nameResolver = (FakeNameResolver) ManagedChannelImpl.getNameResolver(
-        target, null, nameResolverFactory, NAMERESOLVER_ARGS);
+    FakeNameResolver nameResolver
+        = (FakeNameResolver) ((RetryingNameResolver) ManagedChannelImpl.getNameResolver(
+            target, null, nameResolverFactory, NAMERESOLVER_ARGS)).getRetriedNameResolver();
     assertNotNull(nameResolver);
     assertEquals(expectedUri, nameResolver.uri);
     assertEquals(expectedUriString, nameResolver.uri.toString());
diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java
index 055b648e1..5cb3ac6ef 100644
--- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java
+++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java
@@ -280,6 +280,11 @@ public class ManagedChannelImplTest {
       ArgumentCaptor.forClass(ClientStreamListener.class);
 
   private void createChannel(ClientInterceptor... interceptors) {
+    createChannel(false, interceptors);
+  }
+
+  private void createChannel(boolean nameResolutionExpectedToFail,
+      ClientInterceptor... interceptors) {
     checkState(channel == null);
 
     channel = new ManagedChannelImpl(
@@ -288,7 +293,7 @@ public class ManagedChannelImplTest {
         timer.getTimeProvider());
 
     if (requestConnection) {
-      int numExpectedTasks = 0;
+      int numExpectedTasks = nameResolutionExpectedToFail ? 1 : 0;
 
       // Force-exit the initial idle-mode
       channel.syncContext.execute(new Runnable() {
@@ -3000,7 +3005,7 @@ public class ManagedChannelImplTest {
     FakeNameResolverFactory nameResolverFactory =
         new FakeNameResolverFactory.Builder(expectedUri).setError(error).build();
     channelBuilder.nameResolverFactory(nameResolverFactory);
-    createChannel();
+    createChannel(true);
 
     assertThat(getStats(channel).channelTrace.events).contains(new ChannelTrace.Event.Builder()
         .setDescription("Failed to resolve name: " + error)
@@ -3503,10 +3508,11 @@ public class ManagedChannelImplTest {
     ArgumentCaptor<Helper> helperCaptor = ArgumentCaptor.forClass(Helper.class);
     verify(mockLoadBalancerProvider).newLoadBalancer(helperCaptor.capture());
     helper = helperCaptor.getValue();
-    verify(mockLoadBalancer).acceptResolvedAddresses(
-        ResolvedAddresses.newBuilder()
-            .setAddresses(nameResolverFactory.servers)
-            .build());
+    verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture());
+    ResolvedAddresses resolvedAddresses = resolvedAddressCaptor.getValue();
+    assertThat(resolvedAddresses.getAddresses()).isEqualTo(nameResolverFactory.servers);
+    assertThat(resolvedAddresses.getAttributes()
+        .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY)).isNotNull();
 
     // simulating request connection and then transport ready after resolved address
     Subchannel subchannel =
@@ -3609,10 +3615,11 @@ public class ManagedChannelImplTest {
     ArgumentCaptor<Helper> helperCaptor = ArgumentCaptor.forClass(Helper.class);
     verify(mockLoadBalancerProvider).newLoadBalancer(helperCaptor.capture());
     helper = helperCaptor.getValue();
-    verify(mockLoadBalancer).acceptResolvedAddresses(
-        ResolvedAddresses.newBuilder()
-            .setAddresses(nameResolverFactory.servers)
-            .build());
+    verify(mockLoadBalancer).acceptResolvedAddresses(resolvedAddressCaptor.capture());
+    ResolvedAddresses resolvedAddresses = resolvedAddressCaptor.getValue();
+    assertThat(resolvedAddresses.getAddresses()).isEqualTo(nameResolverFactory.servers);
+    assertThat(resolvedAddresses.getAttributes()
+        .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY)).isNotNull();
 
     // simulating request connection and then transport ready after resolved address
     Subchannel subchannel =