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.
grpc · Jun 29, 2023 · 8192ff6 · 8192ff6
1 parent 443a050
commit 8192ff6
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 15 deletions.
diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
@@ -749,9 +749,6 @@ static NameResolver getNameResolver(
       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 @@ static NameResolver getNameResolver(
           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
@@ -139,8 +139,9 @@ public String getDefaultScheme() {
 
   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
@@ -280,6 +280,11 @@ public String getPolicyName() {
       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 @@ channelBuilder, mockTransportFactory, new FakeBackoffPolicyProvider(),
         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 void channelTracing_nameResolvingErrorEvent() throws Exception {
     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 double nextDouble() {
     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 void hedgingScheduledThenChannelShutdown_hedgeShouldStillHappen_newCallSh
     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 =