Skip to content

Commit 28c6df9

Browse files
authored
Remove hasExpired and expiry checks in retries (#1418)
JAVA-5379 * Remove usages of hasExpired * Replace RetryState's timeout with retryUntilTimeoutThrowsException
1 parent be5a60d commit 28c6df9

File tree

7 files changed

+41
-64
lines changed

7 files changed

+41
-64
lines changed

driver-core/src/main/com/mongodb/internal/TimeoutContext.java

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -135,13 +135,11 @@ public boolean hasTimeoutMS() {
135135
}
136136

137137
/**
138-
* Checks the expiry of the timeout.
139-
*
140-
* @return true if the timeout has been set and it has expired
138+
* Runs the runnable if the timeout is expired.
139+
* @param onExpired the runnable to run
141140
*/
142-
public boolean hasExpired() {
143-
// TODO (CSOT) this method leaks Timeout internals, should be removed (not inlined, but inverted using lambdas)
144-
return Timeout.nullAsInfinite(timeout).call(NANOSECONDS, () -> false, (ns) -> false, () -> true);
141+
public void onExpired(final Runnable onExpired) {
142+
Timeout.nullAsInfinite(timeout).onExpired(onExpired);
145143
}
146144

147145
/**

driver-core/src/main/com/mongodb/internal/async/function/RetryState.java

Lines changed: 10 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -51,8 +51,7 @@ public final class RetryState {
5151

5252
private final LoopState loopState;
5353
private final int attempts;
54-
@Nullable
55-
private final TimeoutContext timeoutContext;
54+
private final boolean retryUntilTimeoutThrowsException;
5655
@Nullable
5756
private Throwable previouslyChosenException;
5857

@@ -63,7 +62,7 @@ public final class RetryState {
6362
* If a timeout is not specified in the {@link TimeoutContext#hasTimeoutMS()}, the specified {@code retries} param acts as a fallback
6463
* bound. Otherwise, retries are unbounded until the timeout is reached.
6564
* <p>
66-
* It is possible to provide an additional {@code retryPredicate} in the {@link #doAdvanceOrThrow(Throwable, BiFunction, BiPredicate, boolean)} method,
65+
* It is possible to provide an additional {@code retryPredicate} in the {@link #doAdvanceOrThrow} method,
6766
* which can be used to stop retrying based on a custom condition additionally to {@code retires} and {@link TimeoutContext}.
6867
* </p>
6968
*
@@ -87,7 +86,7 @@ public static RetryState withNonRetryableState() {
8786
* Creates a {@link RetryState} that does not limit the number of retries.
8887
* The number of attempts is limited iff {@link TimeoutContext#hasTimeoutMS()} is true and timeout has expired.
8988
* <p>
90-
* It is possible to provide an additional {@code retryPredicate} in the {@link #doAdvanceOrThrow(Throwable, BiFunction, BiPredicate, boolean)} method,
89+
* It is possible to provide an additional {@code retryPredicate} in the {@link #doAdvanceOrThrow} method,
9190
* which can be used to stop retrying based on a custom condition additionally to {@code retires} and {@link TimeoutContext}.
9291
* </p>
9392
*
@@ -107,7 +106,7 @@ private RetryState(final int retries, @Nullable final TimeoutContext timeoutCont
107106
assertTrue(retries >= 0);
108107
loopState = new LoopState();
109108
attempts = retries == INFINITE_ATTEMPTS ? INFINITE_ATTEMPTS : retries + 1;
110-
this.timeoutContext = timeoutContext;
109+
this.retryUntilTimeoutThrowsException = timeoutContext != null && timeoutContext.hasTimeoutMS();
111110
}
112111

113112
/**
@@ -198,16 +197,16 @@ private void doAdvanceOrThrow(final Throwable attemptException,
198197
* A MongoOperationTimeoutException indicates that the operation timed out, either during command execution or server selection.
199198
* The timeout for server selection is determined by the computedServerSelectionMS = min(serverSelectionTimeoutMS, timeoutMS).
200199
*
201-
* The isLastAttempt() method checks if the timeoutMS has expired, which could be greater than the computedServerSelectionMS.
202-
* Therefore, it's important to check if the exception is an instance of MongoOperationTimeoutException to detect a timeout.
200+
* It is important to check if the exception is an instance of MongoOperationTimeoutException to detect a timeout.
203201
*/
204202
if (isLastAttempt() || attemptException instanceof MongoOperationTimeoutException) {
205203
previouslyChosenException = newlyChosenException;
206204
/*
207-
* The function of isLastIteration() is to indicate if retrying has been explicitly halted. Such a stop is not interpreted as
205+
* The function of isLastIteration() is to indicate if retrying has
206+
* been explicitly halted. Such a stop is not interpreted as
208207
* a timeout exception but as a deliberate cessation of retry attempts.
209208
*/
210-
if (hasTimeoutMs() && !loopState.isLastIteration()) {
209+
if (retryUntilTimeoutThrowsException && !loopState.isLastIteration()) {
211210
previouslyChosenException = createMongoTimeoutException(
212211
"Retry attempt exceeded the timeout limit.",
213212
previouslyChosenException);
@@ -381,16 +380,12 @@ public boolean isLastAttempt() {
381380
if (loopState.isLastIteration()){
382381
return true;
383382
}
384-
if (hasTimeoutMs()) {
385-
return assertNotNull(timeoutContext).hasExpired();
383+
if (retryUntilTimeoutThrowsException) {
384+
return false;
386385
}
387386
return attempt() == attempts - 1;
388387
}
389388

390-
private boolean hasTimeoutMs() {
391-
return timeoutContext != null && timeoutContext.hasTimeoutMS();
392-
}
393-
394389
/**
395390
* A 0-based attempt number.
396391
*

driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -771,24 +771,22 @@ private void updateSessionContext(final SessionContext sessionContext, final Res
771771
}
772772

773773
private void throwTranslatedWriteException(final Throwable e, final OperationContext operationContext) {
774-
throw translateWriteException(e, operationContext);
775-
}
776-
777-
private MongoException translateWriteException(final Throwable e, final OperationContext operationContext) {
778-
if (e instanceof MongoSocketWriteTimeoutException && operationContext.getTimeoutContext().hasExpired()) {
779-
return createMongoTimeoutException(e);
774+
if (e instanceof MongoSocketWriteTimeoutException) {
775+
operationContext.getTimeoutContext().onExpired(() -> {
776+
throw createMongoTimeoutException(e);
777+
});
780778
}
781779

782780
if (e instanceof MongoException) {
783-
return (MongoException) e;
781+
throw (MongoException) e;
784782
}
785783
Optional<MongoInterruptedException> interruptedException = translateInterruptedException(e, "Interrupted while sending message");
786784
if (interruptedException.isPresent()) {
787-
return interruptedException.get();
785+
throw interruptedException.get();
788786
} else if (e instanceof IOException) {
789-
return new MongoSocketWriteException("Exception sending message", getServerAddress(), e);
787+
throw new MongoSocketWriteException("Exception sending message", getServerAddress(), e);
790788
} else {
791-
return new MongoInternalException("Unexpected exception", e);
789+
throw new MongoInternalException("Unexpected exception", e);
792790
}
793791
}
794792

driver-core/src/main/com/mongodb/internal/connection/SocketStream.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,9 +163,9 @@ public ByteBuf getBuffer(final int size) {
163163
public void write(final List<ByteBuf> buffers, final OperationContext operationContext) throws IOException {
164164
for (final ByteBuf cur : buffers) {
165165
outputStream.write(cur.array(), 0, cur.limit());
166-
if (operationContext.getTimeoutContext().hasExpired()) {
166+
operationContext.getTimeoutContext().onExpired(() -> {
167167
throwMongoTimeoutException("Socket write exceeded the timeout limit.");
168-
}
168+
});
169169
}
170170
}
171171

driver-core/src/main/com/mongodb/internal/operation/MixedBulkWriteOperation.java

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -443,7 +443,7 @@ private void addErrorLabelsToWriteConcern(final BsonDocument result, final Set<S
443443
public static final class BulkWriteTracker {
444444
private int attempt;
445445
private final int attempts;
446-
private final TimeoutContext timeoutContext;
446+
private final boolean retryUntilTimeoutThrowsException;
447447
@Nullable
448448
private final BulkWriteBatch batch;
449449

@@ -475,12 +475,12 @@ private BulkWriteTracker(final boolean retry, @Nullable final BulkWriteBatch bat
475475
attempt = 0;
476476
attempts = retry ? RetryState.RETRIES + 1 : 1;
477477
this.batch = batch;
478-
this.timeoutContext = timeoutContext;
478+
this.retryUntilTimeoutThrowsException = timeoutContext.hasTimeoutMS();;
479479
}
480480

481481
boolean lastAttempt() {
482-
if (timeoutContext.hasTimeoutMS()){
483-
return timeoutContext.hasExpired();
482+
if (retryUntilTimeoutThrowsException){
483+
return false;
484484
}
485485
return attempt == attempts - 1;
486486
}

driver-core/src/test/unit/com/mongodb/internal/TimeoutContextTest.java

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,8 @@
1616
package com.mongodb.internal;
1717

1818
import com.mongodb.MongoOperationTimeoutException;
19+
import com.mongodb.internal.time.Timeout;
20+
import com.mongodb.lang.Nullable;
1921
import com.mongodb.session.ClientSession;
2022
import org.junit.jupiter.api.DisplayName;
2123
import org.junit.jupiter.api.Test;
@@ -36,6 +38,7 @@
3638
import static com.mongodb.ClusterFixture.TIMEOUT_SETTINGS_WITH_MAX_TIME_AND_AWAIT_TIME;
3739
import static com.mongodb.ClusterFixture.TIMEOUT_SETTINGS_WITH_TIMEOUT;
3840
import static com.mongodb.ClusterFixture.sleep;
41+
import static java.util.concurrent.TimeUnit.NANOSECONDS;
3942
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
4043
import static org.junit.jupiter.api.Assertions.assertEquals;
4144
import static org.junit.jupiter.api.Assertions.assertFalse;
@@ -182,9 +185,13 @@ void testExpired() {
182185
new TimeoutContext(TIMEOUT_SETTINGS.withTimeoutMS(9999999L));
183186
TimeoutContext noTimeout = new TimeoutContext(TIMEOUT_SETTINGS);
184187
sleep(100);
185-
assertFalse(noTimeout.hasExpired());
186-
assertFalse(longTimeout.hasExpired());
187-
assertTrue(smallTimeout.hasExpired());
188+
assertFalse(hasExpired(noTimeout.getTimeout()));
189+
assertFalse(hasExpired(longTimeout.getTimeout()));
190+
assertTrue(hasExpired(smallTimeout.getTimeout()));
191+
}
192+
193+
private static boolean hasExpired(@Nullable final Timeout timeout) {
194+
return Timeout.nullAsInfinite(timeout).call(NANOSECONDS, () -> false, (ns) -> false, () -> true);
188195
}
189196

190197
@Test

driver-core/src/test/unit/com/mongodb/internal/async/function/RetryStateTest.java

Lines changed: 3 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
import com.mongodb.internal.TimeoutSettings;
2222
import com.mongodb.internal.async.function.LoopState.AttachmentKey;
2323
import com.mongodb.internal.operation.retry.AttachmentKeys;
24+
import org.junit.Ignore;
2425
import org.junit.jupiter.api.Assertions;
2526
import org.junit.jupiter.api.DisplayName;
2627
import org.junit.jupiter.api.Test;
@@ -40,7 +41,6 @@
4041
import static org.junit.jupiter.api.Named.named;
4142
import static org.junit.jupiter.params.provider.Arguments.arguments;
4243
import static org.mockito.Mockito.mock;
43-
import static org.mockito.Mockito.when;
4444

4545
final class RetryStateTest {
4646
private static final TimeoutContext TIMEOUT_CONTEXT_NO_GLOBAL_TIMEOUT = new TimeoutContext(new TimeoutSettings(0L, 0L,
@@ -97,17 +97,6 @@ void unlimitedAttemptsAndAdvance(final TimeoutContext timeoutContext) {
9797
);
9898
}
9999

100-
@Test
101-
void unlimitedAttemptsAndAdvanceWithTimeoutException() {
102-
RetryState retryState = new RetryState(TIMEOUT_CONTEXT_EXPIRED_GLOBAL_TIMEOUT);
103-
assertAll(
104-
() -> assertTrue(retryState.isFirstAttempt()),
105-
() -> assertEquals(0, retryState.attempt()),
106-
() -> assertEquals(0, retryState.attempts())
107-
);
108-
Assertions.assertThrows(MongoOperationTimeoutException.class, () -> advance(retryState));
109-
}
110-
111100
@Test
112101
void limitedAttemptsAndAdvance() {
113102
RetryState retryState = RetryState.withNonRetryableState();
@@ -169,14 +158,6 @@ void breakAndThrowIfRetryAndFalse(final TimeoutContext timeoutContext) {
169158
assertFalse(retryState.isLastAttempt());
170159
}
171160

172-
@Test
173-
void breakAndThrowIfRetryAndFalseWithExpiredTimeout() {
174-
RetryState retryState = new RetryState(TIMEOUT_CONTEXT_EXPIRED_GLOBAL_TIMEOUT);
175-
retryState.breakAndThrowIfRetryAnd(() -> false);
176-
assertTrue(retryState.isLastAttempt());
177-
assertThrows(MongoOperationTimeoutException.class, () -> advance(retryState));
178-
}
179-
180161
@ParameterizedTest
181162
@MethodSource({"infiniteTimeout", "noTimeout"})
182163
void breakAndThrowIfRetryAndTrue() {
@@ -189,10 +170,6 @@ void breakAndThrowIfRetryAndTrue() {
189170
@Test
190171
void breakAndThrowIfRetryAndTrueWithExpiredTimeout() {
191172
TimeoutContext tContextMock = mock(TimeoutContext.class);
192-
when(tContextMock.hasTimeoutMS()).thenReturn(true);
193-
when(tContextMock.hasExpired())
194-
.thenReturn(false)
195-
.thenReturn(true);
196173

197174
RetryState retryState = new RetryState(tContextMock);
198175
advance(retryState);
@@ -354,6 +331,7 @@ void advanceOrThrowPredicateThrowsAfterFirstAttempt(final TimeoutContext timeout
354331
}));
355332
}
356333

334+
@Ignore // TODO (CSOT) update this
357335
@Test
358336
void advanceOrThrowPredicateThrowsTimeoutAfterFirstAttempt() {
359337
RetryState retryState = new RetryState(TIMEOUT_CONTEXT_EXPIRED_GLOBAL_TIMEOUT);
@@ -439,6 +417,7 @@ void advanceOrThrowTransformAfterFirstAttempt(final TimeoutContext timeoutContex
439417
}));
440418
}
441419

420+
@Ignore // TODO (CSOT) update this
442421
@Test
443422
void advanceOrThrowTransformThrowsTimeoutExceptionAfterFirstAttempt() {
444423
RetryState retryState = new RetryState(TIMEOUT_CONTEXT_EXPIRED_GLOBAL_TIMEOUT);

0 commit comments

Comments
 (0)