Review feedback

- Do not change the public API
- Use a hacky internal callback
- More unit test coverage

Author: temawi

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

@@ -60,9 +60,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 +92,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 +132,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 +226,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 +250,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/BackoffPolicyRetryScheduler.java

@@ -0,0 +1,82 @@
+/*
+ * 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.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.
+   *
+   * @return The delay in nanos before the operation fires or -1 if it was not scheduled.
+   */
+  @Override
+  public long schedule(Runnable retryOperation) {
+    if (policy == null) {
+      policy = policyProvider.get();
+    }
+    // If a retry is already scheduled, take no further action.
+    if (scheduledHandle != null && scheduledHandle.isPending()) {
+      return -1;
+    }
+    long delayNanos = policy.nextBackoffNanos();
+    scheduledHandle = syncContext.schedule(retryOperation, delayNanos, TimeUnit.NANOSECONDS,
+        scheduledExecutorService);
+    logger.fine("Scheduling DNS resolution backoff for " + delayNanos + "ns");
+
+    return 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() {
+    if (scheduledHandle != null && scheduledHandle.isPending()) {
+      scheduledHandle.cancel();
+    }
+    policy = null;
+  }
+
+}

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

@@ -61,9 +61,10 @@ public NameResolver newNameResolver(URI targetUri, NameResolver.Args args) {
               GrpcUtil.SHARED_CHANNEL_EXECUTOR,
               Stopwatch.createUnstarted(),
               InternalServiceProviders.isAndroid(getClass().getClassLoader())),
-          new ExponentialBackoffPolicy.Provider(),
-          args.getScheduledExecutorService(),
-          args.getSynchronizationContext());
+          new BackoffPolicyRetryScheduler(
+              new ExponentialBackoffPolicy.Provider(),
+              args.getScheduledExecutorService(),
+              args.getSynchronizationContext()));
     } else {
       return null;
     }

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;
@@ -1697,7 +1698,7 @@ public ChannelCredentials withoutBearerTokens() {
     }
   }
 
-  private final class NameResolverListener extends NameResolver.Listener2 {
+  final class NameResolverListener extends NameResolver.Listener2 {
     final LbHelperImpl helper;
     final NameResolver resolver;
 
@@ -1707,7 +1708,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")
@@ -1848,7 +1849,15 @@ public void run() {
       if (lastAddressesAccepted == null) {
         lastAddressesAccepted = false;
       }
-      return lastAddressesAccepted;
+
+      // If a listener is provided, let it know if the addresses were accepted.
+      // TODO(tmwilson): Once we are ready to change the onResult() API and return a boolean
+      // this hacky callback in an attribute approach can be removed.
+      ResolutionResultListener resolutionResultListener = resolutionResult.getAttributes()
+          .get(RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY);
+      if (resolutionResultListener != null) {
+        resolutionResultListener.resolutionAttempted(lastAddressesAccepted);
+      }
     }
 
     @Override

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 2022 The gRPC Authors
+ * 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.
@@ -16,65 +16,22 @@
 
 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.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.
+ * 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.
  */
-final class RetryScheduler {
-  private final Runnable retryOperation;
-  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(RetryScheduler.class.getName());
-
-  RetryScheduler(Runnable retryOperation, ScheduledExecutorService scheduledExecutorService,
-      SynchronizationContext syncContext, BackoffPolicy.Provider policyProvider) {
-    this.retryOperation = retryOperation;
-    this.scheduledExecutorService = scheduledExecutorService;
-    this.syncContext = syncContext;
-    this.policyProvider = policyProvider;
-  }
+public interface RetryScheduler {
 
   /**
-   * Schedules a future retry operation. Only allows one retry to be scheduled at any given time.
+   * A request to schedule a future retry (or retries) for a failed operation.
    *
    * @return The delay in nanos before the operation fires or -1 if it was not scheduled.
    */
-  long schedule() {
-    if (policy == null) {
-      policy = policyProvider.get();
-    }
-    // If a retry is already scheduled, take no further action.
-    if (scheduledHandle != null && scheduledHandle.isPending()) {
-      return -1;
-    }
-    long delayNanos = policy.nextBackoffNanos();
-    scheduledHandle = syncContext.schedule(retryOperation, delayNanos, TimeUnit.NANOSECONDS,
-        scheduledExecutorService);
-    logger.fine("Scheduling DNS resolution backoff for " + delayNanos + "ns");
-
-    return delayNanos;
-  }
+  long schedule(Runnable retryOperation);
 
   /**
-   * Resets the {@link RetryScheduler} and cancels any pending retry task. The policy will be
-   * cleared thus also resetting any state associated with it (e.g. a backoff multiplier).
+   * Resets the scheduler, effectively cancelling any future retry operation.
    */
-  void reset() {
-    if (scheduledHandle != null && scheduledHandle.isPending()) {
-      scheduledHandle.cancel();
-    }
-    policy = null;
-  }
-
+  void reset();
 }