diff options
author | Terry Wilson <tmwilson@google.com> | 2023-07-12 10:14:50 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-07-12 10:14:50 -0700 |
commit | 78cf1c39cfdec8850fd215f33a9154e50dcae05c (patch) | |
tree | a70eae997bf059a8a4174d8eb966381b6c535713 | |
parent | 4fa2814d65b2536aede30e1f24c461a2f42be1f7 (diff) | |
download | grpc-grpc-java-78cf1c39cfdec8850fd215f33a9154e50dcae05c.tar.gz |
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.
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 cfe65afc1..414a0ae88 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 final class DnsNameResolverProvider extends NameResolverProvider { 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 d5d1f2419..96be3eb37 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 @@ final class ManagedChannelImpl extends ManagedChannel implements 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 b07d7131c..aff10ce93 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 class DnsNameResolverProviderTest { @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 33b127a46..87f704e12 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 @@ public class DnsNameResolverTest { } 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 da3ed7c50..01deda156 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 @@ public class ServiceConfigErrorHandlingTest { 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 class ServiceConfigErrorHandlingTest { 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 class ServiceConfigErrorHandlingTest { } void allResolved() { - for (RetryingNameResolver resolver : resolvers) { - ((FakeNameResolver)resolver.getRetriedNameResolver()).resolved(); + for (FakeNameResolver resolver : resolvers) { + resolver.resolved(); } } |