core: Apply RetryingNameResolver in ManagedChannelImpl (#10371)

Wrapping the DnsNameResolver in DnsNameResolverProvider can cause problems to external name resolvers that delegate to a DnsResolver already wrapped in RetryingNameResolver. ManagedChannelImpl would end up wrapping these name resolvers again, causing an exception later from a RetryingNameResolver safeguard that checks for double wrapping.
grpc · Jul 12, 2023 · 78cf1c3 · 78cf1c3
1 parent 4fa2814
commit 78cf1c3
Show file tree

Hide file tree

Showing 5 changed files with 15 additions and 37 deletions.
diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java
@@ -56,19 +56,13 @@ public NameResolver newNameResolver(URI targetUri, NameResolver.Args args) {
       Preconditions.checkArgument(targetPath.startsWith("/"),
           "the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri);
       String name = targetPath.substring(1);
-      return new RetryingNameResolver(
-          new DnsNameResolver(
-              targetUri.getAuthority(),
-              name,
-              args,
-              GrpcUtil.SHARED_CHANNEL_EXECUTOR,
-              Stopwatch.createUnstarted(),
-              IS_ANDROID),
-          new BackoffPolicyRetryScheduler(
-              new ExponentialBackoffPolicy.Provider(),
-              args.getScheduledExecutorService(),
-              args.getSynchronizationContext()),
-          args.getSynchronizationContext());
+      return new DnsNameResolver(
+          targetUri.getAuthority(),
+          name,
+          args,
+          GrpcUtil.SHARED_CHANNEL_EXECUTOR,
+          Stopwatch.createUnstarted(),
+          IS_ANDROID);
     } else {
       return null;
     }

diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java
@@ -750,21 +750,14 @@ static NameResolver getNameResolver(
       NameResolver.Factory nameResolverFactory, NameResolver.Args nameResolverArgs) {
     NameResolver resolver = getNameResolver(target, nameResolverFactory, nameResolverArgs);
 
-    // 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
-    // removed from ManagedChannelImpl.
+    // We wrap the name resolver in a RetryingNameResolver to give it the ability to retry failures.
     // TODO: After a transition period, all NameResolver implementations that need retry should use
     //       RetryingNameResolver directly and this step can be removed.
-    NameResolver usedNameResolver;
-    if (resolver instanceof RetryingNameResolver) {
-      usedNameResolver = resolver;
-    } else {
-      usedNameResolver = new RetryingNameResolver(resolver,
+    NameResolver usedNameResolver = new RetryingNameResolver(resolver,
           new BackoffPolicyRetryScheduler(new ExponentialBackoffPolicy.Provider(),
               nameResolverArgs.getScheduledExecutorService(),
               nameResolverArgs.getSynchronizationContext()),
           nameResolverArgs.getSynchronizationContext());
-    }
 
     if (overrideAuthority == null) {
       return usedNameResolver;

diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java
@@ -61,9 +61,7 @@ public void isAvailable() {
   @Test
   public void newNameResolver() {
     assertSame(DnsNameResolver.class,
-        ((RetryingNameResolver) provider.newNameResolver(
-            URI.create("dns:///localhost:443"), args))
-            .getRetriedNameResolver().getClass());
+        provider.newNameResolver(URI.create("dns:///localhost:443"), args).getClass());
     assertNull(
         provider.newNameResolver(URI.create("notdns:///localhost:443"), args));
   }

diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java
@@ -1293,8 +1293,7 @@ private void testInvalidUri(URI uri) {
   }
 
   private void testValidUri(URI uri, String exportedAuthority, int expectedPort) {
-    DnsNameResolver resolver = (DnsNameResolver) ((RetryingNameResolver) provider.newNameResolver(
-        uri, args)).getRetriedNameResolver();
+    DnsNameResolver resolver = (DnsNameResolver) provider.newNameResolver(uri, args);
     assertNotNull(resolver);
     assertEquals(expectedPort, resolver.getPort());
     assertEquals(exportedAuthority, resolver.getServiceAuthority());

diff --git a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java
@@ -542,7 +542,7 @@ private static final class FakeNameResolverFactory extends NameResolver.Factory
     final URI expectedUri;
     final List<EquivalentAddressGroup> servers;
     final boolean resolvedAtStart;
-    final ArrayList<RetryingNameResolver> resolvers = new ArrayList<>();
+    final ArrayList<FakeNameResolver> resolvers = new ArrayList<>();
     final AtomicReference<Map<String, ?>> nextRawServiceConfig = new AtomicReference<>();
     final AtomicReference<Attributes> nextAttributes = new AtomicReference<>(Attributes.EMPTY);
 
@@ -561,13 +561,7 @@ public NameResolver newNameResolver(final URI targetUri, NameResolver.Args args)
         return null;
       }
       assertEquals(DEFAULT_PORT, args.getDefaultPort());
-      RetryingNameResolver resolver = new RetryingNameResolver(
-          new FakeNameResolver(args.getServiceConfigParser()),
-          new BackoffPolicyRetryScheduler(
-              new FakeBackoffPolicyProvider(),
-              args.getScheduledExecutorService(),
-              args.getSynchronizationContext()),
-          args.getSynchronizationContext());
+      FakeNameResolver resolver = new FakeNameResolver(args.getServiceConfigParser());
       resolvers.add(resolver);
       return resolver;
     }
@@ -578,8 +572,8 @@ public String getDefaultScheme() {
     }
 
     void allResolved() {
-      for (RetryingNameResolver resolver : resolvers) {
-        ((FakeNameResolver)resolver.getRetriedNameResolver()).resolved();
+      for (FakeNameResolver resolver : resolvers) {
+        resolver.resolved();
       }
     }