Skip to content

Commit cc88c19

Browse files
QuanzzzzQuanzzzz
committed
Fix overflow bugs in backoff strategy.
1 parent 38f54b2 commit cc88c19

File tree

6 files changed

+226
-7
lines changed

6 files changed

+226
-7
lines changed

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/retry/backoff/BackoffStrategy.java

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,12 @@
2424
@FunctionalInterface
2525
public interface BackoffStrategy {
2626

27+
/**
28+
* Max permitted retry times. To prevent exponentialDelay from overflow, there must be 2 ^ retriesAttempted
29+
* <= 2 ^ 31 - 1, which means retriesAttempted <= 30, so that is the ceil for retriesAttempted.
30+
*/
31+
int RETRIES_ATTEMPTED_CEILING = (int) Math.floor(Math.log(Integer.MAX_VALUE) / Math.log(2));
32+
2733
/**
2834
* Compute the delay before the next retry request. This strategy is only consulted when there will be a next retry.
2935
*
@@ -33,7 +39,8 @@ public interface BackoffStrategy {
3339
Duration computeDelayBeforeNextRetry(RetryPolicyContext context);
3440

3541
default int calculateExponentialDelay(int retriesAttempted, Duration baseDelay, Duration maxBackoffTime) {
36-
return (int) Math.min((1L << retriesAttempted) * baseDelay.toMillis(), maxBackoffTime.toMillis());
42+
int cappedRetries = Math.min(retriesAttempted, RETRIES_ATTEMPTED_CEILING);
43+
return (int) Math.min(baseDelay.multipliedBy(1L << cappedRetries).toMillis(), maxBackoffTime.toMillis());
3744
}
3845

3946
static BackoffStrategy defaultStrategy() {

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/retry/backoff/EqualJitterBackoffStrategy.java

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
package software.amazon.awssdk.core.retry.backoff;
1717

18+
import static software.amazon.awssdk.utils.NumericUtils.min;
1819
import static software.amazon.awssdk.utils.Validate.isNotNegative;
1920

2021
import java.time.Duration;
@@ -44,13 +45,21 @@ public final class EqualJitterBackoffStrategy implements BackoffStrategy,
4445
ToCopyableBuilder<EqualJitterBackoffStrategy.Builder,
4546
EqualJitterBackoffStrategy> {
4647

48+
private static final Duration BASE_DELAY_CEILING = Duration.ofMillis(Integer.MAX_VALUE); // Around 24 days
49+
private static final Duration MAX_BACKOFF_CEILING = Duration.ofMillis(Integer.MAX_VALUE); // Around 24 days
50+
4751
private final Duration baseDelay;
4852
private final Duration maxBackoffTime;
49-
private final Random random = new Random();
53+
private final Random random;
5054

5155
private EqualJitterBackoffStrategy(BuilderImpl builder) {
52-
this.baseDelay = isNotNegative(builder.baseDelay, "baseDelay");
53-
this.maxBackoffTime = isNotNegative(builder.maxBackoffTime, "maxBackoffTime");
56+
this(builder.baseDelay, builder.maxBackoffTime, new Random());
57+
}
58+
59+
EqualJitterBackoffStrategy(final Duration baseDelay, final Duration maxBackoffTime, final Random random) {
60+
this.baseDelay = min(isNotNegative(baseDelay, "baseDelay"), BASE_DELAY_CEILING);
61+
this.maxBackoffTime = min(isNotNegative(maxBackoffTime, "maxBackoffTime"), MAX_BACKOFF_CEILING);
62+
this.random = random;
5463
}
5564

5665
@Override

‎core/sdk-core/src/main/java/software/amazon/awssdk/core/retry/backoff/FullJitterBackoffStrategy.java

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
package software.amazon.awssdk.core.retry.backoff;
1717

18+
import static software.amazon.awssdk.utils.NumericUtils.min;
1819
import static software.amazon.awssdk.utils.Validate.isNotNegative;
1920

2021
import java.time.Duration;
@@ -40,14 +41,21 @@
4041
public final class FullJitterBackoffStrategy implements BackoffStrategy,
4142
ToCopyableBuilder<FullJitterBackoffStrategy.Builder,
4243
FullJitterBackoffStrategy> {
44+
private static final Duration BASE_DELAY_CEILING = Duration.ofMillis(Integer.MAX_VALUE); // Around 24 days
45+
private static final Duration MAX_BACKOFF_CEILING = Duration.ofMillis(Integer.MAX_VALUE); // Around 24 days
4346

4447
private final Duration baseDelay;
4548
private final Duration maxBackoffTime;
46-
private final Random random = new Random();
49+
private final Random random;
4750

4851
private FullJitterBackoffStrategy(BuilderImpl builder) {
49-
this.baseDelay = isNotNegative(builder.baseDelay, "baseDelay");
50-
this.maxBackoffTime = isNotNegative(builder.maxBackoffTime, "maxBackoffTime");
52+
this(builder.baseDelay, builder.maxBackoffTime, new Random());
53+
}
54+
55+
FullJitterBackoffStrategy(final Duration baseDelay, final Duration maxBackoffTime, final Random random) {
56+
this.baseDelay = min(isNotNegative(baseDelay, "baseDelay"), BASE_DELAY_CEILING);
57+
this.maxBackoffTime = min(isNotNegative(maxBackoffTime, "maxBackoffTime"), MAX_BACKOFF_CEILING);
58+
this.random = random;
5159
}
5260

5361
@Override

‎core/sdk-core/src/test/java/software/amazon/awssdk/core/retry/backoff/EqualJitterBackoffStrategyTest.java

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,116 @@
1+
/*
2+
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License").
5+
* You may not use this file except in compliance with the License.
6+
* A copy of the License is located at
7+
*
8+
* http://aws.amazon.com/apache2.0
9+
*
10+
* or in the "license" file accompanying this file. This file is distributed
11+
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either
12+
* express or implied. See the License for the specific language governing
13+
* permissions and limitations under the License.
14+
*/
15+
16+
package software.amazon.awssdk.core.retry.backoff;
17+
18+
import org.junit.Test;
19+
import org.mockito.Mock;
20+
import software.amazon.awssdk.core.retry.RetryPolicyContext;
21+
22+
import java.time.Duration;
23+
import java.util.Random;
24+
25+
import static org.hamcrest.Matchers.is;
26+
import static org.junit.Assert.*;
27+
import static org.mockito.Mockito.mock;
28+
import static org.mockito.Mockito.when;
29+
import static org.testng.Assert.assertThrows;
30+
31+
public class EqualJitterBackoffStrategyTest {
32+
33+
private static final int RANDOM_RESULT = 12345;
34+
private static final Duration FIVE_DAYS = Duration.ofDays(5);
35+
private static final Duration MAX_DURATION = Duration.ofSeconds(Long.MAX_VALUE);
36+
private static final Duration ONE_SECOND = Duration.ofSeconds(1);
37+
private static final Duration ONE_NANO_SECOND = Duration.ofNanos(1);
38+
private static final int NANO_IN_MILLISECONDS = 1_000_000;
39+
private static final Duration NEGATIVE_ONE_SECOND = Duration.ofSeconds(-1);
40+
41+
@Mock
42+
private Random mockRandom = mock(Random.class);
43+
44+
@Test
45+
public void exponentialDelayOverflowWithMaxBackoffTest() {
46+
test(FIVE_DAYS, MAX_DURATION, 3, Integer.MAX_VALUE);
47+
}
48+
49+
@Test
50+
public void exponentialDelayOverflowWithMaxBaseDelayTest() {
51+
test(MAX_DURATION, MAX_DURATION, 1, Integer.MAX_VALUE);
52+
}
53+
54+
@Test
55+
public void maxBaseDelayShortBackoffTest() {
56+
test(MAX_DURATION, ONE_SECOND, 1, (int) ONE_SECOND.toMillis());
57+
}
58+
59+
@Test
60+
public void normalConditionTest() {
61+
test(ONE_SECOND, MAX_DURATION, 10, (1 << 10) * (int) ONE_SECOND.toMillis());
62+
}
63+
64+
@Test
65+
public void tinyBackoffNormalRetriesTest() {
66+
test(MAX_DURATION, ONE_NANO_SECOND, 10, 0);
67+
}
68+
69+
@Test
70+
public void tinyBaseDelayNormalRetriesTest() {
71+
test(ONE_NANO_SECOND, MAX_DURATION, 30, (int) (1L << 30) / NANO_IN_MILLISECONDS);
72+
}
73+
74+
@Test
75+
public void tinyBaseDelayExtraRetriesTest() {
76+
test(ONE_NANO_SECOND, MAX_DURATION, 100,
77+
(int) (1L << 30) / NANO_IN_MILLISECONDS); // RETRIES_ATTEMPTED_CEILING == 30
78+
}
79+
80+
@Test
81+
public void exponentialDelayOverflowWithExtraRetriesTest() {
82+
test(MAX_DURATION, MAX_DURATION, 100, Integer.MAX_VALUE);
83+
}
84+
85+
@Test
86+
public void tinyBaseDelayUnderflowTest() {
87+
test(ONE_NANO_SECOND, MAX_DURATION, 0, 0);
88+
}
89+
90+
@Test
91+
public void negativeBaseDelayTest() {
92+
assertThrows(IllegalArgumentException.class, () ->
93+
test(NEGATIVE_ONE_SECOND, MAX_DURATION, 1, 0));
94+
}
95+
96+
@Test
97+
public void negativeBackoffTest() {
98+
assertThrows(IllegalArgumentException.class, () ->
99+
test(ONE_SECOND, NEGATIVE_ONE_SECOND, 1, 0));
100+
}
101+
102+
private void test(final Duration baseDelay, final Duration maxBackoffTime,
103+
final int retriesAttempted, final int expectedCeilingMillis) {
104+
final BackoffStrategy backoffStrategy = new EqualJitterBackoffStrategy(baseDelay, maxBackoffTime, mockRandom);
105+
106+
when(mockRandom.nextInt(expectedCeilingMillis /2 + 1)).thenReturn(RANDOM_RESULT);
107+
108+
assertThat(backoffStrategy.computeDelayBeforeNextRetry(toRetryContext(retriesAttempted)),
109+
is(Duration.ofMillis(expectedCeilingMillis / 2 + RANDOM_RESULT)));
110+
}
111+
112+
private static RetryPolicyContext toRetryContext(final int retriesAttempted) {
113+
return RetryPolicyContext.builder().retriesAttempted(retriesAttempted).build();
114+
}
115+
116+
}

‎utils/src/main/java/software/amazon/awssdk/utils/NumericUtils.java

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515

1616
package software.amazon.awssdk.utils;
1717

18+
import java.time.Duration;
1819
import software.amazon.awssdk.annotations.SdkProtectedApi;
1920

2021
@SdkProtectedApi
@@ -41,4 +42,12 @@ public static int saturatedCast(long value) {
4142
return (int) value;
4243
}
4344

45+
public static Duration min(Duration a, Duration b) {
46+
return (a.compareTo(b) < 0) ? a : b;
47+
}
48+
49+
public static Duration max(Duration a, Duration b) {
50+
return (a.compareTo(b) > 0) ? a : b;
51+
}
52+
4453
}

