Skip to content

Commit

Permalink
Fix exponential Backoff overflow/npe issues by using ms for cap (#282)
Browse files Browse the repository at this point in the history 
This commit changes the exponential backoff default maxBackoff to be
Long.MAX_VALUE _milliseconds_ instead of _seconds_. The limit still
represents over 292 million years, plenty enough for a backoff :)

This prevents overflow and arithmetic exceptions, notably when computing
a jitter on top. This upper limit is also enforced in the constructor
when explicitly passing in a `maxBackoff`.

This commit also fixes 2 additional issues:
 - the computed BackoffDelay could have a `delay` component greater than
 the `max`, which would result in a negative jitter higher bound when
 computing the jitter bounds (now prevented in Backoff.exponential)
 - the "depends on previous value" variant would not use the default
 maxBackoffInterval if provided maxBackoff parameter was null, leading
 to a NullPointerException.

Supersedes and closes #269.
  • Loading branch information
@simonbasle
simonbasle authored Mar 11, 2022
1 parent cceaba5 commit fc0caa2
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 7 deletions.
18 changes: 14 additions & 4 deletions reactor-extra/src/main/java/reactor/retry/Backoff.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017-2021 VMware Inc. or its affiliates, All Rights Reserved.
* Copyright (c) 2017-2022 VMware Inc. or its affiliates, All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -77,7 +77,7 @@ public String toString() {
* value will be different from the actual exponential value for the iteration.
*
* @param firstBackoff First backoff duration
* @param maxBackoff Maximum backoff duration
* @param maxBackoff Maximum backoff duration, capped to {@link Long#MAX_VALUE} milliseconds
* @param factor The multiplicand for calculating backoff
* @param basedOnPreviousValue If true, calculation is based on previous value which may
* be a backoff with jitter applied
Expand All @@ -86,7 +86,11 @@ public String toString() {
static Backoff exponential(Duration firstBackoff, @Nullable Duration maxBackoff, int factor, boolean basedOnPreviousValue) {
if (firstBackoff == null || firstBackoff.isNegative() || firstBackoff.isZero())
throw new IllegalArgumentException("firstBackoff must be > 0");
Duration maxBackoffInterval = maxBackoff != null ? maxBackoff : Duration.ofSeconds(Long.MAX_VALUE);
Duration cap = Duration.ofMillis(Long.MAX_VALUE);
if (maxBackoff != null && maxBackoff.compareTo(cap) > 0) {
throw new IllegalArgumentException("maxBackoff must be less than Long.MAX_VALUE milliseconds");
}
Duration maxBackoffInterval = maxBackoff != null && maxBackoff.compareTo(cap) < 0 ? maxBackoff : cap;
if (maxBackoffInterval.compareTo(firstBackoff) < 0)
throw new IllegalArgumentException("maxBackoff must be >= firstBackoff");
if (!basedOnPreviousValue) {
Expand All @@ -100,6 +104,9 @@ public BackoffDelay apply(IterationContext<?> context) {
else {
try {
nextBackoff = firstBackoff.multipliedBy((long) Math.pow(factor, (context.iteration() - 1)));
if (nextBackoff.compareTo(maxBackoffInterval) >= 0) {
nextBackoff = maxBackoffInterval;
}
}
catch (ArithmeticException e) {
nextBackoff = maxBackoffInterval;
Expand Down Expand Up @@ -128,12 +135,15 @@ public BackoffDelay apply(IterationContext<?> context) {
}
else try {
nextBackoff = prevBackoff.multipliedBy(factor);
if (nextBackoff.compareTo(maxBackoffInterval) >= 0) {
nextBackoff = maxBackoffInterval;
}
}
catch (ArithmeticException e) {
nextBackoff = maxBackoffInterval;
}
nextBackoff = nextBackoff.compareTo(firstBackoff) < 0 ? firstBackoff : nextBackoff;
return new BackoffDelay(firstBackoff, maxBackoff, nextBackoff);
return new BackoffDelay(firstBackoff, maxBackoffInterval, nextBackoff);
}

@Override
Expand Down
47 changes: 44 additions & 3 deletions reactor-extra/src/test/java/reactor/retry/BackoffTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2017-2021 VMware Inc. or its affiliates, All Rights Reserved.
* Copyright (c) 2017-2022 VMware Inc. or its affiliates, All Rights Reserved.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -75,13 +75,19 @@ public void exponentialDoesntThrowArithmeticException_explicitMax() {

BackoffDelay delay = null;
IterationContext<String> context = null;
//note: reaches max in about 15 iterations
for (int i = 0; i < 71; i++) {
if (i == 0) {
delay = new BackoffDelay(INIT, EXPLICIT_MAX, INIT);
}
else {
context = new DefaultContext<>(null, i, delay, null);
delay = backoff.apply(context);

RandomJitter jitter = new RandomJitter(0.5d);
assertThat(jitter.highJitterBound(delay, 250))
.as("jitter applied without exception in round #" + i)
.isNotNegative();
}
}

Expand All @@ -94,19 +100,25 @@ public void exponentialDoesntThrowArithmeticException_explicitMax() {
@Test
public void exponentialDoesntThrowArithmeticException_noSpecificMax() {
final Duration INIT = Duration.ofSeconds(10);
final Duration EXPECTED_MAX = Duration.ofSeconds(Long.MAX_VALUE);
final Duration EXPECTED_MAX = Duration.ofMillis(Long.MAX_VALUE);

Backoff backoff = Backoff.exponential(INIT, null, 2, false);

BackoffDelay delay = null;
IterationContext<String> context = null;
//note: reaches max in about 50 iterations
for (int i = 0; i < 71; i++) {
if (i == 0) {
delay = new BackoffDelay(INIT, null, INIT);
}
else {
context = new DefaultContext<>(null, i, delay, null);
delay = backoff.apply(context);

RandomJitter jitter = new RandomJitter(0.5d);
assertThat(jitter.highJitterBound(delay, 250))
.as("jitter applied without exception in round #" + i)
.isNotNegative();
}
}

Expand All @@ -125,13 +137,19 @@ public void exponentialDoesntThrowArithmeticException_explicitMaxDependsOnPrevio

BackoffDelay delay = null;
IterationContext<String> context = null;
//note: reaches max in about 15 iterations
for (int i = 0; i < 71; i++) {
if (i == 0) {
delay = new BackoffDelay(INIT, EXPLICIT_MAX, INIT);
}
else {
context = new DefaultContext<>(null, i, delay, null);
delay = backoff.apply(context);

RandomJitter jitter = new RandomJitter(0.5d);
assertThat(jitter.highJitterBound(delay, 250))
.as("jitter applied without exception in round #" + i)
.isNotNegative();
}
}

Expand All @@ -144,19 +162,25 @@ public void exponentialDoesntThrowArithmeticException_explicitMaxDependsOnPrevio
@Test
public void exponentialDoesntThrowArithmeticException_noSpecificMaxDependsOnPrevious() {
final Duration INIT = Duration.ofSeconds(10);
final Duration EXPECTED_MAX = Duration.ofSeconds(Long.MAX_VALUE);
final Duration EXPECTED_MAX = Duration.ofMillis(Long.MAX_VALUE);

Backoff backoff = Backoff.exponential(INIT, null, 2, true);

BackoffDelay delay = null;
IterationContext<String> context = null;
//note: reaches max in about 50 iterations
for (int i = 0; i < 71; i++) {
if (i == 0) {
delay = new BackoffDelay(INIT, null, INIT);
}
else {
context = new DefaultContext<>(null, i, delay, null);
delay = backoff.apply(context);

RandomJitter jitter = new RandomJitter(0.5d);
assertThat(jitter.highJitterBound(delay, 250))
.as("jitter applied without exception in round #" + i)
.isNotNegative();
}
}

Expand All @@ -179,6 +203,23 @@ public void exponentialRejectsMaxLowerThanFirst() {
.withMessage("maxBackoff must be >= firstBackoff");
}

@Test
public void exponentialRejectsMaxGreaterThanLongMaxMilliseconds() {
assertThatIllegalArgumentException()
.isThrownBy(() -> Backoff.exponential(Duration.ofSeconds(2),
Duration.ofMillis(Long.MAX_VALUE).plusMillis(10),
1, false))
.as("not based on previous value")
.withMessage("maxBackoff must be less than Long.MAX_VALUE milliseconds");

assertThatIllegalArgumentException()
.isThrownBy(() -> Backoff.exponential(Duration.ofSeconds(2),
Duration.ofMillis(Long.MAX_VALUE).plusMillis(10),
1, true))
.as("based on previous value")
.withMessage("maxBackoff must be less than Long.MAX_VALUE milliseconds");
}

@Test
public void exponentialAcceptsMaxEqualToFirst() {
assertThatCode(() -> Backoff.exponential(Duration.ofSeconds(1), Duration.ofSeconds(1), 1, false))
Expand Down

0 comments on commit fc0caa2

Please sign in to comment.