Revert "Move name resolution retry from managed channel to name resolver. (#9758)"

This reverts commit 43bc578. It breaks
API without stair-stepping and needs to be tweaked architecturally.

Author: ejona86

api/src/main/java/io/grpc/NameResolver.java

@@ -59,10 +59,6 @@
  */
 @ExperimentalApi("https://github.com/grpc/grpc-java/issues/1770")
 public abstract class NameResolver {
-
-  // Outside listeners that get notified of the result of each name resolution.
-  private ArrayList<ResolutionResultListener> resolutionResultListeners = new ArrayList<>();
-
   /**
    * Returns the authority used to authenticate connections to servers.  It <strong>must</strong> be
    * from a trusted source, because if the authority is tampered with, RPCs may be sent to the
@@ -95,9 +91,8 @@ public void onError(Status error) {
           }
 
           @Override
-          public boolean onResult(ResolutionResult resolutionResult) {
+          public void onResult(ResolutionResult resolutionResult) {
             listener.onAddresses(resolutionResult.getAddresses(), resolutionResult.getAttributes());
-            return true;
           }
       });
     }
@@ -136,43 +131,6 @@ public void start(Listener2 listener) {
    */
   public void refresh() {}
 
-  /**
-   * Adds a new {@link ResolutionResultListener} that will get notified of the outcome of each
-   * resolution.
-   *
-   * @since 1.53.0
-   */
-  public final void addResolutionResultListener(ResolutionResultListener listener) {
-    checkArgument(listener != null, "listener");
-    resolutionResultListeners.add(listener);
-  }
-
-  /**
-   * Removes an existing {@link ResolutionResultListener}.
-   *
-   * @return {@code true} if the listener was removed, otherwise {@code false}
-   * @since 1.53.0
-   */
-  public final boolean removeResolutionResultListener(ResolutionResultListener listener) {
-    checkArgument(listener != null);
-    return resolutionResultListeners.remove(listener);
-  }
-
-  /**
-   * Intended for extending classes to call when they know the result of a name resolution.
-   *
-   * <p>Note that while these listeners can be added to any {@link NameResolver}, only concrete
-   * implementations that call this method will actually support this facility.
-   *
-   * @param successful {@code true} if resolution was successful and the addresses were accepted.
-   * @since 1.53.0
-   */
-  protected final void fireResolutionResultEvent(boolean successful) {
-    for (ResolutionResultListener listener : resolutionResultListeners) {
-      listener.resolutionAttempted(successful);
-    }
-  }
-
   /**
    * Factory that creates {@link NameResolver} instances.
    *
@@ -267,12 +225,9 @@ public final void onAddresses(
      * {@link ResolutionResult#getAddresses()} is empty, {@link #onError(Status)} will be called.
      *
      * @param resolutionResult the resolved server addresses, attributes, and Service Config.
-     * @return {@code true} if the listener accepts the resolved addresses, otherwise {@code false}.
-     *         If the addresses are not accepted the {@link NameResolver} will refresh and retry
-     *         later if it uses polling.
      * @since 1.21.0
      */
-    public abstract boolean onResult(ResolutionResult resolutionResult);
+    public abstract void onResult(ResolutionResult resolutionResult);
 
     /**
      * Handles a name resolving error from the resolver. The listener is responsible for eventually
@@ -294,24 +249,6 @@ public final void onAddresses(
   @Documented
   public @interface ResolutionResultAttr {}
 
-
-  /**
-   * A callback interface called at the end of every resolve operation to indicate if the operation
-   * was successful. Success means that there were no problems with either the name resolution part
-   * nor with {@link Listener} accepting the resolution results.
-   */
-  public interface ResolutionResultListener {
-
-    /**
-     * Called after an attempt at name resolution.
-     *
-     * <p>Note! Implementations of this should return quickly and not throw exceptions.
-     *
-     * @param successful {@code true} if resolution was successful and the addresses were accepted.
-     */
-    void resolutionAttempted(boolean successful);
-  }
-
   /**
    * Information that a {@link Factory} uses to create a {@link NameResolver}.
    *

core/src/main/java/io/grpc/internal/DnsNameResolver.java

@@ -318,7 +318,6 @@ public void run() {
           result = doResolve(false);
           if (result.error != null) {
             savedListener.onError(result.error);
-            fireResolutionResultEvent(false);
             return;
           }
           if (result.addresses != null) {
@@ -331,7 +330,7 @@ public void run() {
             resolutionResultBuilder.setAttributes(result.attributes);
           }
         }
-        fireResolutionResultEvent(savedListener.onResult(resolutionResultBuilder.build()));
+        savedListener.onResult(resolutionResultBuilder.build());
       } catch (IOException e) {
         savedListener.onError(
             Status.UNAVAILABLE.withDescription("Unable to resolve host " + host).withCause(e));

core/src/main/java/io/grpc/internal/DnsNameResolverProvider.java

@@ -47,23 +47,19 @@ public final class DnsNameResolverProvider extends NameResolverProvider {
   private static final String SCHEME = "dns";
 
   @Override
-  public NameResolver newNameResolver(URI targetUri, NameResolver.Args args) {
+  public DnsNameResolver 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 RetryingNameResolver(
-          new DnsNameResolver(
-              targetUri.getAuthority(),
-              name,
-              args,
-              GrpcUtil.SHARED_CHANNEL_EXECUTOR,
-              Stopwatch.createUnstarted(),
-              InternalServiceProviders.isAndroid(getClass().getClassLoader())),
-          new ExponentialBackoffPolicy.Provider(),
-          args.getScheduledExecutorService(),
-          args.getSynchronizationContext());
+      return new DnsNameResolver(
+          targetUri.getAuthority(),
+          name,
+          args,
+          GrpcUtil.SHARED_CHANNEL_EXECUTOR,
+          Stopwatch.createUnstarted(),
+          InternalServiceProviders.isAndroid(getClass().getClassLoader()));
     } else {
       return null;
     }

core/src/main/java/io/grpc/internal/ManagedChannelImpl.java

@@ -280,10 +280,6 @@ public void uncaughtException(Thread t, Throwable e) {
   // Must be mutated and read from constructor or syncContext
   // used for channel tracing when value changed
   private ManagedChannelServiceConfig lastServiceConfig = EMPTY_SERVICE_CONFIG;
-  // Must be mutated and read from constructor or syncContext
-  // Denotes if the last resolved addresses were accepted by the load balancer. A {@code null}
-  // value indicates no attempt has been made yet.
-  private Boolean lastAddressesAccepted;
 
   @Nullable
   private final ManagedChannelServiceConfig defaultServiceConfig;
@@ -371,6 +367,7 @@ private void shutdownNameResolverAndLoadBalancer(boolean channelIsActive) {
       checkState(lbHelper != null, "lbHelper is null");
     }
     if (nameResolver != null) {
+      cancelNameResolverBackoff();
       nameResolver.shutdown();
       nameResolverStarted = false;
       if (channelIsActive) {
@@ -453,10 +450,42 @@ 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. Must be run
+   * Force name resolution refresh to happen immediately and reset refresh back-off. Must be run
    * from syncContext.
    */
+  private void refreshAndResetNameResolution() {
+    syncContext.throwIfNotInThisSynchronizationContext();
+    cancelNameResolverBackoff();
+    refreshNameResolution();
+  }
+
   private void refreshNameResolution() {
     syncContext.throwIfNotInThisSynchronizationContext();
     if (nameResolverStarted) {
@@ -1261,7 +1290,7 @@ private void maybeTerminateChannel() {
   // Must be called from syncContext
   private void handleInternalSubchannelState(ConnectivityStateInfo newState) {
     if (newState.getState() == TRANSIENT_FAILURE || newState.getState() == IDLE) {
-      refreshNameResolution();
+      refreshAndResetNameResolution();
     }
   }
 
@@ -1308,9 +1337,9 @@ public void run() {
         if (shutdown.get()) {
           return;
         }
-        if (lastAddressesAccepted != null && !lastAddressesAccepted) {
+        if (scheduledNameResolverRefresh != null && scheduledNameResolverRefresh.isPending()) {
           checkState(nameResolverStarted, "name resolver must be started");
-          refreshNameResolution();
+          refreshAndResetNameResolution();
         }
         for (InternalSubchannel subchannel : subchannels) {
           subchannel.resetConnectBackoff();
@@ -1466,7 +1495,7 @@ public void refreshNameResolution() {
       final class LoadBalancerRefreshNameResolution implements Runnable {
         @Override
         public void run() {
-          ManagedChannelImpl.this.refreshNameResolution();
+          refreshAndResetNameResolution();
         }
       }
 
@@ -1707,7 +1736,7 @@ private final class NameResolverListener extends NameResolver.Listener2 {
     }
 
     @Override
-    public boolean onResult(final ResolutionResult resolutionResult) {
+    public void onResult(final ResolutionResult resolutionResult) {
       final class NamesResolved implements Runnable {
 
         @SuppressWarnings("ReferenceEquality")
@@ -1716,7 +1745,6 @@ public void run() {
           if (ManagedChannelImpl.this.nameResolver != resolver) {
             return;
           }
-          lastAddressesAccepted = false;
 
           List<EquivalentAddressGroup> servers = resolutionResult.getAddresses();
           channelLogger.log(
@@ -1730,6 +1758,7 @@ public void run() {
             lastResolutionState = ResolutionState.SUCCESS;
           }
 
+          nameResolverBackoffPolicy = null;
           ConfigOrError configOrError = resolutionResult.getServiceConfig();
           InternalConfigSelector resolvedConfigSelector =
               resolutionResult.getAttributes().get(InternalConfigSelector.KEY);
@@ -1787,7 +1816,6 @@ public void run() {
                 // we later check for these error codes when investigating pick results in
                 // GrpcUtil.getTransportFromPickResult().
                 onError(configOrError.getError());
-                lastAddressesAccepted = false;
                 return;
               } else {
                 effectiveServiceConfig = lastServiceConfig;
@@ -1831,24 +1859,21 @@ public void run() {
             }
             Attributes attributes = attrBuilder.build();
 
-            lastAddressesAccepted = helper.lb.tryAcceptResolvedAddresses(
+            boolean addressesAccepted = helper.lb.tryAcceptResolvedAddresses(
                 ResolvedAddresses.newBuilder()
                     .setAddresses(servers)
                     .setAttributes(attributes)
                     .setLoadBalancingPolicyConfig(effectiveServiceConfig.getLoadBalancingConfig())
                     .build());
+
+            if (!addressesAccepted) {
+              scheduleExponentialBackOffInSyncContext();
+            }
           }
         }
       }
 
       syncContext.execute(new NamesResolved());
-
-      // If NameResolved did not assign a value to lastAddressesAccepted, we assume there was an
-      // exception and set it to false.
-      if (lastAddressesAccepted == null) {
-        lastAddressesAccepted = false;
-      }
-      return lastAddressesAccepted;
     }
 
     @Override
@@ -1878,6 +1903,29 @@ 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());
     }
   }