AbstractRetryingFilterBuilder to require jitter (#1692)

Motivation: AbstractRetryingFilterBuilder currently allows configuration with no jitter, but this will trigger an assertion error at build time. In practice most use cases are encouraged to use some form of jitter so we should deprecate the non-jitter build methods and make sure they don't trigger an assertion error. Modifications: - Deprecate buildWithImmediateRetries, buildWithConstantBackoff, and buildWithConstantBackoff methods on AbstractRetryingFilterBuilder. - Enhance jitter bounds upfront validation in RepeatStrategies and RetryStrategies to throw early if we can easily detect invalid arguments. Result: No more assertion error in AbstractRetryingFilterBuilder due to lack of jitter, users encouraged to use jitter in retry strategies.
apple · Jul 22, 2021 · 813ab13 · 813ab13
1 parent ba13567
commit 813ab13
Show file tree

Hide file tree

Showing 4 changed files with 31 additions and 14 deletions.
diff --git a/...alk-client-api/src/main/java/io/servicetalk/client/api/AbstractRetryingFilterBuilder.java b/...alk-client-api/src/main/java/io/servicetalk/client/api/AbstractRetryingFilterBuilder.java
@@ -34,6 +34,7 @@
 import static io.servicetalk.concurrent.api.RetryStrategies.retryWithExponentialBackoffDeltaJitter;
 import static io.servicetalk.concurrent.api.RetryStrategies.retryWithExponentialBackoffFullJitter;
 import static java.time.Duration.ofDays;
+import static java.time.Duration.ofMillis;
 import static java.util.Objects.requireNonNull;
 
 /**
@@ -48,6 +49,7 @@
 public abstract class AbstractRetryingFilterBuilder<Builder
         extends AbstractRetryingFilterBuilder<Builder, Filter, Meta>, Filter, Meta> {
     private static final Duration FULL_JITTER = ofDays(1024);
+    private static final Duration NULL_JITTER = ofMillis(1);
     private int maxRetries;
     @Nullable
     private BiPredicate<Meta, Throwable> retryForPredicate;
@@ -85,33 +87,39 @@ public final Builder retryFor(final BiPredicate<Meta, Throwable> retryForPredica
 
     /**
      * Creates a new retrying {@link Filter} which retries without delay.
-     *
+     * @deprecated Use {@link #buildWithConstantBackoffFullJitter(Duration)} or
+     * {@link #buildWithConstantBackoffFullJitter(Duration)}.
      * @return a new retrying {@link Filter} which retries without delay
      */
+    @Deprecated
     public final Filter buildWithImmediateRetries() {
-        return build(readOnlySettings(null, null, null, null, false));
+        return build(readOnlySettings(null, NULL_JITTER, null, null, false));
     }
 
     /**
      * Creates a new retrying {@link Filter} which adds the passed constant {@link Duration} as a delay between retries.
-     *
+     * @deprecated Use {@link #buildWithConstantBackoffDeltaJitter(Duration, Duration)} or
+     * {@link #buildWithConstantBackoffFullJitter(Duration)}.
      * @param delay Constant {@link Duration} of delay between retries
      * @return A new retrying {@link Filter} which adds a constant delay between retries
      */
+    @Deprecated
     public final Filter buildWithConstantBackoff(final Duration delay) {
-        return build(readOnlySettings(delay, null, null, null, false));
+        return build(readOnlySettings(delay, NULL_JITTER, null, null, false));
     }
 
     /**
      * Creates a new retrying {@link Filter} which adds the passed constant {@link Duration} as a delay between retries.
-     *
+     * @deprecated Use {@link #buildWithConstantBackoffFullJitter(Duration, Executor)} or
+     * {@link #buildWithConstantBackoffDeltaJitter(Duration, Duration)}.
      * @param delay Constant {@link Duration} of delay between retries
      * @param timerExecutor {@link Executor} to be used to schedule timers for backoff. It takes precedence over an
      * alternative timer {@link Executor} from {@link ReadOnlyRetryableSettings#newStrategy(Executor)} argument
      * @return A new retrying {@link Filter} which adds a constant delay between retries
      */
+    @Deprecated
     public final Filter buildWithConstantBackoff(final Duration delay, final Executor timerExecutor) {
-        return build(readOnlySettings(delay, null, null, timerExecutor, false));
+        return build(readOnlySettings(delay, NULL_JITTER, null, timerExecutor, false));
     }
 
     /**
@@ -247,7 +255,7 @@ public BiPredicate<Meta, Throwable> defaultRetryForPredicate() {
     }
 
     private ReadOnlyRetryableSettings<Meta> readOnlySettings(@Nullable final Duration initialDelay,
-                                                             @Nullable final Duration jitter,
+                                                             final Duration jitter,
                                                              @Nullable final Duration maxDelay,
                                                              @Nullable final Executor timerExecutor,
                                                              final boolean exponential) {
@@ -267,7 +275,6 @@ public static final class ReadOnlyRetryableSettings<Meta> {
         private final BiPredicate<Meta, Throwable> retryForPredicate;
         @Nullable
         private final Duration initialDelay;
-        @Nullable
         private final Duration jitter;
         @Nullable
         private final Duration maxDelay;
@@ -278,7 +285,7 @@ public static final class ReadOnlyRetryableSettings<Meta> {
         private ReadOnlyRetryableSettings(final int maxRetries,
                                           final BiPredicate<Meta, Throwable> retryForPredicate,
                                           @Nullable final Duration initialDelay,
-                                          @Nullable final Duration jitter,
+                                          final Duration jitter,
                                           @Nullable final Duration maxDelay,
                                           @Nullable final Executor timerExecutor,
                                           final boolean exponential) {
@@ -287,7 +294,7 @@ private ReadOnlyRetryableSettings(final int maxRetries,
             this.initialDelay = initialDelay;
             this.timerExecutor = timerExecutor;
             this.exponential = exponential;
-            this.jitter = jitter;
+            this.jitter = requireNonNull(jitter);
             this.maxDelay = maxDelay;
         }
 
@@ -315,7 +322,6 @@ public BiIntFunction<Throwable, Completable> newStrategy(final Executor alternat
             if (initialDelay == null) {
                 return (count, throwable) -> count <= maxRetries ? completed() : failed(throwable);
             } else {
-                assert jitter != null;
                 final Executor effectiveExecutor = timerExecutor == null ?
                         requireNonNull(alternativeTimerExecutor) : timerExecutor;
                 if (exponential) {

diff --git a/servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/RepeatStrategies.java b/servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/RepeatStrategies.java
@@ -20,6 +20,7 @@
 
 import static io.servicetalk.concurrent.api.Completable.failed;
 import static io.servicetalk.concurrent.api.RetryStrategies.baseDelayNanos;
+import static io.servicetalk.concurrent.api.RetryStrategies.checkFullJitter;
 import static io.servicetalk.concurrent.api.RetryStrategies.checkJitterDelta;
 import static io.servicetalk.concurrent.api.RetryStrategies.checkMaxRetries;
 import static io.servicetalk.concurrent.api.RetryStrategies.maxShift;
@@ -69,6 +70,7 @@ public static IntFunction<Completable> repeatWithConstantBackoffFullJitter(final
                                                                                final Executor timerExecutor) {
         requireNonNull(timerExecutor);
         final long delayNanos = delay.toNanos();
+        checkFullJitter(delayNanos);
         return repeatCount -> timerExecutor.timer(current().nextLong(0, delayNanos), NANOSECONDS);
     }
 
@@ -90,6 +92,7 @@ public static IntFunction<Completable> repeatWithConstantBackoffFullJitter(final
         checkMaxRetries(maxRepeats);
         requireNonNull(timerExecutor);
         final long delayNanos = delay.toNanos();
+        checkFullJitter(delayNanos);
         return repeatCount -> repeatCount <= maxRepeats ?
                 timerExecutor.timer(current().nextLong(0, delayNanos), NANOSECONDS) : terminateRepeat();
     }

diff --git a/servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/RetryStrategies.java b/servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/RetryStrategies.java
@@ -59,6 +59,7 @@ public static BiIntFunction<Throwable, Completable> retryWithConstantBackoffFull
         requireNonNull(timerExecutor);
         requireNonNull(causeFilter);
         final long delayNanos = delay.toNanos();
+        checkFullJitter(delayNanos);
         return (retryCount, cause) -> retryCount <= maxRetries && causeFilter.test(cause) ?
                 timerExecutor.timer(current().nextLong(0, delayNanos), NANOSECONDS) : failed(cause);
     }
@@ -82,6 +83,7 @@ public static BiIntFunction<Throwable, Completable> retryWithConstantBackoffFull
         requireNonNull(timerExecutor);
         requireNonNull(causeFilter);
         final long delayNanos = delay.toNanos();
+        checkFullJitter(delayNanos);
         return (retryCount, cause) -> causeFilter.test(cause) ?
                 timerExecutor.timer(current().nextLong(0, delayNanos), NANOSECONDS) : failed(cause);
     }
@@ -298,12 +300,18 @@ static void checkMaxRetries(final int maxRetries) {
     }
 
     static void checkJitterDelta(long jitterNanos, long delayNanos) {
-        if (jitterNanos > delayNanos || Long.MAX_VALUE - delayNanos < jitterNanos) {
+        if (jitterNanos >= delayNanos || Long.MAX_VALUE - delayNanos < jitterNanos) {
             throw new IllegalArgumentException("jitter " + jitterNanos +
                     "ns would result in [under|over]flow as a delta to delay " + delayNanos + "ns");
         }
     }
 
+    static void checkFullJitter(long jitterNanos) {
+        if (jitterNanos <= 0) {
+            throw new IllegalArgumentException("jitter " + jitterNanos + "ns must be >=0.");
+        }
+    }
+
     static long maxShift(long v) {
         return numberOfLeadingZeros(v) - 1;
     }

diff --git a/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ClientClosureRaceTest.java b/servicetalk-http-netty/src/test/java/io/servicetalk/http/netty/ClientClosureRaceTest.java
@@ -48,6 +48,7 @@
 import static io.servicetalk.http.netty.HttpProtocolConfigs.h1;
 import static java.net.InetAddress.getLoopbackAddress;
 import static java.nio.charset.StandardCharsets.US_ASCII;
+import static java.time.Duration.ofNanos;
 
 @Timeout(90)
 class ClientClosureRaceTest {
@@ -180,7 +181,6 @@ private SingleAddressHttpClientBuilder<HostAndPort, InetSocketAddress> newClient
                             // on the same connection which would result in a non-retryable exception. Since this test
                             // doesn't care about idempotency it should always retry.
                             true
-                        )
-                        .buildWithImmediateRetries());
+                        ).buildWithConstantBackoffFullJitter(ofNanos(1)));
     }
 }