From fcb5c54e4b82d354f42ced0121928fabce9ef53f Mon Sep 17 00:00:00 2001 From: Terry Wilson Date: Fri, 3 Feb 2023 18:23:32 -0800 Subject: [PATCH] Move name resolution retry from managed channel to name resolver (take #2) (#9812) This change has these main aspects to it: 1. Removal of any name resolution responsibility from ManagedChannelImpl 2. Creation of a new RetryScheduler to own generic retry logic - Can also be used outside the name resolution context 3. Creation of a new RetryingNameScheduler that can be used to wrap any polling name resolver to add retry capability 4. A new facility in NameResolver to allow implementations to notify listeners on the success of name resolution attempts - RetryingNameScheduler relies on this --- .../internal/BackoffPolicyRetryScheduler.java | 85 ++++++ .../internal/DnsNameResolverProvider.java | 22 +- .../io/grpc/internal/ManagedChannelImpl.java | 103 +++---- .../java/io/grpc/internal/RetryScheduler.java | 36 +++ .../grpc/internal/RetryingNameResolver.java | 125 ++++++++ .../BackoffPolicyRetrySchedulerTest.java | 109 +++++++ .../internal/DnsNameResolverProviderTest.java | 7 +- .../io/grpc/internal/DnsNameResolverTest.java | 267 ++++++++++++------ .../internal/ForwardingNameResolverTest.java | 1 - ...ManagedChannelImplGetNameResolverTest.java | 1 + .../grpc/internal/ManagedChannelImplTest.java | 196 ------------- .../internal/RetryingNameResolverTest.java | 142 ++++++++++ .../ServiceConfigErrorHandlingTest.java | 19 +- 13 files changed, 753 insertions(+), 360 deletions(-) create mode 100644 core/src/main/java/io/grpc/internal/BackoffPolicyRetryScheduler.java create mode 100644 core/src/main/java/io/grpc/internal/RetryScheduler.java create mode 100644 core/src/main/java/io/grpc/internal/RetryingNameResolver.java create mode 100644 core/src/test/java/io/grpc/internal/BackoffPolicyRetrySchedulerTest.java create mode 100644 core/src/test/java/io/grpc/internal/RetryingNameResolverTest.java diff --git a/core/src/main/java/io/grpc/internal/BackoffPolicyRetryScheduler.java b/core/src/main/java/io/grpc/internal/BackoffPolicyRetryScheduler.java new file mode 100644 index 00000000000..e5a66205364 --- /dev/null +++ b/core/src/main/java/io/grpc/internal/BackoffPolicyRetryScheduler.java @@ -0,0 +1,85 @@ +/* + * Copyright 2023 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.internal; + +import io.grpc.SynchronizationContext; +import io.grpc.SynchronizationContext.ScheduledHandle; +import java.util.concurrent.ScheduledExecutorService; +import java.util.concurrent.TimeUnit; +import java.util.logging.Level; +import java.util.logging.Logger; + +/** + * Schedules a retry operation according to a {@link BackoffPolicy}. The retry is run within a + * {@link SynchronizationContext}. At most one retry is scheduled at a time. + */ +final class BackoffPolicyRetryScheduler implements RetryScheduler { + private final ScheduledExecutorService scheduledExecutorService; + private final SynchronizationContext syncContext; + private final BackoffPolicy.Provider policyProvider; + + private BackoffPolicy policy; + private ScheduledHandle scheduledHandle; + + private static final Logger logger = Logger.getLogger( + BackoffPolicyRetryScheduler.class.getName()); + + BackoffPolicyRetryScheduler(BackoffPolicy.Provider policyProvider, + ScheduledExecutorService scheduledExecutorService, + SynchronizationContext syncContext) { + this.policyProvider = policyProvider; + this.scheduledExecutorService = scheduledExecutorService; + this.syncContext = syncContext; + } + + /** + * Schedules a future retry operation. Only allows one retry to be scheduled at any given time. + */ + @Override + public void schedule(Runnable retryOperation) { + syncContext.throwIfNotInThisSynchronizationContext(); + + if (policy == null) { + policy = policyProvider.get(); + } + // If a retry is already scheduled, take no further action. + if (scheduledHandle != null && scheduledHandle.isPending()) { + return; + } + long delayNanos = policy.nextBackoffNanos(); + scheduledHandle = syncContext.schedule(retryOperation, delayNanos, TimeUnit.NANOSECONDS, + scheduledExecutorService); + logger.log(Level.FINE, "Scheduling DNS resolution backoff for {0}ns", delayNanos); + } + + /** + * Resets the {@link BackoffPolicyRetryScheduler} and cancels any pending retry task. The policy + * will be cleared thus also resetting any state associated with it (e.g. a backoff multiplier). + */ + @Override + public void reset() { + syncContext.throwIfNotInThisSynchronizationContext(); + + syncContext.execute(() -> { + if (scheduledHandle != null && scheduledHandle.isPending()) { + scheduledHandle.cancel(); + } + policy = null; + }); + } + +} diff --git a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java index 8078aa0d4c9..da0410f3e55 100644 --- a/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java +++ b/core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java @@ -47,19 +47,25 @@ public final class DnsNameResolverProvider extends NameResolverProvider { private static final String SCHEME = "dns"; @Override - public DnsNameResolver newNameResolver(URI targetUri, NameResolver.Args args) { + public NameResolver newNameResolver(URI targetUri, NameResolver.Args args) { if (SCHEME.equals(targetUri.getScheme())) { String targetPath = Preconditions.checkNotNull(targetUri.getPath(), "targetPath"); Preconditions.checkArgument(targetPath.startsWith("/"), "the path component (%s) of the target (%s) must start with '/'", targetPath, targetUri); String name = targetPath.substring(1); - return new DnsNameResolver( - targetUri.getAuthority(), - name, - args, - GrpcUtil.SHARED_CHANNEL_EXECUTOR, - Stopwatch.createUnstarted(), - InternalServiceProviders.isAndroid(getClass().getClassLoader())); + return new RetryingNameResolver( + new DnsNameResolver( + targetUri.getAuthority(), + name, + args, + GrpcUtil.SHARED_CHANNEL_EXECUTOR, + Stopwatch.createUnstarted(), + InternalServiceProviders.isAndroid(getClass().getClassLoader())), + new BackoffPolicyRetryScheduler( + new ExponentialBackoffPolicy.Provider(), + args.getScheduledExecutorService(), + args.getSynchronizationContext()), + args.getSynchronizationContext()); } 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 2606083db94..4ab29d25734 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -85,6 +85,7 @@ import io.grpc.internal.ManagedChannelServiceConfig.ServiceConfigConvertedSelector; import io.grpc.internal.RetriableStream.ChannelBufferMeter; import io.grpc.internal.RetriableStream.Throttle; +import io.grpc.internal.RetryingNameResolver.ResolutionResultListener; import java.net.URI; import java.net.URISyntaxException; import java.util.ArrayList; @@ -367,7 +368,6 @@ private void shutdownNameResolverAndLoadBalancer(boolean channelIsActive) { checkState(lbHelper != null, "lbHelper is null"); } if (nameResolver != null) { - cancelNameResolverBackoff(); nameResolver.shutdown(); nameResolverStarted = false; if (channelIsActive) { @@ -450,42 +450,10 @@ private void rescheduleIdleTimer() { idleTimer.reschedule(idleTimeoutMillis, TimeUnit.MILLISECONDS); } - // Run from syncContext - @VisibleForTesting - class DelayedNameResolverRefresh implements Runnable { - @Override - public void run() { - scheduledNameResolverRefresh = null; - refreshNameResolution(); - } - } - - // Must be used from syncContext - @Nullable private ScheduledHandle scheduledNameResolverRefresh; - // The policy to control backoff between name resolution attempts. Non-null when an attempt is - // scheduled. Must be used from syncContext - @Nullable private BackoffPolicy nameResolverBackoffPolicy; - - // Must be run from syncContext - private void cancelNameResolverBackoff() { - syncContext.throwIfNotInThisSynchronizationContext(); - if (scheduledNameResolverRefresh != null) { - scheduledNameResolverRefresh.cancel(); - scheduledNameResolverRefresh = null; - nameResolverBackoffPolicy = null; - } - } - /** - * Force name resolution refresh to happen immediately and reset refresh back-off. Must be run + * Force name resolution refresh to happen immediately. Must be run * from syncContext. */ - private void refreshAndResetNameResolution() { - syncContext.throwIfNotInThisSynchronizationContext(); - cancelNameResolverBackoff(); - refreshNameResolution(); - } - private void refreshNameResolution() { syncContext.throwIfNotInThisSynchronizationContext(); if (nameResolverStarted) { @@ -783,7 +751,24 @@ static NameResolver getNameResolver( if (overrideAuthority == null) { return resolver; } - return new ForwardingNameResolver(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 + // removed from ManagedChannelImpl. + // 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, + new BackoffPolicyRetryScheduler(new ExponentialBackoffPolicy.Provider(), + nameResolverArgs.getScheduledExecutorService(), + nameResolverArgs.getSynchronizationContext()), + nameResolverArgs.getSynchronizationContext()); + } + + return new ForwardingNameResolver(usedNameResolver) { @Override public String getServiceAuthority() { return overrideAuthority; @@ -1291,7 +1276,7 @@ private void maybeTerminateChannel() { // Must be called from syncContext private void handleInternalSubchannelState(ConnectivityStateInfo newState) { if (newState.getState() == TRANSIENT_FAILURE || newState.getState() == IDLE) { - refreshAndResetNameResolution(); + refreshNameResolution(); } } @@ -1338,9 +1323,8 @@ public void run() { if (shutdown.get()) { return; } - if (scheduledNameResolverRefresh != null && scheduledNameResolverRefresh.isPending()) { - checkState(nameResolverStarted, "name resolver must be started"); - refreshAndResetNameResolution(); + if (nameResolverStarted) { + refreshNameResolution(); } for (InternalSubchannel subchannel : subchannels) { subchannel.resetConnectBackoff(); @@ -1496,7 +1480,7 @@ public void refreshNameResolution() { final class LoadBalancerRefreshNameResolution implements Runnable { @Override public void run() { - refreshAndResetNameResolution(); + ManagedChannelImpl.this.refreshNameResolution(); } } @@ -1727,7 +1711,7 @@ public ChannelCredentials withoutBearerTokens() { } } - private final class NameResolverListener extends NameResolver.Listener2 { + final class NameResolverListener extends NameResolver.Listener2 { final LbHelperImpl helper; final NameResolver resolver; @@ -1759,8 +1743,9 @@ public void run() { lastResolutionState = ResolutionState.SUCCESS; } - nameResolverBackoffPolicy = null; ConfigOrError configOrError = resolutionResult.getServiceConfig(); + ResolutionResultListener resolutionResultListener = resolutionResult.getAttributes() + .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY); InternalConfigSelector resolvedConfigSelector = resolutionResult.getAttributes().get(InternalConfigSelector.KEY); ManagedChannelServiceConfig validServiceConfig = @@ -1817,6 +1802,9 @@ public void run() { // we later check for these error codes when investigating pick results in // GrpcUtil.getTransportFromPickResult(). onError(configOrError.getError()); + if (resolutionResultListener != null) { + resolutionResultListener.resolutionAttempted(false); + } return; } else { effectiveServiceConfig = lastServiceConfig; @@ -1861,15 +1849,15 @@ public void run() { } Attributes attributes = attrBuilder.build(); - boolean addressesAccepted = helper.lb.tryAcceptResolvedAddresses( + boolean lastAddressesAccepted = helper.lb.tryAcceptResolvedAddresses( ResolvedAddresses.newBuilder() .setAddresses(servers) .setAttributes(attributes) .setLoadBalancingPolicyConfig(effectiveServiceConfig.getLoadBalancingConfig()) .build()); - - if (!addressesAccepted) { - scheduleExponentialBackOffInSyncContext(); + // If a listener is provided, let it know if the addresses were accepted. + if (resolutionResultListener != null) { + resolutionResultListener.resolutionAttempted(lastAddressesAccepted); } } } @@ -1905,29 +1893,6 @@ private void handleErrorInSyncContext(Status error) { } helper.lb.handleNameResolutionError(error); - - scheduleExponentialBackOffInSyncContext(); - } - - private void scheduleExponentialBackOffInSyncContext() { - if (scheduledNameResolverRefresh != null && scheduledNameResolverRefresh.isPending()) { - // The name resolver may invoke onError multiple times, but we only want to - // schedule one backoff attempt - // TODO(ericgribkoff) Update contract of NameResolver.Listener or decide if we - // want to reset the backoff interval upon repeated onError() calls - return; - } - if (nameResolverBackoffPolicy == null) { - nameResolverBackoffPolicy = backoffPolicyProvider.get(); - } - long delayNanos = nameResolverBackoffPolicy.nextBackoffNanos(); - channelLogger.log( - ChannelLogLevel.DEBUG, - "Scheduling DNS resolution backoff for {0} ns", delayNanos); - scheduledNameResolverRefresh = - syncContext.schedule( - new DelayedNameResolverRefresh(), delayNanos, TimeUnit.NANOSECONDS, - transportFactory .getScheduledExecutorService()); } } diff --git a/core/src/main/java/io/grpc/internal/RetryScheduler.java b/core/src/main/java/io/grpc/internal/RetryScheduler.java new file mode 100644 index 00000000000..d2877e3ac0a --- /dev/null +++ b/core/src/main/java/io/grpc/internal/RetryScheduler.java @@ -0,0 +1,36 @@ +/* + * Copyright 2023 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.internal; + +/** + * This interface is used to schedule future retry attempts for a failed operation. The retry delay + * and the number of attempt is defined by implementing classes. Implementations should assure + * that only one future retry operation is ever scheduled at a time. + */ +public interface RetryScheduler { + + /** + * A request to schedule a future retry (or retries) for a failed operation. Noop if an operation + * has already been scheduled. + */ + void schedule(Runnable retryOperation); + + /** + * Resets the scheduler, effectively cancelling any future retry operation. + */ + void reset(); +} diff --git a/core/src/main/java/io/grpc/internal/RetryingNameResolver.java b/core/src/main/java/io/grpc/internal/RetryingNameResolver.java new file mode 100644 index 00000000000..e723ce4f7f0 --- /dev/null +++ b/core/src/main/java/io/grpc/internal/RetryingNameResolver.java @@ -0,0 +1,125 @@ +/* + * Copyright 2023 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.internal; + +import com.google.common.annotations.VisibleForTesting; +import io.grpc.Attributes; +import io.grpc.NameResolver; +import io.grpc.Status; +import io.grpc.SynchronizationContext; + +/** + * This wrapper class can add retry capability to any polling {@link NameResolver} implementation + * that supports calling {@link ResolutionResultListener}s with the outcome of each resolution. + * + *

The {@link NameResolver} used with this + */ +final class RetryingNameResolver extends ForwardingNameResolver { + + private final NameResolver retriedNameResolver; + private final RetryScheduler retryScheduler; + private final SynchronizationContext syncContext; + + static final Attributes.Key RESOLUTION_RESULT_LISTENER_KEY + = Attributes.Key.create( + "io.grpc.internal.RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY"); + + /** + * Creates a new {@link RetryingNameResolver}. + * + * @param retriedNameResolver A {@link NameResolver} that will have failed attempt retried. + * @param retryScheduler Used to schedule the retry attempts. + */ + RetryingNameResolver(NameResolver retriedNameResolver, RetryScheduler retryScheduler, + SynchronizationContext syncContext) { + super(retriedNameResolver); + this.retriedNameResolver = retriedNameResolver; + this.retryScheduler = retryScheduler; + this.syncContext = syncContext; + } + + @Override + public void start(Listener2 listener) { + super.start(new RetryingListener(listener)); + } + + @Override + public void shutdown() { + super.shutdown(); + retryScheduler.reset(); + } + + /** + * Used to get the underlying {@link NameResolver} that is getting its failed attempts retried. + */ + @VisibleForTesting + NameResolver getRetriedNameResolver() { + return retriedNameResolver; + } + + @VisibleForTesting + class DelayedNameResolverRefresh implements Runnable { + @Override + public void run() { + refresh(); + } + } + + private class RetryingListener extends Listener2 { + private Listener2 delegateListener; + + RetryingListener(Listener2 delegateListener) { + this.delegateListener = delegateListener; + } + + @Override + public void onResult(ResolutionResult resolutionResult) { + // If the resolution result listener is already an attribute it indicates that a name resolver + // has already been wrapped with this class. This indicates a misconfiguration. + if (resolutionResult.getAttributes().get(RESOLUTION_RESULT_LISTENER_KEY) != null) { + throw new IllegalStateException( + "RetryingNameResolver can only be used once to wrap a NameResolver"); + } + + delegateListener.onResult(resolutionResult.toBuilder().setAttributes( + resolutionResult.getAttributes().toBuilder() + .set(RESOLUTION_RESULT_LISTENER_KEY, new ResolutionResultListener()).build()) + .build()); + } + + @Override + public void onError(Status error) { + delegateListener.onError(error); + syncContext.execute(() -> retryScheduler.schedule(new DelayedNameResolverRefresh())); + } + } + + /** + * Simple callback class to store in {@link ResolutionResult} attributes so that + * ManagedChannel can indicate if the resolved addresses were accepted. Temporary until + * the Listener2.onResult() API can be changed to return a boolean for this purpose. + */ + class ResolutionResultListener { + public void resolutionAttempted(boolean successful) { + if (successful) { + retryScheduler.reset(); + } else { + retryScheduler.schedule(new DelayedNameResolverRefresh()); + } + } + } +} diff --git a/core/src/test/java/io/grpc/internal/BackoffPolicyRetrySchedulerTest.java b/core/src/test/java/io/grpc/internal/BackoffPolicyRetrySchedulerTest.java new file mode 100644 index 00000000000..0ec7a218087 --- /dev/null +++ b/core/src/test/java/io/grpc/internal/BackoffPolicyRetrySchedulerTest.java @@ -0,0 +1,109 @@ +/* + * Copyright 2023 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.internal; + +import static com.google.common.truth.Truth.assertThat; +import static org.mockito.Mockito.mock; + +import io.grpc.SynchronizationContext; +import java.lang.Thread.UncaughtExceptionHandler; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import org.junit.Before; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; + +/** + * Unit tests for {@link BackoffPolicyRetryScheduler}. + */ +@RunWith(JUnit4.class) +public class BackoffPolicyRetrySchedulerTest { + + private final FakeClock fakeClock = new FakeClock(); + + private BackoffPolicyRetryScheduler scheduler; + private final SynchronizationContext syncContext = new SynchronizationContext( + mock(UncaughtExceptionHandler.class)); + + @Before + public void setup() { + scheduler = new BackoffPolicyRetryScheduler(new FakeBackoffPolicyProvider(), + fakeClock.getScheduledExecutorService(), syncContext); + } + + @Test + public void schedule() { + AtomicInteger retryCount = new AtomicInteger(); + Runnable retry = retryCount::incrementAndGet; + syncContext.execute(() -> scheduler.schedule(retry)); + + fakeClock.forwardTime(2, TimeUnit.NANOSECONDS); + + assertThat(retryCount.get()).isEqualTo(1); + } + + @Test + public void schedule_noMultiple() { + AtomicInteger retryCount = new AtomicInteger(); + Runnable retry = retryCount::incrementAndGet; + + // We schedule multiple retries... + syncContext.execute(() -> scheduler.schedule(retry)); + syncContext.execute(() -> scheduler.schedule(retry)); + + fakeClock.forwardTime(2, TimeUnit.NANOSECONDS); + + // But only one of them should have run. + assertThat(retryCount.get()).isEqualTo(1); + } + + @Test + public void reset() { + AtomicInteger retryCount = new AtomicInteger(); + Runnable retry = retryCount::incrementAndGet; + Runnable retryTwo = () -> { + retryCount.getAndAdd(2); + }; + + // We schedule one retry. + syncContext.execute(() -> scheduler.schedule(retry)); + + // But then reset. + syncContext.execute(() -> scheduler.reset()); + + // And schedule a different retry. + syncContext.execute(() -> scheduler.schedule(retryTwo)); + + fakeClock.forwardTime(2, TimeUnit.NANOSECONDS); + + // The retry after the reset should have been run. + assertThat(retryCount.get()).isEqualTo(2); + } + + private static class FakeBackoffPolicyProvider implements BackoffPolicy.Provider { + @Override + public BackoffPolicy get() { + return new BackoffPolicy() { + @Override + public long nextBackoffNanos() { + return 1; + } + }; + } + } +} \ No newline at end of file diff --git a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java index 5d127b72d10..b07d7131c5e 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverProviderTest.java @@ -33,6 +33,8 @@ /** Unit tests for {@link DnsNameResolverProvider}. */ @RunWith(JUnit4.class) public class DnsNameResolverProviderTest { + private final FakeClock fakeClock = new FakeClock(); + private final SynchronizationContext syncContext = new SynchronizationContext( new Thread.UncaughtExceptionHandler() { @Override @@ -46,6 +48,7 @@ public void uncaughtException(Thread t, Throwable e) { .setSynchronizationContext(syncContext) .setServiceConfigParser(mock(ServiceConfigParser.class)) .setChannelLogger(mock(ChannelLogger.class)) + .setScheduledExecutorService(fakeClock.getScheduledExecutorService()) .build(); private DnsNameResolverProvider provider = new DnsNameResolverProvider(); @@ -58,7 +61,9 @@ public void isAvailable() { @Test public void newNameResolver() { assertSame(DnsNameResolver.class, - provider.newNameResolver(URI.create("dns:///localhost:443"), args).getClass()); + ((RetryingNameResolver) provider.newNameResolver( + URI.create("dns:///localhost:443"), args)) + .getRetriedNameResolver().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 c7c00994cfd..33b127a46bc 100644 --- a/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/DnsNameResolverTest.java @@ -25,6 +25,8 @@ import static org.junit.Assert.assertTrue; import static org.junit.Assert.fail; import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; import static org.mockito.Mockito.times; @@ -37,7 +39,6 @@ import com.google.common.collect.Iterables; import com.google.common.net.InetAddresses; import com.google.common.testing.FakeTicker; -import io.grpc.Attributes; import io.grpc.ChannelLogger; import io.grpc.EquivalentAddressGroup; import io.grpc.HttpConnectProxiedSocketAddress; @@ -111,17 +112,18 @@ public void uncaughtException(Thread t, Throwable e) { throw new AssertionError(e); } }); - private final NameResolver.Args args = NameResolver.Args.newBuilder() - .setDefaultPort(DEFAULT_PORT) - .setProxyDetector(GrpcUtil.DEFAULT_PROXY_DETECTOR) - .setSynchronizationContext(syncContext) - .setServiceConfigParser(mock(ServiceConfigParser.class)) - .setChannelLogger(mock(ChannelLogger.class)) - .build(); private final DnsNameResolverProvider provider = new DnsNameResolverProvider(); private final FakeClock fakeClock = new FakeClock(); private final FakeClock fakeExecutor = new FakeClock(); + private static final FakeClock.TaskFilter NAME_RESOLVER_REFRESH_TASK_FILTER = + new FakeClock.TaskFilter() { + @Override + public boolean shouldAccept(Runnable command) { + return command.toString().contains( + RetryingNameResolver.DelayedNameResolverRefresh.class.getName()); + } + }; private final FakeExecutorResource fakeExecutorResource = new FakeExecutorResource(); @@ -138,6 +140,15 @@ public Executor create() { public void close(Executor instance) {} } + private final NameResolver.Args args = NameResolver.Args.newBuilder() + .setDefaultPort(DEFAULT_PORT) + .setProxyDetector(GrpcUtil.DEFAULT_PROXY_DETECTOR) + .setSynchronizationContext(syncContext) + .setServiceConfigParser(mock(ServiceConfigParser.class)) + .setChannelLogger(mock(ChannelLogger.class)) + .setScheduledExecutorService(fakeExecutor.getScheduledExecutorService()) + .build(); + @Mock private NameResolver.Listener2 mockListener; @Captor @@ -149,18 +160,18 @@ public void close(Executor instance) {} @Mock private RecordFetcher recordFetcher; - private DnsNameResolver newResolver(String name, int defaultPort) { + private RetryingNameResolver newResolver(String name, int defaultPort) { return newResolver( name, defaultPort, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted()); } - private DnsNameResolver newResolver(String name, int defaultPort, boolean isAndroid) { + private RetryingNameResolver newResolver(String name, int defaultPort, boolean isAndroid) { return newResolver( name, defaultPort, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(), isAndroid); } - private DnsNameResolver newResolver( + private RetryingNameResolver newResolver( String name, int defaultPort, ProxyDetector proxyDetector, @@ -168,7 +179,7 @@ private DnsNameResolver newResolver( return newResolver(name, defaultPort, proxyDetector, stopwatch, false); } - private DnsNameResolver newResolver( + private RetryingNameResolver newResolver( String name, final int defaultPort, final ProxyDetector proxyDetector, @@ -181,21 +192,31 @@ private DnsNameResolver newResolver( .setSynchronizationContext(syncContext) .setServiceConfigParser(mock(ServiceConfigParser.class)) .setChannelLogger(mock(ChannelLogger.class)) + .setScheduledExecutorService(fakeExecutor.getScheduledExecutorService()) .build(); return newResolver(name, stopwatch, isAndroid, args); } - private DnsNameResolver newResolver( + private RetryingNameResolver newResolver( String name, Stopwatch stopwatch, boolean isAndroid, NameResolver.Args args) { - DnsNameResolver dnsResolver = - new DnsNameResolver( - null, name, args, fakeExecutorResource, stopwatch, isAndroid); + DnsNameResolver dnsResolver = new DnsNameResolver(null, name, args, fakeExecutorResource, + stopwatch, isAndroid); // By default, using the mocked ResourceResolver to avoid I/O dnsResolver.setResourceResolver(new JndiResourceResolver(recordFetcher)); - return dnsResolver; + + // In practice the DNS name resolver provider always wraps the resolver in a + // RetryingNameResolver which adds retry capabilities to it. We use the same setup here. + return new RetryingNameResolver( + dnsResolver, + new BackoffPolicyRetryScheduler( + new ExponentialBackoffPolicy.Provider(), + fakeExecutor.getScheduledExecutorService(), + syncContext + ), + syncContext); } @Before @@ -203,6 +224,15 @@ public void setUp() { DnsNameResolver.enableJndi = true; networkaddressCacheTtlPropertyValue = System.getProperty(DnsNameResolver.NETWORKADDRESS_CACHE_TTL_PROPERTY); + + // By default the mock listener processes the result successfully. + doAnswer(invocation -> { + ResolutionResult result = invocation.getArgument(0); + syncContext.execute( + () -> result.getAttributes().get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY) + .resolutionAttempted(true)); + return null; + }).when(mockListener).onResult(isA(ResolutionResult.class)); } @After @@ -216,12 +246,6 @@ public void restoreSystemProperty() { } } - @After - public void noMorePendingTasks() { - assertEquals(0, fakeClock.numPendingTasks()); - assertEquals(0, fakeExecutor.numPendingTasks()); - } - @Test public void invalidDnsName() throws Exception { testInvalidUri(new URI("dns", null, "/[invalid]", null)); @@ -287,10 +311,11 @@ private void resolveNeverCache(boolean isAndroid) throws Exception { final List answer2 = createAddressList(1); String name = "foo.googleapis.com"; - DnsNameResolver resolver = newResolver(name, 81, isAndroid); + RetryingNameResolver resolver = newResolver(name, 81, isAndroid); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); AddressResolver mockResolver = mock(AddressResolver.class); when(mockResolver.resolveAddress(anyString())).thenReturn(answer1).thenReturn(answer2); - resolver.setAddressResolver(mockResolver); + dnsResolver.setAddressResolver(mockResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); @@ -303,8 +328,9 @@ private void resolveNeverCache(boolean isAndroid) throws Exception { verify(mockListener, times(2)).onResult(resultCaptor.capture()); assertAnswerMatches(answer2, 81, resultCaptor.getValue()); assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); - resolver.shutdown(); + syncContext.execute(() -> resolver.shutdown()); verify(mockResolver, times(2)).resolveAddress(anyString()); } @@ -313,18 +339,20 @@ private void resolveNeverCache(boolean isAndroid) throws Exception { public void testExecutor_default() throws Exception { final List answer = createAddressList(2); - DnsNameResolver resolver = newResolver("foo.googleapis.com", 81); + RetryingNameResolver resolver = newResolver("foo.googleapis.com", 81); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); AddressResolver mockResolver = mock(AddressResolver.class); when(mockResolver.resolveAddress(anyString())).thenReturn(answer); - resolver.setAddressResolver(mockResolver); + dnsResolver.setAddressResolver(mockResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); verify(mockListener).onResult(resultCaptor.capture()); assertAnswerMatches(answer, 81, resultCaptor.getValue()); assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); - resolver.shutdown(); + syncContext.execute(() -> resolver.shutdown()); assertThat(fakeExecutorResource.createCount.get()).isEqualTo(1); } @@ -341,6 +369,7 @@ public void testExecutor_custom() throws Exception { .setSynchronizationContext(syncContext) .setServiceConfigParser(mock(ServiceConfigParser.class)) .setChannelLogger(mock(ChannelLogger.class)) + .setScheduledExecutorService(fakeExecutor.getScheduledExecutorService()) .setOffloadExecutor( new Executor() { @Override @@ -351,19 +380,21 @@ public void execute(Runnable command) { }) .build(); - DnsNameResolver resolver = - newResolver("foo.googleapis.com", Stopwatch.createUnstarted(), false, args); + RetryingNameResolver resolver = newResolver( + "foo.googleapis.com", Stopwatch.createUnstarted(), false, args); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); AddressResolver mockResolver = mock(AddressResolver.class); when(mockResolver.resolveAddress(anyString())).thenReturn(answer); - resolver.setAddressResolver(mockResolver); + dnsResolver.setAddressResolver(mockResolver); resolver.start(mockListener); assertEquals(0, fakeExecutor.runDueTasks()); verify(mockListener).onResult(resultCaptor.capture()); assertAnswerMatches(answer, 81, resultCaptor.getValue()); assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); - resolver.shutdown(); + syncContext.execute(() -> resolver.shutdown()); assertThat(fakeExecutorResource.createCount.get()).isEqualTo(0); assertThat(executions.get()).isEqualTo(1); @@ -376,13 +407,14 @@ public void resolve_cacheForever() throws Exception { String name = "foo.googleapis.com"; FakeTicker fakeTicker = new FakeTicker(); - DnsNameResolver resolver = - newResolver(name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker)); + RetryingNameResolver resolver = newResolver( + name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker)); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); AddressResolver mockResolver = mock(AddressResolver.class); when(mockResolver.resolveAddress(anyString())) .thenReturn(answer1) .thenThrow(new AssertionError("should not called twice")); - resolver.setAddressResolver(mockResolver); + dnsResolver.setAddressResolver(mockResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); @@ -396,7 +428,7 @@ public void resolve_cacheForever() throws Exception { assertEquals(0, fakeClock.numPendingTasks()); verifyNoMoreInteractions(mockListener); - resolver.shutdown(); + syncContext.execute(() -> resolver.shutdown()); verify(mockResolver).resolveAddress(anyString()); } @@ -409,13 +441,14 @@ public void resolve_usingCache() throws Exception { String name = "foo.googleapis.com"; FakeTicker fakeTicker = new FakeTicker(); - DnsNameResolver resolver = - newResolver(name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker)); + RetryingNameResolver resolver = newResolver( + name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker)); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); AddressResolver mockResolver = mock(AddressResolver.class); when(mockResolver.resolveAddress(anyString())) .thenReturn(answer) .thenThrow(new AssertionError("should not reach here.")); - resolver.setAddressResolver(mockResolver); + dnsResolver.setAddressResolver(mockResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); @@ -430,7 +463,7 @@ public void resolve_usingCache() throws Exception { assertEquals(0, fakeClock.numPendingTasks()); verifyNoMoreInteractions(mockListener); - resolver.shutdown(); + syncContext.execute(() -> resolver.shutdown()); verify(mockResolver).resolveAddress(anyString()); } @@ -444,12 +477,13 @@ public void resolve_cacheExpired() throws Exception { String name = "foo.googleapis.com"; FakeTicker fakeTicker = new FakeTicker(); - DnsNameResolver resolver = - newResolver(name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker)); + RetryingNameResolver resolver = newResolver( + name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker)); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); AddressResolver mockResolver = mock(AddressResolver.class); when(mockResolver.resolveAddress(anyString())).thenReturn(answer1) .thenReturn(answer2); - resolver.setAddressResolver(mockResolver); + dnsResolver.setAddressResolver(mockResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); @@ -463,8 +497,9 @@ public void resolve_cacheExpired() throws Exception { verify(mockListener, times(2)).onResult(resultCaptor.capture()); assertAnswerMatches(answer2, 81, resultCaptor.getValue()); assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); - resolver.shutdown(); + syncContext.execute(() -> resolver.shutdown()); verify(mockResolver, times(2)).resolveAddress(anyString()); } @@ -487,11 +522,12 @@ private void resolveDefaultValue() throws Exception { String name = "foo.googleapis.com"; FakeTicker fakeTicker = new FakeTicker(); - DnsNameResolver resolver = - newResolver(name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker)); + RetryingNameResolver resolver = newResolver( + name, 81, GrpcUtil.NOOP_PROXY_DETECTOR, Stopwatch.createUnstarted(fakeTicker)); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); AddressResolver mockResolver = mock(AddressResolver.class); when(mockResolver.resolveAddress(anyString())).thenReturn(answer1).thenReturn(answer2); - resolver.setAddressResolver(mockResolver); + dnsResolver.setAddressResolver(mockResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); @@ -511,8 +547,9 @@ private void resolveDefaultValue() throws Exception { verify(mockListener, times(2)).onResult(resultCaptor.capture()); assertAnswerMatches(answer2, 81, resultCaptor.getValue()); assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); - resolver.shutdown(); + syncContext.execute(() -> resolver.shutdown()); verify(mockResolver, times(2)).resolveAddress(anyString()); } @@ -520,8 +557,9 @@ private void resolveDefaultValue() throws Exception { @Test public void resolve_emptyResult() throws Exception { DnsNameResolver.enableTxt = true; - DnsNameResolver nr = newResolver("dns:///addr.fake:1234", 443); - nr.setAddressResolver(new AddressResolver() { + RetryingNameResolver resolver = newResolver("dns:///addr.fake:1234", 443); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); + dnsResolver.setAddressResolver(new AddressResolver() { @Override public List resolveAddress(String host) throws Exception { return Collections.emptyList(); @@ -531,18 +569,60 @@ public List resolveAddress(String host) throws Exception { when(mockResourceResolver.resolveTxt(anyString())) .thenReturn(Collections.emptyList()); - nr.setResourceResolver(mockResourceResolver); + dnsResolver.setResourceResolver(mockResourceResolver); - nr.start(mockListener); + resolver.start(mockListener); assertThat(fakeExecutor.runDueTasks()).isEqualTo(1); ArgumentCaptor ac = ArgumentCaptor.forClass(ResolutionResult.class); verify(mockListener).onResult(ac.capture()); verifyNoMoreInteractions(mockListener); assertThat(ac.getValue().getAddresses()).isEmpty(); - assertThat(ac.getValue().getAttributes()).isEqualTo(Attributes.EMPTY); assertThat(ac.getValue().getServiceConfig()).isNull(); verify(mockResourceResolver, never()).resolveSrv(anyString()); + + assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); + } + + // Load balancer rejects the empty addresses. + @Test + public void resolve_emptyResult_notAccepted() throws Exception { + doAnswer(invocation -> { + ResolutionResult result = invocation.getArgument(0); + result.getAttributes().get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY) + .resolutionAttempted(false); + return null; + }).when(mockListener).onResult(isA(ResolutionResult.class)); + + DnsNameResolver.enableTxt = true; + RetryingNameResolver resolver = newResolver("dns:///addr.fake:1234", 443); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); + dnsResolver.setAddressResolver(new AddressResolver() { + @Override + public List resolveAddress(String host) throws Exception { + return Collections.emptyList(); + } + }); + ResourceResolver mockResourceResolver = mock(ResourceResolver.class); + when(mockResourceResolver.resolveTxt(anyString())) + .thenReturn(Collections.emptyList()); + + dnsResolver.setResourceResolver(mockResourceResolver); + + resolver.start(mockListener); + syncContext.execute(() -> assertThat(fakeExecutor.runDueTasks()).isEqualTo(1)); + + ArgumentCaptor ac = ArgumentCaptor.forClass(ResolutionResult.class); + verify(mockListener).onResult(ac.capture()); + verifyNoMoreInteractions(mockListener); + assertThat(ac.getValue().getAddresses()).isEmpty(); + assertThat(ac.getValue().getServiceConfig()).isNull(); + verify(mockResourceResolver, never()).resolveSrv(anyString()); + + assertEquals(0, fakeClock.numPendingTasks()); + // A retry should be scheduled + assertThat(fakeExecutor.numPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)).isEqualTo(1); } @Test @@ -554,9 +634,10 @@ public void resolve_nullResourceResolver() throws Exception { .thenReturn(Collections.singletonList(backendAddr)); String name = "foo.googleapis.com"; - DnsNameResolver resolver = newResolver(name, 81); - resolver.setAddressResolver(mockAddressResolver); - resolver.setResourceResolver(null); + RetryingNameResolver resolver = newResolver(name, 81); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); + dnsResolver.setAddressResolver(mockAddressResolver); + dnsResolver.setResourceResolver(null); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); verify(mockListener).onResult(resultCaptor.capture()); @@ -566,8 +647,10 @@ public void resolve_nullResourceResolver() throws Exception { Iterables.getOnlyElement(result.getAddresses()).getAddresses()); assertThat(resolvedBackendAddr.getAddress()).isEqualTo(backendAddr); verify(mockAddressResolver).resolveAddress(name); - assertThat(result.getAttributes()).isEqualTo(Attributes.EMPTY); assertThat(result.getServiceConfig()).isNull(); + + assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); } @Test @@ -578,15 +661,20 @@ public void resolve_nullResourceResolver_addressFailure() throws Exception { .thenThrow(new IOException("no addr")); String name = "foo.googleapis.com"; - DnsNameResolver resolver = newResolver(name, 81); - resolver.setAddressResolver(mockAddressResolver); - resolver.setResourceResolver(null); + RetryingNameResolver resolver = newResolver(name, 81); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); + dnsResolver.setAddressResolver(mockAddressResolver); + dnsResolver.setResourceResolver(null); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); verify(mockListener).onError(errorCaptor.capture()); Status errorStatus = errorCaptor.getValue(); assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE); assertThat(errorStatus.getCause()).hasMessageThat().contains("no addr"); + + assertEquals(0, fakeClock.numPendingTasks()); + // A retry should be scheduled + assertThat(fakeExecutor.numPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)).isEqualTo(1); } @Test @@ -613,12 +701,14 @@ public ConfigOrError parseServiceConfig(Map rawServiceConfig) { .setProxyDetector(GrpcUtil.NOOP_PROXY_DETECTOR) .setSynchronizationContext(syncContext) .setServiceConfigParser(serviceConfigParser) + .setScheduledExecutorService(fakeClock.getScheduledExecutorService()) .build(); String name = "foo.googleapis.com"; - DnsNameResolver resolver = newResolver(name, Stopwatch.createUnstarted(), false, args); - resolver.setAddressResolver(mockAddressResolver); - resolver.setResourceResolver(mockResourceResolver); + RetryingNameResolver resolver = newResolver(name, Stopwatch.createUnstarted(), false, args); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); + dnsResolver.setAddressResolver(mockAddressResolver); + dnsResolver.setResourceResolver(mockResourceResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); @@ -631,6 +721,9 @@ public ConfigOrError parseServiceConfig(Map rawServiceConfig) { assertThat(result.getServiceConfig().getConfig()).isNotNull(); verify(mockAddressResolver).resolveAddress(name); verify(mockResourceResolver).resolveTxt("_grpc_config." + name); + + assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); } @Test @@ -642,9 +735,10 @@ public void resolve_addressFailure_neverLookUpServiceConfig() throws Exception { String name = "foo.googleapis.com"; ResourceResolver mockResourceResolver = mock(ResourceResolver.class); - DnsNameResolver resolver = newResolver(name, 81); - resolver.setAddressResolver(mockAddressResolver); - resolver.setResourceResolver(mockResourceResolver); + RetryingNameResolver resolver = newResolver(name, 81); + DnsNameResolver dnsResolver = (DnsNameResolver)resolver.getRetriedNameResolver(); + dnsResolver.setAddressResolver(mockAddressResolver); + dnsResolver.setResourceResolver(mockResourceResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); verify(mockListener).onError(errorCaptor.capture()); @@ -652,6 +746,10 @@ public void resolve_addressFailure_neverLookUpServiceConfig() throws Exception { assertThat(errorStatus.getCode()).isEqualTo(Code.UNAVAILABLE); assertThat(errorStatus.getCause()).hasMessageThat().contains("no addr"); verify(mockResourceResolver, never()).resolveTxt(anyString()); + + assertEquals(0, fakeClock.numPendingTasks()); + // A retry should be scheduled + assertThat(fakeExecutor.numPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)).isEqualTo(1); } @Test @@ -666,9 +764,10 @@ public void resolve_serviceConfigLookupFails_nullServiceConfig() throws Exceptio when(mockResourceResolver.resolveTxt(anyString())) .thenThrow(new Exception("something like javax.naming.NamingException")); - DnsNameResolver resolver = newResolver(name, 81); - resolver.setAddressResolver(mockAddressResolver); - resolver.setResourceResolver(mockResourceResolver); + RetryingNameResolver resolver = newResolver(name, 81); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); + dnsResolver.setAddressResolver(mockAddressResolver); + dnsResolver.setResourceResolver(mockResourceResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); verify(mockListener).onResult(resultCaptor.capture()); @@ -678,9 +777,11 @@ public void resolve_serviceConfigLookupFails_nullServiceConfig() throws Exceptio Iterables.getOnlyElement(result.getAddresses()).getAddresses()); assertThat(resolvedBackendAddr.getAddress()).isEqualTo(backendAddr); verify(mockAddressResolver).resolveAddress(name); - assertThat(result.getAttributes()).isEqualTo(Attributes.EMPTY); assertThat(result.getServiceConfig()).isNull(); verify(mockResourceResolver).resolveTxt(anyString()); + + assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); } @Test @@ -695,9 +796,10 @@ public void resolve_serviceConfigMalformed_serviceConfigError() throws Exception when(mockResourceResolver.resolveTxt(anyString())) .thenReturn(Collections.singletonList("grpc_config=something invalid")); - DnsNameResolver resolver = newResolver(name, 81); - resolver.setAddressResolver(mockAddressResolver); - resolver.setResourceResolver(mockResourceResolver); + RetryingNameResolver resolver = newResolver(name, 81); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); + dnsResolver.setAddressResolver(mockAddressResolver); + dnsResolver.setResourceResolver(mockResourceResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); verify(mockListener).onResult(resultCaptor.capture()); @@ -707,10 +809,12 @@ public void resolve_serviceConfigMalformed_serviceConfigError() throws Exception Iterables.getOnlyElement(result.getAddresses()).getAddresses()); assertThat(resolvedBackendAddr.getAddress()).isEqualTo(backendAddr); verify(mockAddressResolver).resolveAddress(name); - assertThat(result.getAttributes()).isEqualTo(Attributes.EMPTY); assertThat(result.getServiceConfig()).isNotNull(); assertThat(result.getServiceConfig().getError()).isNotNull(); verify(mockResourceResolver).resolveTxt(anyString()); + + assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); } @Test @@ -757,11 +861,12 @@ public HttpConnectProxiedSocketAddress proxyFor(SocketAddress targetAddress) { .setPassword("password").build(); } }; - DnsNameResolver resolver = - newResolver(name, port, alwaysDetectProxy, Stopwatch.createUnstarted()); + RetryingNameResolver resolver = newResolver( + name, port, alwaysDetectProxy, Stopwatch.createUnstarted()); + DnsNameResolver dnsResolver = (DnsNameResolver) resolver.getRetriedNameResolver(); AddressResolver mockAddressResolver = mock(AddressResolver.class); when(mockAddressResolver.resolveAddress(anyString())).thenThrow(new AssertionError()); - resolver.setAddressResolver(mockAddressResolver); + dnsResolver.setAddressResolver(mockAddressResolver); resolver.start(mockListener); assertEquals(1, fakeExecutor.runDueTasks()); @@ -777,6 +882,9 @@ public HttpConnectProxiedSocketAddress proxyFor(SocketAddress targetAddress) { assertEquals("username", socketAddress.getUsername()); assertEquals("password", socketAddress.getPassword()); assertTrue(socketAddress.getTargetAddress().isUnresolved()); + + assertEquals(0, fakeClock.numPendingTasks()); + assertEquals(0, fakeExecutor.numPendingTasks()); } @Test @@ -1185,7 +1293,8 @@ private void testInvalidUri(URI uri) { } private void testValidUri(URI uri, String exportedAuthority, int expectedPort) { - DnsNameResolver resolver = provider.newNameResolver(uri, args); + DnsNameResolver resolver = (DnsNameResolver) ((RetryingNameResolver) provider.newNameResolver( + uri, args)).getRetriedNameResolver(); assertNotNull(resolver); assertEquals(expectedPort, resolver.getPort()); assertEquals(exportedAuthority, resolver.getServiceAuthority()); diff --git a/core/src/test/java/io/grpc/internal/ForwardingNameResolverTest.java b/core/src/test/java/io/grpc/internal/ForwardingNameResolverTest.java index f8f9ed2cfbe..5da172626bb 100644 --- a/core/src/test/java/io/grpc/internal/ForwardingNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/ForwardingNameResolverTest.java @@ -81,7 +81,6 @@ public void start_observer() { NameResolver.Listener2 listener = new NameResolver.Listener2() { @Override public void onResult(ResolutionResult result) { - } @Override diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java index 481db99b11d..4aa12973cef 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplGetNameResolverTest.java @@ -43,6 +43,7 @@ public class ManagedChannelImplGetNameResolverTest { .setSynchronizationContext(new SynchronizationContext(mock(UncaughtExceptionHandler.class))) .setServiceConfigParser(mock(ServiceConfigParser.class)) .setChannelLogger(mock(ChannelLogger.class)) + .setScheduledExecutorService(new FakeClock().getScheduledExecutorService()) .build(); @Test diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 6c2a398fe5f..3491eab2e65 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -55,7 +55,6 @@ import com.google.common.base.Throwables; import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Iterables; import com.google.common.util.concurrent.ListenableFuture; import com.google.common.util.concurrent.MoreExecutors; import com.google.common.util.concurrent.SettableFuture; @@ -205,14 +204,6 @@ public String toString() { private final FakeClock timer = new FakeClock(); private final FakeClock executor = new FakeClock(); private final FakeClock balancerRpcExecutor = new FakeClock(); - private static final FakeClock.TaskFilter NAME_RESOLVER_REFRESH_TASK_FILTER = - new FakeClock.TaskFilter() { - @Override - public boolean shouldAccept(Runnable command) { - return command.toString().contains( - ManagedChannelImpl.DelayedNameResolverRefresh.class.getName()); - } - }; private final InternalChannelz channelz = new InternalChannelz(); @@ -309,10 +300,6 @@ public void run() { numExpectedTasks += 1; } - if (getNameResolverRefresh() != null) { - numExpectedTasks += 1; - } - assertEquals(numExpectedTasks, timer.numPendingTasks()); ArgumentCaptor helperCaptor = ArgumentCaptor.forClass(Helper.class); @@ -1062,139 +1049,6 @@ public void callOptionsExecutor() { TimeUnit.SECONDS.toNanos(ManagedChannelImpl.SUBCHANNEL_SHUTDOWN_DELAY_SECONDS)); } - @Test - public void nameResolutionFailed() { - Status error = Status.UNAVAILABLE.withCause(new Throwable("fake name resolution error")); - FakeNameResolverFactory nameResolverFactory = - new FakeNameResolverFactory.Builder(expectedUri) - .setServers(Collections.singletonList(new EquivalentAddressGroup(socketAddress))) - .setError(error) - .build(); - channelBuilder.nameResolverFactory(nameResolverFactory); - // Name resolution is started as soon as channel is created. - createChannel(); - FakeNameResolverFactory.FakeNameResolver resolver = nameResolverFactory.resolvers.get(0); - verify(mockLoadBalancer).handleNameResolutionError(same(error)); - assertEquals(1, timer.numPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)); - - timer.forwardNanos(RECONNECT_BACKOFF_INTERVAL_NANOS - 1); - assertEquals(0, resolver.refreshCalled); - - timer.forwardNanos(1); - assertEquals(1, resolver.refreshCalled); - verify(mockLoadBalancer, times(2)).handleNameResolutionError(same(error)); - - // Verify an additional name resolution failure does not schedule another timer - resolver.refresh(); - verify(mockLoadBalancer, times(3)).handleNameResolutionError(same(error)); - assertEquals(1, timer.numPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)); - - // Allow the next refresh attempt to succeed - resolver.error = null; - - // For the second attempt, the backoff should occur at RECONNECT_BACKOFF_INTERVAL_NANOS * 2 - timer.forwardNanos(RECONNECT_BACKOFF_INTERVAL_NANOS * 2 - 1); - assertEquals(2, resolver.refreshCalled); - timer.forwardNanos(1); - assertEquals(3, resolver.refreshCalled); - assertEquals(0, timer.numPendingTasks()); - - // Verify that the successful resolution reset the backoff policy - resolver.listener.onError(error); - timer.forwardNanos(RECONNECT_BACKOFF_INTERVAL_NANOS - 1); - assertEquals(3, resolver.refreshCalled); - timer.forwardNanos(1); - assertEquals(4, resolver.refreshCalled); - assertEquals(0, timer.numPendingTasks()); - } - - @Test - public void nameResolutionFailed_delayedTransportShutdownCancelsBackoff() { - Status error = Status.UNAVAILABLE.withCause(new Throwable("fake name resolution error")); - - FakeNameResolverFactory nameResolverFactory = - new FakeNameResolverFactory.Builder(expectedUri).setError(error).build(); - channelBuilder.nameResolverFactory(nameResolverFactory); - // Name resolution is started as soon as channel is created. - createChannel(); - verify(mockLoadBalancer).handleNameResolutionError(same(error)); - - FakeClock.ScheduledTask nameResolverBackoff = getNameResolverRefresh(); - assertNotNull(nameResolverBackoff); - assertFalse(nameResolverBackoff.isCancelled()); - - // Add a pending call to the delayed transport - ClientCall call = channel.newCall(method, CallOptions.DEFAULT); - Metadata headers = new Metadata(); - call.start(mockCallListener, headers); - - // The pending call on the delayed transport stops the name resolver backoff from cancelling - channel.shutdown(); - assertFalse(nameResolverBackoff.isCancelled()); - - // Notify that a subchannel is ready, which drains the delayed transport - SubchannelPicker picker = mock(SubchannelPicker.class); - Status status = Status.UNAVAILABLE.withDescription("for test"); - when(picker.pickSubchannel(any(PickSubchannelArgs.class))) - .thenReturn(PickResult.withDrop(status)); - updateBalancingStateSafely(helper, READY, picker); - executor.runDueTasks(); - verify(mockCallListener).onClose(same(status), any(Metadata.class)); - - assertTrue(nameResolverBackoff.isCancelled()); - } - - @Test - public void nameResolverReturnsEmptySubLists_resolutionRetry() throws Exception { - // The mock LB is set to reject the addresses. - when(mockLoadBalancer.acceptResolvedAddresses(isA(ResolvedAddresses.class))).thenReturn(false); - - // Pass a FakeNameResolverFactory with an empty list and LB config - FakeNameResolverFactory nameResolverFactory = - new FakeNameResolverFactory.Builder(expectedUri).build(); - Map rawServiceConfig = - parseConfig("{\"loadBalancingConfig\": [ {\"mock_lb\": { \"setting1\": \"high\" } } ] }"); - ManagedChannelServiceConfig parsedServiceConfig = - createManagedChannelServiceConfig(rawServiceConfig, null); - nameResolverFactory.nextConfigOrError.set(ConfigOrError.fromConfig(parsedServiceConfig)); - channelBuilder.nameResolverFactory(nameResolverFactory); - createChannel(); - - // A resolution retry has been scheduled - assertEquals(1, timer.numPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)); - } - - @Test - public void nameResolverReturnsEmptySubLists_optionallyAllowed() throws Exception { - // Pass a FakeNameResolverFactory with an empty list and LB config - FakeNameResolverFactory nameResolverFactory = - new FakeNameResolverFactory.Builder(expectedUri).build(); - String rawLbConfig = "{ \"setting1\": \"high\" }"; - Object parsedLbConfig = new Object(); - Map rawServiceConfig = - parseConfig("{\"loadBalancingConfig\": [ {\"mock_lb\": " + rawLbConfig + " } ] }"); - ManagedChannelServiceConfig parsedServiceConfig = - createManagedChannelServiceConfig( - rawServiceConfig, - new PolicySelection( - mockLoadBalancerProvider, - parsedLbConfig)); - nameResolverFactory.nextConfigOrError.set(ConfigOrError.fromConfig(parsedServiceConfig)); - channelBuilder.nameResolverFactory(nameResolverFactory); - createChannel(); - - // LoadBalancer received the empty list and the LB config - verify(mockLoadBalancerProvider).newLoadBalancer(any(Helper.class)); - ArgumentCaptor resultCaptor = - ArgumentCaptor.forClass(ResolvedAddresses.class); - verify(mockLoadBalancer).acceptResolvedAddresses(resultCaptor.capture()); - assertThat(resultCaptor.getValue().getAddresses()).isEmpty(); - assertThat(resultCaptor.getValue().getLoadBalancingPolicyConfig()).isEqualTo(parsedLbConfig); - - // A no resolution retry - assertEquals(0, timer.numPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER)); - } - @Test public void loadBalancerThrowsInHandleResolvedAddresses() { RuntimeException ex = new RuntimeException("simulated"); @@ -3019,52 +2873,6 @@ public void balancerRefreshNameResolution() { assertEquals(initialRefreshCount + 1, resolver.refreshCalled); } - @Test - public void resetConnectBackoff() { - // Start with a name resolution failure to trigger backoff attempts - Status error = Status.UNAVAILABLE.withCause(new Throwable("fake name resolution error")); - FakeNameResolverFactory nameResolverFactory = - new FakeNameResolverFactory.Builder(expectedUri).setError(error).build(); - channelBuilder.nameResolverFactory(nameResolverFactory); - // Name resolution is started as soon as channel is created. - createChannel(); - FakeNameResolverFactory.FakeNameResolver resolver = nameResolverFactory.resolvers.get(0); - verify(mockLoadBalancer).handleNameResolutionError(same(error)); - - FakeClock.ScheduledTask nameResolverBackoff = getNameResolverRefresh(); - assertNotNull("There should be a name resolver backoff task", nameResolverBackoff); - assertEquals(0, resolver.refreshCalled); - - // Verify resetConnectBackoff() calls refresh and cancels the scheduled backoff - channel.resetConnectBackoff(); - assertEquals(1, resolver.refreshCalled); - assertTrue(nameResolverBackoff.isCancelled()); - - // Simulate a race between cancel and the task scheduler. Should be a no-op. - nameResolverBackoff.command.run(); - assertEquals(1, resolver.refreshCalled); - - // Verify that the reconnect policy was recreated and the backoff multiplier reset to 1 - timer.forwardNanos(RECONNECT_BACKOFF_INTERVAL_NANOS); - assertEquals(2, resolver.refreshCalled); - } - - @Test - public void resetConnectBackoff_noOpWithoutPendingResolverBackoff() { - FakeNameResolverFactory nameResolverFactory = - new FakeNameResolverFactory.Builder(expectedUri) - .setServers(Collections.singletonList(new EquivalentAddressGroup(socketAddress))) - .build(); - channelBuilder.nameResolverFactory(nameResolverFactory); - createChannel(); - FakeNameResolverFactory.FakeNameResolver nameResolver = nameResolverFactory.resolvers.get(0); - assertEquals(0, nameResolver.refreshCalled); - - channel.resetConnectBackoff(); - - assertEquals(0, nameResolver.refreshCalled); - } - @Test public void resetConnectBackoff_noOpWhenChannelShutdown() { FakeNameResolverFactory nameResolverFactory = @@ -4517,10 +4325,6 @@ private static ChannelStats getStats( return instrumented.getStats().get(); } - private FakeClock.ScheduledTask getNameResolverRefresh() { - return Iterables.getOnlyElement(timer.getPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER), null); - } - // Helper methods to call methods from SynchronizationContext private static Subchannel createSubchannelSafely( final Helper helper, final EquivalentAddressGroup addressGroup, final Attributes attrs, diff --git a/core/src/test/java/io/grpc/internal/RetryingNameResolverTest.java b/core/src/test/java/io/grpc/internal/RetryingNameResolverTest.java new file mode 100644 index 00000000000..b13b94e2491 --- /dev/null +++ b/core/src/test/java/io/grpc/internal/RetryingNameResolverTest.java @@ -0,0 +1,142 @@ +/* + * Copyright 2023 The gRPC Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.grpc.internal; + +import static com.google.common.truth.Truth.assertThat; +import static org.junit.Assert.fail; +import static org.mockito.ArgumentMatchers.isA; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; + +import io.grpc.NameResolver; +import io.grpc.NameResolver.Listener2; +import io.grpc.NameResolver.ResolutionResult; +import io.grpc.Status; +import io.grpc.SynchronizationContext; +import io.grpc.internal.RetryingNameResolver.ResolutionResultListener; +import java.lang.Thread.UncaughtExceptionHandler; +import org.junit.Before; +import org.junit.Rule; +import org.junit.Test; +import org.junit.runner.RunWith; +import org.junit.runners.JUnit4; +import org.mockito.ArgumentCaptor; +import org.mockito.Captor; +import org.mockito.Mock; +import org.mockito.junit.MockitoJUnit; +import org.mockito.junit.MockitoRule; + +/** + * Unit test for {@link RetryingNameResolver}. + */ +@RunWith(JUnit4.class) +public class RetryingNameResolverTest { + + @Rule + public final MockitoRule mocks = MockitoJUnit.rule(); + + @Mock + private NameResolver mockNameResolver; + @Mock + private Listener2 mockListener; + @Mock + private RetryScheduler mockRetryScheduler; + @Captor + private ArgumentCaptor listenerCaptor; + @Captor + private ArgumentCaptor onResultCaptor; + private final SynchronizationContext syncContext = new SynchronizationContext( + mock(UncaughtExceptionHandler.class)); + + private RetryingNameResolver retryingNameResolver; + + @Before + public void setup() { + retryingNameResolver = new RetryingNameResolver(mockNameResolver, mockRetryScheduler, + syncContext); + } + + @Test + public void startAndShutdown() { + retryingNameResolver.start(mockListener); + retryingNameResolver.shutdown(); + } + + // Make sure the ResolutionResultListener callback is added to the ResolutionResult attributes, + // and the retry scheduler is reset since the name resolution was successful. + @Test + public void onResult_sucess() { + retryingNameResolver.start(mockListener); + verify(mockNameResolver).start(listenerCaptor.capture()); + + listenerCaptor.getValue().onResult(ResolutionResult.newBuilder().build()); + verify(mockListener).onResult(onResultCaptor.capture()); + ResolutionResultListener resolutionResultListener = onResultCaptor.getValue() + .getAttributes() + .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY); + assertThat(resolutionResultListener).isNotNull(); + + resolutionResultListener.resolutionAttempted(true); + verify(mockRetryScheduler).reset(); + } + + // Make sure the ResolutionResultListener callback is added to the ResolutionResult attributes, + // and that a retry gets scheduled when the resolution results are rejected. + @Test + public void onResult_failure() { + retryingNameResolver.start(mockListener); + verify(mockNameResolver).start(listenerCaptor.capture()); + + listenerCaptor.getValue().onResult(ResolutionResult.newBuilder().build()); + verify(mockListener).onResult(onResultCaptor.capture()); + ResolutionResultListener resolutionResultListener = onResultCaptor.getValue() + .getAttributes() + .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY); + assertThat(resolutionResultListener).isNotNull(); + + resolutionResultListener.resolutionAttempted(false); + verify(mockRetryScheduler).schedule(isA(Runnable.class)); + } + + // Wrapping a NameResolver more than once is a misconfiguration. + @Test + public void onResult_failure_doubleWrapped() { + NameResolver doubleWrappedResolver = new RetryingNameResolver(retryingNameResolver, + mockRetryScheduler, syncContext); + + doubleWrappedResolver.start(mockListener); + verify(mockNameResolver).start(listenerCaptor.capture()); + + try { + listenerCaptor.getValue().onResult(ResolutionResult.newBuilder().build()); + } catch (IllegalStateException e) { + assertThat(e).hasMessageThat().contains("can only be used once"); + return; + } + fail("An exception should have been thrown for a double wrapped NAmeResolver"); + } + + // A retry should get scheduled when name resolution fails. + @Test + public void onError() { + retryingNameResolver.start(mockListener); + verify(mockNameResolver).start(listenerCaptor.capture()); + listenerCaptor.getValue().onError(Status.DEADLINE_EXCEEDED); + verify(mockListener).onError(Status.DEADLINE_EXCEEDED); + verify(mockRetryScheduler).schedule(isA(Runnable.class)); + } +} \ No newline at end of file diff --git a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java index 24317e80692..f2f99460286 100644 --- a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java +++ b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java @@ -98,7 +98,7 @@ public String toString() { @Override public boolean shouldAccept(Runnable command) { return command.toString().contains( - ManagedChannelImpl.DelayedNameResolverRefresh.class.getName()); + RetryingNameResolver.DelayedNameResolverRefresh.class.getName()); } }; @@ -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,7 +561,13 @@ public NameResolver newNameResolver(final URI targetUri, NameResolver.Args args) return null; } assertEquals(DEFAULT_PORT, args.getDefaultPort()); - FakeNameResolver resolver = new FakeNameResolver(args.getServiceConfigParser()); + RetryingNameResolver resolver = new RetryingNameResolver( + new FakeNameResolver(args.getServiceConfigParser()), + new BackoffPolicyRetryScheduler( + new FakeBackoffPolicyProvider(), + args.getScheduledExecutorService(), + args.getSynchronizationContext()), + args.getSynchronizationContext()); resolvers.add(resolver); return resolver; } @@ -572,8 +578,8 @@ public String getDefaultScheme() { } void allResolved() { - for (FakeNameResolver resolver : resolvers) { - resolver.resolved(); + for (RetryingNameResolver resolver : resolvers) { + ((FakeNameResolver)resolver.getRetriedNameResolver()).resolved(); } } @@ -647,7 +653,8 @@ FakeNameResolverFactory build() { } private FakeClock.ScheduledTask getNameResolverRefresh() { - return Iterables.getOnlyElement(timer.getPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER), null); + return Iterables.getOnlyElement( + timer.getPendingTasks(NAME_RESOLVER_REFRESH_TASK_FILTER), null); } private static class FakeLoadBalancer extends LoadBalancer {