From 78cf1c39cfdec8850fd215f33a9154e50dcae05c Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Wed, 12 Jul 2023 10:14:50 -0700 Subject: [PATCH] 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. --- .../internal/DnsNameResolverProvider.java | 20 +++++++------------ .../io/grpc/internal/ManagedChannelImpl.java | 11 ++-------- .../internal/DnsNameResolverProviderTest.java | 4 +--- .../io/grpc/internal/DnsNameResolverTest.java | 3 +-- .../ServiceConfigErrorHandlingTest.java | 14 ++++--------- 5 files changed, 15 insertions(+), 37 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java index cfe65afc1be..414a0ae88fe 100644 --- 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 index d5d1f2419b6..96be3eb37e7 100644 --- 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 index b07d7131c5e..aff10ce9337 100644 --- 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 index 33b127a46bc..87f704e1267 100644 --- 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 index da3ed7c508e..01deda15628 100644 --- 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 servers; final boolean resolvedAtStart; - final ArrayList resolvers = new ArrayList<>(); + final ArrayList resolvers = new ArrayList<>(); final AtomicReference> nextRawServiceConfig = new AtomicReference<>(); final AtomicReference 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(); } }