From a650c6f4147890de46d7b7a260ddb5c425a0c599 Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Thu, 29 Jun 2023 14:18:06 -0700 Subject: [PATCH] core: ManagedChannelImpl to always use RetryingNameResolver (#10328) (#10330) 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 4ab29d25734..58f2b32c8ac 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -748,9 +748,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 @@ -768,6 +765,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 3491eab2e65..edf3ae3a2bd 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -279,6 +279,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( @@ -287,7 +292,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() { @@ -2955,7 +2960,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) @@ -3458,10 +3463,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 = @@ -3564,10 +3570,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 =