Skip to content

Commit

Permalink
concurrent-api: fix IllegalArgumentExcetion for zero jitter in RetryS…
Browse files Browse the repository at this point in the history 
…trategies (#2777)

Motivation:

If you pass in a jitter of 0 for the retry strategies with delta delay you'll
get an `IllegalArgumentException` thrown by the
`Random.nextLong(lower, upper)` method because the lower and
upper are the same value. This is because the upper bound argument
is non-inclusive.

Modifications:

Add 1 to each call to extend the range by 1 nanosecond and make the
full delay range selectable. We also do this in the case of the
`*FullJitter` methods to make the full delay an option instead of being
limited to `delay - 1ns`.

Result:

Less illegal argument exceptions and (technically, although admittedly
not practically important) correct delay ranges.
  • Loading branch information
@bryce-anderson
bryce-anderson committed Dec 8, 2023
1 parent 1d00f69 commit cf40cb3
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 17 deletions.
47 changes: 30 additions & 17 deletions servicetalk-concurrent-api/src/main/java/io/servicetalk/concurrent/api/RetryStrategies.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,10 +58,12 @@ public static BiIntFunction<Throwable, Completable> retryWithConstantBackoffFull
checkMaxRetries(maxRetries);
requireNonNull(timerExecutor);
requireNonNull(causeFilter);
final long delayNanos = delay.toNanos();
checkFullJitter(delayNanos);
final long jitterNanos = delay.toNanos();
checkFullJitter(jitterNanos);
// add 1 because upper bound is non-inclusive.
final long upperBound = addWithOverflowProtection(jitterNanos, 1);
return (retryCount, cause) -> retryCount <= maxRetries && causeFilter.test(cause) ?
timerExecutor.timer(current().nextLong(0, delayNanos), NANOSECONDS) : failed(cause);
timerExecutor.timer(current().nextLong(0, upperBound), NANOSECONDS) : failed(cause);
}

