From ad6f07ccc2bc99eb8b4b81ef0777e31c306cb08e Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Thu, 29 Jun 2023 13:47:23 -0700 Subject: [PATCH] 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. --- .../io/grpc/internal/ManagedChannelImpl.java | 7 ++--- ...ManagedChannelImplGetNameResolverTest.java | 5 ++-- .../grpc/internal/ManagedChannelImplTest.java | 27 ++++++++++++------- 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 696a3388556..d5d1f2419b6 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 @@ 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 index 4aa12973cef..b63d53a6f83 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 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 index 055b648e106..5cb3ac6efae 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 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 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 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 =