‎utils/src/test/java/software/amazon/awssdk/utils/NumericUtilsTest.java

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
package software.amazon.awssdk.utils;
2+
3+
import java.time.Duration;
4+
5+
import org.junit.Test;
6+
7+
import static org.hamcrest.CoreMatchers.is;
8+
import static org.junit.Assert.*;
9+
import static software.amazon.awssdk.utils.NumericUtils.max;
10+
import static software.amazon.awssdk.utils.NumericUtils.min;
11+
12+
public class NumericUtilsTest {
13+
14+
private final Duration SHORT_DURATION = Duration.ofMillis(10);
15+
private final Duration SHORT_SAME_DURATION = Duration.ofMillis(10);
16+
private final Duration LONG_DURATION = Duration.ofMillis(100);
17+
private final Duration NEGATIVE_SHORT_DURATION = Duration.ofMillis(-10);
18+
private final Duration NEGATIVE_SHORT_SAME_DURATION = Duration.ofMillis(-10);
19+
private final Duration NEGATIVE_LONG_DURATION = Duration.ofMillis(-100);
20+
21+
@Test
22+
public void minTestDifferentDurations() {
23+
assertThat(min(SHORT_DURATION, LONG_DURATION), is(SHORT_DURATION));
24+
}
25+
26+
@Test
27+
public void minTestDifferentDurationsReverse() {
28+
assertThat(min(LONG_DURATION, SHORT_DURATION), is(SHORT_DURATION));
29+
}
30+
31+
@Test
32+
public void minTestSameDurations() {
33+
assertThat(min(SHORT_DURATION, SHORT_SAME_DURATION), is(SHORT_SAME_DURATION));
34+
}
35+
36+
@Test
37+
public void minTestDifferentNegativeDurations() {
38+
assertThat(min(NEGATIVE_SHORT_DURATION, NEGATIVE_LONG_DURATION), is(NEGATIVE_LONG_DURATION));
39+
}
40+
41+
@Test
42+
public void minTestNegativeSameDurations() {
43+
assertThat(min(NEGATIVE_SHORT_DURATION, NEGATIVE_SHORT_SAME_DURATION), is(NEGATIVE_SHORT_DURATION));
44+
}
45+
46+
@Test
47+
public void maxTestDifferentDurations() {
48+
assertThat(max(LONG_DURATION, SHORT_DURATION), is(LONG_DURATION));
49+
}
50+
51+
@Test
52+
public void maxTestDifferentDurationsReverse() {
53+
assertThat(max(SHORT_DURATION, LONG_DURATION), is(LONG_DURATION));
54+
}
55+
56+
@Test
57+
public void maxTestSameDurations() {
58+
assertThat(max(SHORT_DURATION, SHORT_SAME_DURATION), is(SHORT_SAME_DURATION));
59+
}
60+
61+
@Test
62+
public void maxTestDifferentNegativeDurations() {
63+
assertThat(max(NEGATIVE_SHORT_DURATION, NEGATIVE_LONG_DURATION), is(NEGATIVE_SHORT_DURATION));
64+
}
65+
66+
@Test
67+
public void maxTestNegativeSameDurations() {
68+
assertThat(max(NEGATIVE_SHORT_DURATION, NEGATIVE_SHORT_SAME_DURATION), is(NEGATIVE_SHORT_DURATION));
69+
}
70+
}

0 commit comments

Comments
 (0)