/**
Expand All @@ -82,10 +84,12 @@ public static BiIntFunction<Throwable, Completable> retryWithConstantBackoffFull
final Executor timerExecutor) {
requireNonNull(timerExecutor);
requireNonNull(causeFilter);
final long delayNanos = delay.toNanos();
checkFullJitter(delayNanos);
final long jitterNanos = delay.toNanos();
checkFullJitter(jitterNanos);
// add 1 because upper bound is non-inclusive.
final long upperBound = addWithOverflowProtection(jitterNanos, 1);
return (retryCount, cause) -> causeFilter.test(cause) ?
timerExecutor.timer(current().nextLong(0, delayNanos), NANOSECONDS) : failed(cause);
timerExecutor.timer(current().nextLong(0, upperBound), NANOSECONDS) : failed(cause);
}

/**
Expand All @@ -110,7 +114,8 @@ public static BiIntFunction<Throwable, Completable> retryWithConstantBackoffDelt
final long jitterNanos = jitter.toNanos();
checkJitterDelta(jitterNanos, delayNanos);
final long lowerBound = delayNanos - jitterNanos;
final long upperBound = delayNanos + jitterNanos;
// Add 1 because the upper bound is non-inclusive.
final long upperBound = addWithOverflowProtection(addWithOverflowProtection(delayNanos, jitterNanos), 1);
return (retryCount, cause) -> causeFilter.test(cause) ?
timerExecutor.timer(current().nextLong(lowerBound, upperBound), NANOSECONDS) : failed(cause);
}
Expand Down Expand Up @@ -141,7 +146,8 @@ public static BiIntFunction<Throwable, Completable> retryWithConstantBackoffDelt
final long jitterNanos = jitter.toNanos();
checkJitterDelta(jitterNanos, delayNanos);
final long lowerBound = delayNanos - jitterNanos;
final long upperBound = delayNanos + jitterNanos;
// Add 1 because the upper bound is non-inclusive.
final long upperBound = addWithOverflowProtection(addWithOverflowProtection(delayNanos, jitterNanos), 1);
return (retryCount, cause) -> retryCount <= maxRetries && causeFilter.test(cause) ?
timerExecutor.timer(current().nextLong(lowerBound, upperBound), NANOSECONDS) : failed(cause);
}
Expand Down Expand Up @@ -172,8 +178,10 @@ public static BiIntFunction<Throwable, Completable> retryWithExponentialBackoffF
final long maxInitialShift = maxShift(initialDelayNanos);
return (retryCount, cause) -> causeFilter.test(cause) ?
timerExecutor.timer(current().nextLong(0,
baseDelayNanos(initialDelayNanos, maxDelayNanos, maxInitialShift, retryCount)), NANOSECONDS) :
failed(cause);
// Add 1 because the upper bound is non-inclusive.
addWithOverflowProtection(baseDelayNanos(initialDelayNanos, maxDelayNanos, maxInitialShift,
retryCount), 1)), NANOSECONDS)
: failed(cause);
}

/**
Expand Down Expand Up @@ -206,8 +214,10 @@ public static BiIntFunction<Throwable, Completable> retryWithExponentialBackoffF
final long maxInitialShift = maxShift(initialDelayNanos);
return (retryCount, cause) -> retryCount <= maxRetries && causeFilter.test(cause) ?
timerExecutor.timer(current().nextLong(0,
baseDelayNanos(initialDelayNanos, maxDelayNanos, maxInitialShift, retryCount)), NANOSECONDS) :
failed(cause);
// Add 1 because the upper bound is non-inclusive.
addWithOverflowProtection(baseDelayNanos(initialDelayNanos, maxDelayNanos, maxInitialShift,
retryCount), 1)), NANOSECONDS)
: failed(cause);
}

/**
Expand Down Expand Up @@ -240,10 +250,11 @@ public static BiIntFunction<Throwable, Completable> retryWithExponentialBackoffD
return failed(cause);
}
final long baseDelayNanos = baseDelayNanos(initialDelayNanos, maxDelayNanos, maxInitialShift, retryCount);
return timerExecutor.timer(
current().nextLong(max(0, baseDelayNanos - jitterNanos),
min(maxDelayNanos, addWithOverflowProtection(baseDelayNanos, jitterNanos))),
NANOSECONDS);
final long lowerBound = max(0, baseDelayNanos - jitterNanos);
// Add 1 because the upper bound is non-inclusive.
final long upperBound = addWithOverflowProtection(
min(maxDelayNanos, addWithOverflowProtection(baseDelayNanos, jitterNanos)), 1);
return timerExecutor.timer(current().nextLong(lowerBound, upperBound), NANOSECONDS);
};
}

Expand Down Expand Up @@ -283,7 +294,9 @@ public static BiIntFunction<Throwable, Completable> retryWithExponentialBackoffD
final long baseDelayNanos = baseDelayNanos(initialDelayNanos, maxDelayNanos, maxInitialShift, retryCount);
return timerExecutor.timer(
current().nextLong(max(0, baseDelayNanos - jitterNanos),
min(maxDelayNanos, addWithOverflowProtection(baseDelayNanos, jitterNanos))),
// Add 1 because the upper bound is non-inclusive.
addWithOverflowProtection(
min(maxDelayNanos, addWithOverflowProtection(baseDelayNanos, jitterNanos)), 1)),
NANOSECONDS);
};
}
Expand Down
14 changes: 14 additions & 0 deletions ...etalk-concurrent-api/src/test/java/io/servicetalk/concurrent/api/RetryStrategiesTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,20 @@ void testBackoffWithJitter() throws Exception {
verifyNoMoreInteractions(timerExecutor);
}

@Test
void testBackoffWith0Jitter() throws Exception {
Duration backoff = ofSeconds(1);
Duration jitter = ofMillis(0);
RetryStrategy strategy = new RetryStrategy(retryWithConstantBackoffDeltaJitter(2, cause -> true,
backoff, jitter, timerExecutor));
io.servicetalk.concurrent.test.internal.TestCompletableSubscriber subscriber =
strategy.invokeAndListen(DELIBERATE_EXCEPTION);
verifyDelayWithDeltaJitter(backoff.toNanos(), jitter.toNanos(), 1);
timers.take().verifyListenCalled().onComplete();
subscriber.awaitOnComplete();
verifyNoMoreInteractions(timerExecutor);
}

@Test
void testBackoffMaxRetries() throws Exception {
Duration backoff = ofSeconds(1);
Expand Down

0 comments on commit cf40cb3

Please sign in to comment.