Skip to content

Commit 8c0febd

Browse files
katcharovkatcharov
committed
Replace RetryState's timeout with retryUntilTimeoutThrowsException
1 parent ef147d8 commit 8c0febd

File tree

5 files changed

+32
-99
lines changed

5 files changed

+32
-99
lines changed

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

Lines changed: 2 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -135,15 +135,9 @@ 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);
145-
}
146-
147141
public void onExpired(final Runnable onExpired) {
148142
Timeout.nullAsInfinite(timeout).onExpired(onExpired);
149143
}
@@ -374,7 +368,6 @@ public Timeout startWaitQueueTimeout(final StartTime checkoutStart) {
374368
return checkoutStart.timeoutAfterOrInfiniteIfNegative(ms, MILLISECONDS);
375369
}
376370

377-
// TODO (CSOT) method not used in production;
378371
@Nullable
379372
public Timeout getTimeout() {
380373
return timeout;

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

Lines changed: 9 additions & 13 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
/**
@@ -204,10 +203,11 @@ private void doAdvanceOrThrow(final Throwable attemptException,
204203
if (isLastAttempt() || attemptException instanceof MongoOperationTimeoutException) {
205204
previouslyChosenException = newlyChosenException;
206205
/*
207-
* The function of isLastIteration() is to indicate if retrying has been explicitly halted. Such a stop is not interpreted as
206+
* The function of isLastIteration() is to indicate if retrying has
207+
* been explicitly halted. Such a stop is not interpreted as
208208
* a timeout exception but as a deliberate cessation of retry attempts.
209209
*/
210-
if (hasTimeoutMs() && !loopState.isLastIteration()) {
210+
if (retryUntilTimeoutThrowsException && !loopState.isLastIteration()) {
211211
previouslyChosenException = createMongoTimeoutException(
212212
"Retry attempt exceeded the timeout limit.",
213213
previouslyChosenException);
@@ -381,16 +381,12 @@ public boolean isLastAttempt() {
381381
if (loopState.isLastIteration()){
382382
return true;
383383
}
384-
if (hasTimeoutMs()) {
385-
return assertNotNull(timeoutContext).hasExpired();
384+
if (retryUntilTimeoutThrowsException) {
385+
return false;
386386
}
387387
return attempt() == attempts - 1;
388388
}
389389

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

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

Lines changed: 11 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -443,21 +443,24 @@ 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

450450
static void attachNew(final RetryState retryState, final boolean retry, final TimeoutContext timeoutContext) {
451-
retryState.attach(AttachmentKeys.bulkWriteTracker(), new BulkWriteTracker(retry, null, timeoutContext), false);
451+
boolean retryUntilTimeoutThrowsException = timeoutContext.hasTimeoutMS();
452+
retryState.attach(AttachmentKeys.bulkWriteTracker(), new BulkWriteTracker(retry, null, retryUntilTimeoutThrowsException), false);
452453
}
453454

454455
static void attachNew(final RetryState retryState, final BulkWriteBatch batch, final TimeoutContext timeoutContext) {
455-
attach(retryState, new BulkWriteTracker(batch.getRetryWrites(), batch, timeoutContext));
456+
boolean retryUntilTimeoutThrowsException = timeoutContext.hasTimeoutMS();
457+
attach(retryState, new BulkWriteTracker(batch.getRetryWrites(), batch, retryUntilTimeoutThrowsException));
456458
}
457459

458460
static BulkWriteTracker attachNext(final RetryState retryState, final BulkWriteBatch batch, final TimeoutContext timeoutContext) {
461+
boolean retryUntilTimeoutThrowsException = timeoutContext.hasTimeoutMS();
459462
BulkWriteBatch nextBatch = batch.getNextBatch();
460-
BulkWriteTracker nextTracker = new BulkWriteTracker(nextBatch.getRetryWrites(), nextBatch, timeoutContext);
463+
BulkWriteTracker nextTracker = new BulkWriteTracker(nextBatch.getRetryWrites(), nextBatch, retryUntilTimeoutThrowsException);
461464
attach(retryState, nextTracker);
462465
return nextTracker;
463466
}
@@ -471,16 +474,16 @@ private static void attach(final RetryState retryState, final BulkWriteTracker t
471474
}
472475
}
473476

474-
private BulkWriteTracker(final boolean retry, @Nullable final BulkWriteBatch batch, final TimeoutContext timeoutContext) {
477+
private BulkWriteTracker(final boolean retry, @Nullable final BulkWriteBatch batch, final boolean retryUntilTimeoutThrowsException) {
475478
attempt = 0;
476479
attempts = retry ? RetryState.RETRIES + 1 : 1;
477480
this.batch = batch;
478-
this.timeoutContext = timeoutContext;
481+
this.retryUntilTimeoutThrowsException = retryUntilTimeoutThrowsException;
479482
}
480483

481484
boolean lastAttempt() {
482-
if (timeoutContext.hasTimeoutMS()){
483-
return timeoutContext.hasExpired();
485+
if (retryUntilTimeoutThrowsException){
486+
return false;
484487
}
485488
return attempt == attempts - 1;
486489
}

‎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: 0 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,6 @@
4040
import static org.junit.jupiter.api.Named.named;
4141
import static org.junit.jupiter.params.provider.Arguments.arguments;
4242
import static org.mockito.Mockito.mock;
43-
import static org.mockito.Mockito.when;
4443

4544
final class RetryStateTest {
4645
private static final TimeoutContext TIMEOUT_CONTEXT_NO_GLOBAL_TIMEOUT = new TimeoutContext(new TimeoutSettings(0L, 0L,
@@ -97,17 +96,6 @@ void unlimitedAttemptsAndAdvance(final TimeoutContext timeoutContext) {
9796
);
9897
}
9998

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-
11199
@Test
112100
void limitedAttemptsAndAdvance() {
113101
RetryState retryState = RetryState.withNonRetryableState();
@@ -169,14 +157,6 @@ void breakAndThrowIfRetryAndFalse(final TimeoutContext timeoutContext) {
169157
assertFalse(retryState.isLastAttempt());
170158
}
171159

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-
180160
@ParameterizedTest
181161
@MethodSource({"infiniteTimeout", "noTimeout"})
182162
void breakAndThrowIfRetryAndTrue() {
@@ -189,10 +169,6 @@ void breakAndThrowIfRetryAndTrue() {
189169
@Test
190170
void breakAndThrowIfRetryAndTrueWithExpiredTimeout() {
191171
TimeoutContext tContextMock = mock(TimeoutContext.class);
192-
when(tContextMock.hasTimeoutMS()).thenReturn(true);
193-
when(tContextMock.hasExpired())
194-
.thenReturn(false)
195-
.thenReturn(true);
196172

197173
RetryState retryState = new RetryState(tContextMock);
198174
advance(retryState);
@@ -354,24 +330,6 @@ void advanceOrThrowPredicateThrowsAfterFirstAttempt(final TimeoutContext timeout
354330
}));
355331
}
356332

357-
@Test
358-
void advanceOrThrowPredicateThrowsTimeoutAfterFirstAttempt() {
359-
RetryState retryState = new RetryState(TIMEOUT_CONTEXT_EXPIRED_GLOBAL_TIMEOUT);
360-
RuntimeException predicateException = new RuntimeException() {
361-
};
362-
RuntimeException attemptException = new RuntimeException() {
363-
};
364-
MongoOperationTimeoutException mongoOperationTimeoutException = assertThrows(MongoOperationTimeoutException.class,
365-
() -> retryState.advanceOrThrow(attemptException, (e1, e2) -> e2, (rs, e) -> {
366-
assertTrue(rs.isFirstAttempt());
367-
assertEquals(attemptException, e);
368-
throw predicateException;
369-
}));
370-
371-
assertEquals(EXPECTED_TIMEOUT_MESSAGE, mongoOperationTimeoutException.getMessage());
372-
assertEquals(attemptException, mongoOperationTimeoutException.getCause());
373-
}
374-
375333
@ParameterizedTest
376334
@MethodSource({"infiniteTimeout", "noTimeout"})
377335
void advanceOrThrowPredicateThrows(final TimeoutContext timeoutContext) {
@@ -439,30 +397,6 @@ void advanceOrThrowTransformAfterFirstAttempt(final TimeoutContext timeoutContex
439397
}));
440398
}
441399

442-
@Test
443-
void advanceOrThrowTransformThrowsTimeoutExceptionAfterFirstAttempt() {
444-
RetryState retryState = new RetryState(TIMEOUT_CONTEXT_EXPIRED_GLOBAL_TIMEOUT);
445-
RuntimeException attemptException = new RuntimeException() {
446-
};
447-
RuntimeException transformerResult = new RuntimeException() {
448-
};
449-
MongoOperationTimeoutException mongoOperationTimeoutException =
450-
assertThrows(MongoOperationTimeoutException.class, () -> retryState.advanceOrThrow(attemptException,
451-
(e1, e2) -> {
452-
assertNull(e1);
453-
assertEquals(attemptException, e2);
454-
return transformerResult;
455-
},
456-
(rs, e) -> {
457-
assertEquals(attemptException, e);
458-
return false;
459-
}));
460-
461-
assertEquals(EXPECTED_TIMEOUT_MESSAGE, mongoOperationTimeoutException.getMessage());
462-
assertEquals(transformerResult, mongoOperationTimeoutException.getCause());
463-
464-
}
465-
466400
@ParameterizedTest
467401
@MethodSource({"infiniteTimeout", "noTimeout"})
468402
void advanceOrThrowTransform(final TimeoutContext timeoutContext) {

0 commit comments

Comments
 (0)