Skip to content

Commit 6101b16

Browse files
katcharovkatcharov
committed
Remove hasExpired and expiry checks in retries (rely on exceptions)
1 parent f866991 commit 6101b16

14 files changed

+90
-321
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/AsyncRunnable.java

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,6 @@
1616

1717
package com.mongodb.internal.async;
1818

19-
import com.mongodb.internal.TimeoutContext;
2019
import com.mongodb.internal.async.function.RetryState;
2120
import com.mongodb.internal.async.function.RetryingAsyncCallbackSupplier;
2221

@@ -268,10 +267,10 @@ default <R> AsyncSupplier<R> thenSupply(final AsyncSupplier<R> supplier) {
268267
* @see RetryingAsyncCallbackSupplier
269268
*/
270269
default AsyncRunnable thenRunRetryingWhile(
271-
final TimeoutContext timeoutContext, final AsyncRunnable runnable, final Predicate<Throwable> shouldRetry) {
270+
final AsyncRunnable runnable, final Predicate<Throwable> shouldRetry) {
272271
return thenRun(callback -> {
273272
new RetryingAsyncCallbackSupplier<Void>(
274-
new RetryState(timeoutContext),
273+
new RetryState(),
275274
(rs, lastAttemptFailure) -> shouldRetry.test(lastAttemptFailure),
276275
// `finish` is required here instead of `unsafeFinish`
277276
// because only `finish` meets the contract of

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

Lines changed: 10 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@
3131
import static com.mongodb.assertions.Assertions.assertFalse;
3232
import static com.mongodb.assertions.Assertions.assertNotNull;
3333
import static com.mongodb.assertions.Assertions.assertTrue;
34-
import static com.mongodb.internal.TimeoutContext.createMongoTimeoutException;
3534

3635
/**
3736
* Represents both the state associated with a retryable activity and a handle that can be used to affect retrying, e.g.,
@@ -52,8 +51,6 @@ public final class RetryState {
5251
private final LoopState loopState;
5352
private final int attempts;
5453
@Nullable
55-
private final TimeoutContext timeoutContext;
56-
@Nullable
5754
private Throwable previouslyChosenException;
5855

5956
/**
@@ -63,51 +60,44 @@ public final class RetryState {
6360
* If a timeout is not specified in the {@link TimeoutContext#hasTimeoutMS()}, the specified {@code retries} param acts as a fallback
6461
* bound. Otherwise, retries are unbounded until the timeout is reached.
6562
* <p>
66-
* It is possible to provide an additional {@code retryPredicate} in the {@link #doAdvanceOrThrow(Throwable, BiFunction, BiPredicate, boolean)} method,
63+
* It is possible to provide an additional {@code retryPredicate} in the {@link #doAdvanceOrThrow} method,
6764
* which can be used to stop retrying based on a custom condition additionally to {@code retires} and {@link TimeoutContext}.
6865
* </p>
6966
*
7067
* @param retries A positive number of allowed retries. {@link Integer#MAX_VALUE} is a special value interpreted as being unlimited.
71-
* @param timeoutContext A timeout context that will be used to determine if the operation has timed out.
7268
* @see #attempts()
7369
*/
74-
public static RetryState withRetryableState(final int retries, final TimeoutContext timeoutContext) {
70+
public static RetryState withRetryableState(final int retries) {
7571
assertTrue(retries > 0);
76-
if (timeoutContext.hasTimeoutMS()){
77-
return new RetryState(INFINITE_ATTEMPTS, timeoutContext);
78-
}
79-
return new RetryState(retries, null);
72+
return new RetryState(retries);
8073
}
8174

8275
public static RetryState withNonRetryableState() {
83-
return new RetryState(0, null);
76+
return new RetryState(0);
8477
}
8578

8679
/**
8780
* Creates a {@link RetryState} that does not limit the number of retries.
8881
* The number of attempts is limited iff {@link TimeoutContext#hasTimeoutMS()} is true and timeout has expired.
8982
* <p>
90-
* It is possible to provide an additional {@code retryPredicate} in the {@link #doAdvanceOrThrow(Throwable, BiFunction, BiPredicate, boolean)} method,
83+
* It is possible to provide an additional {@code retryPredicate} in the {@link #doAdvanceOrThrow} method,
9184
* which can be used to stop retrying based on a custom condition additionally to {@code retires} and {@link TimeoutContext}.
9285
* </p>
9386
*
94-
* @param timeoutContext A timeout context that will be used to determine if the operation has timed out.
9587
* @see #attempts()
9688
*/
97-
public RetryState(final TimeoutContext timeoutContext) {
98-
this(INFINITE_ATTEMPTS, timeoutContext);
89+
public RetryState() {
90+
this(INFINITE_ATTEMPTS);
9991
}
10092

10193
/**
10294
* @param retries A non-negative number of allowed retries. {@link Integer#MAX_VALUE} is a special value interpreted as being unlimited.
103-
* @param timeoutContext A timeout context that will be used to determine if the operation has timed out.
10495
* @see #attempts()
10596
*/
106-
private RetryState(final int retries, @Nullable final TimeoutContext timeoutContext) {
97+
private RetryState(final int retries) {
10798
assertTrue(retries >= 0);
10899
loopState = new LoopState();
109100
attempts = retries == INFINITE_ATTEMPTS ? INFINITE_ATTEMPTS : retries + 1;
110-
this.timeoutContext = timeoutContext;
111101
}
112102

113103
/**
@@ -203,15 +193,6 @@ private void doAdvanceOrThrow(final Throwable attemptException,
203193
*/
204194
if (isLastAttempt() || attemptException instanceof MongoOperationTimeoutException) {
205195
previouslyChosenException = newlyChosenException;
206-
/*
207-
* The function of isLastIteration() is to indicate if retrying has been explicitly halted. Such a stop is not interpreted as
208-
* a timeout exception but as a deliberate cessation of retry attempts.
209-
*/
210-
if (hasTimeoutMs() && !loopState.isLastIteration()) {
211-
previouslyChosenException = createMongoTimeoutException(
212-
"Retry attempt timed out.",
213-
previouslyChosenException);
214-
}
215196
throw previouslyChosenException;
216197
} else {
217198
// note that we must not update the state, e.g, `previouslyChosenException`, `loopState`, before calling `retryPredicate`
@@ -381,16 +362,9 @@ public boolean isLastAttempt() {
381362
if (loopState.isLastIteration()){
382363
return true;
383364
}
384-
if (hasTimeoutMs()) {
385-
return assertNotNull(timeoutContext).hasExpired();
386-
}
387365
return attempt() == attempts - 1;
388366
}
389367

390-
private boolean hasTimeoutMs() {
391-
return timeoutContext != null && timeoutContext.hasTimeoutMS();
392-
}
393-
394368
/**
395369
* A 0-based attempt number.
396370
*
@@ -403,9 +377,9 @@ public int attempt() {
403377
/**
404378
* Returns a positive maximum number of attempts:
405379
* <ul>
406-
* <li>0 if the number of retries is {@linkplain #RetryState(TimeoutContext) unlimited};</li>
380+
* <li>0 if the number of retries is {@linkplain #RetryState() unlimited};</li>
407381
* <li>1 if no retries are allowed;</li>
408-
* <li>{@link #RetryState(int, TimeoutContext) retries} + 1 otherwise.</li>
382+
* <li>{@link #RetryState(int) retries} + 1 otherwise.</li>
409383
* </ul>
410384
*
411385
* @see #attempt()

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -291,7 +291,6 @@ private void authenticationLoopAsync(final InternalConnection connection, final
291291
final SingleResultCallback<Void> callback) {
292292
fallbackState = FallbackState.INITIAL;
293293
beginAsync().thenRunRetryingWhile(
294-
operationContext.getTimeoutContext(),
295294
c -> super.authenticateAsync(connection, description, operationContext, c),
296295
e -> triggersRetry(e) && shouldRetryHandler()
297296
).finish(callback);

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,7 +181,7 @@ static <D, T> void executeRetryableReadAsync(
181181
final CommandReadTransformerAsync<D, T> transformer,
182182
final boolean retryReads,
183183
final SingleResultCallback<T> callback) {
184-
RetryState retryState = initialRetryState(retryReads, binding.getOperationContext().getTimeoutContext());
184+
RetryState retryState = initialRetryState(retryReads);
185185
binding.retain();
186186
OperationContext operationContext = binding.getOperationContext();
187187
AsyncCallbackSupplier<T> asyncRead = decorateReadWithRetriesAsync(retryState, binding.getOperationContext(),
@@ -240,7 +240,7 @@ static <T, R> void executeRetryableWriteAsync(
240240
final Function<BsonDocument, BsonDocument> retryCommandModifier,
241241
final SingleResultCallback<R> callback) {
242242

243-
RetryState retryState = initialRetryState(true, binding.getOperationContext().getTimeoutContext());
243+
RetryState retryState = initialRetryState(true);
244244
binding.retain();
245245
OperationContext operationContext = binding.getOperationContext();
246246

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

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@
2828
import com.mongodb.assertions.Assertions;
2929
import com.mongodb.connection.ConnectionDescription;
3030
import com.mongodb.connection.ServerDescription;
31-
import com.mongodb.internal.TimeoutContext;
3231
import com.mongodb.internal.async.function.RetryState;
3332
import com.mongodb.internal.connection.OperationContext;
3433
import com.mongodb.internal.operation.OperationHelper.ResourceSupplierInternalException;
@@ -99,9 +98,9 @@ private static Throwable chooseRetryableWriteException(
9998

10099
/* Read Binding Helpers */
101100

102-
static RetryState initialRetryState(final boolean retry, final TimeoutContext timeoutContext) {
101+
static RetryState initialRetryState(final boolean retry) {
103102
if (retry) {
104-
return RetryState.withRetryableState(RetryState.RETRIES, timeoutContext);
103+
return RetryState.withRetryableState(RetryState.RETRIES);
105104
}
106105
return RetryState.withNonRetryableState();
107106
}

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -290,7 +290,7 @@ public BatchCursor<T> execute(final ReadBinding binding) {
290290
throw invalidTimeoutModeException;
291291
}
292292

293-
RetryState retryState = initialRetryState(retryReads, binding.getOperationContext().getTimeoutContext());
293+
RetryState retryState = initialRetryState(retryReads);
294294
Supplier<BatchCursor<T>> read = decorateReadWithRetries(retryState, binding.getOperationContext(), () ->
295295
withSourceAndConnection(binding::getReadConnectionSource, false, (source, connection) -> {
296296
retryState.breakAndThrowIfRetryAnd(() -> !canRetryRead(source.getServerDescription(), binding.getOperationContext()));
@@ -314,7 +314,7 @@ public void executeAsync(final AsyncReadBinding binding, final SingleResultCallb
314314
return;
315315
}
316316

317-
RetryState retryState = initialRetryState(retryReads, binding.getOperationContext().getTimeoutContext());
317+
RetryState retryState = initialRetryState(retryReads);
318318
binding.retain();
319319
AsyncCallbackSupplier<AsyncBatchCursor<T>> asyncRead = decorateReadWithRetriesAsync(
320320
retryState, binding.getOperationContext(), (AsyncCallbackSupplier<AsyncBatchCursor<T>>) funcCallback ->

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ public ListCollectionsOperation<T> timeoutMode(@Nullable final TimeoutMode timeo
159159

160160
@Override
161161
public BatchCursor<T> execute(final ReadBinding binding) {
162-
RetryState retryState = initialRetryState(retryReads, binding.getOperationContext().getTimeoutContext());
162+
RetryState retryState = initialRetryState(retryReads);
163163
Supplier<BatchCursor<T>> read = decorateReadWithRetries(retryState, binding.getOperationContext(), () ->
164164
withSourceAndConnection(binding::getReadConnectionSource, false, (source, connection) -> {
165165
retryState.breakAndThrowIfRetryAnd(() -> !canRetryRead(source.getServerDescription(), binding.getOperationContext()));
@@ -177,7 +177,7 @@ public BatchCursor<T> execute(final ReadBinding binding) {
177177

178178
@Override
179179
public void executeAsync(final AsyncReadBinding binding, final SingleResultCallback<AsyncBatchCursor<T>> callback) {
180-
RetryState retryState = initialRetryState(retryReads, binding.getOperationContext().getTimeoutContext());
180+
RetryState retryState = initialRetryState(retryReads);
181181
binding.retain();
182182
AsyncCallbackSupplier<AsyncBatchCursor<T>> asyncRead = decorateReadWithRetriesAsync(
183183
retryState, binding.getOperationContext(), (AsyncCallbackSupplier<AsyncBatchCursor<T>>) funcCallback ->

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ public ListIndexesOperation<T> timeoutMode(@Nullable final TimeoutMode timeoutMo
118118

119119
@Override
120120
public BatchCursor<T> execute(final ReadBinding binding) {
121-
RetryState retryState = initialRetryState(retryReads, binding.getOperationContext().getTimeoutContext());
121+
RetryState retryState = initialRetryState(retryReads);
122122
Supplier<BatchCursor<T>> read = decorateReadWithRetries(retryState, binding.getOperationContext(), () ->
123123
withSourceAndConnection(binding::getReadConnectionSource, false, (source, connection) -> {
124124
retryState.breakAndThrowIfRetryAnd(() -> !canRetryRead(source.getServerDescription(), binding.getOperationContext()));
@@ -136,7 +136,7 @@ public BatchCursor<T> execute(final ReadBinding binding) {
136136

137137
@Override
138138
public void executeAsync(final AsyncReadBinding binding, final SingleResultCallback<AsyncBatchCursor<T>> callback) {
139-
RetryState retryState = initialRetryState(retryReads, binding.getOperationContext().getTimeoutContext());
139+
RetryState retryState = initialRetryState(retryReads);
140140
binding.retain();
141141
AsyncCallbackSupplier<AsyncBatchCursor<T>> asyncRead = decorateReadWithRetriesAsync(
142142
retryState, binding.getOperationContext(), (AsyncCallbackSupplier<AsyncBatchCursor<T>>) funcCallback ->

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

Lines changed: 2 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ public BulkWriteResult execute(final WriteBinding binding) {
185185
* Fortunately, these counters do not exist concurrently with each other. While maintaining the counters manually,
186186
* we must adhere to the contract of `RetryingSyncSupplier`. When the retry timeout is implemented, there will be no counters,
187187
* and the code related to the attempt tracking in `BulkWriteTracker` will be removed. */
188-
RetryState retryState = new RetryState(timeoutContext);
188+
RetryState retryState = new RetryState();
189189
BulkWriteTracker.attachNew(retryState, retryWrites, timeoutContext);
190190
Supplier<BulkWriteResult> retryingBulkWrite = decorateWriteWithRetries(retryState, binding.getOperationContext(), () ->
191191
withSourceAndConnection(binding::getWriteConnectionSource, true, (source, connection) -> {
@@ -216,7 +216,7 @@ public BulkWriteResult execute(final WriteBinding binding) {
216216
public void executeAsync(final AsyncWriteBinding binding, final SingleResultCallback<BulkWriteResult> callback) {
217217
TimeoutContext timeoutContext = binding.getOperationContext().getTimeoutContext();
218218
// see the comment in `execute(WriteBinding)` explaining the manual tracking of attempts
219-
RetryState retryState = new RetryState(timeoutContext);
219+
RetryState retryState = new RetryState();
220220
BulkWriteTracker.attachNew(retryState, retryWrites, timeoutContext);
221221
binding.retain();
222222
AsyncCallbackSupplier<BulkWriteResult> retryingBulkWrite = this.<BulkWriteResult>decorateWriteWithRetries(retryState,
@@ -443,7 +443,6 @@ 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;
447446
@Nullable
448447
private final BulkWriteBatch batch;
449448

@@ -475,13 +474,9 @@ private BulkWriteTracker(final boolean retry, @Nullable final BulkWriteBatch bat
475474
attempt = 0;
476475
attempts = retry ? RetryState.RETRIES + 1 : 1;
477476
this.batch = batch;
478-
this.timeoutContext = timeoutContext;
479477
}
480478

481479
boolean lastAttempt() {
482-
if (timeoutContext.hasTimeoutMS()){
483-
return timeoutContext.hasExpired();
484-
}
485480
return attempt == attempts - 1;
486481
}
487482

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -189,7 +189,7 @@ static <D, T> T executeRetryableRead(
189189
final Decoder<D> decoder,
190190
final CommandReadTransformer<D, T> transformer,
191191
final boolean retryReads) {
192-
RetryState retryState = CommandOperationHelper.initialRetryState(retryReads, binding.getOperationContext().getTimeoutContext());
192+
RetryState retryState = CommandOperationHelper.initialRetryState(retryReads);
193193

194194
Supplier<T> read = decorateReadWithRetries(retryState, binding.getOperationContext(), () ->
195195
withSourceAndConnection(readConnectionSourceSupplier, false, (source, connection) -> {
@@ -242,7 +242,7 @@ static <T, R> R executeRetryableWrite(
242242
final CommandCreator commandCreator,
243243
final CommandWriteTransformer<T, R> transformer,
244244
final com.mongodb.Function<BsonDocument, BsonDocument> retryCommandModifier) {
245-
RetryState retryState = CommandOperationHelper.initialRetryState(true, binding.getOperationContext().getTimeoutContext());
245+
RetryState retryState = CommandOperationHelper.initialRetryState(true);
246246
Supplier<R> retryingWrite = decorateWriteWithRetries(retryState, binding.getOperationContext(), () -> {
247247
boolean firstAttempt = retryState.isFirstAttempt();
248248
SessionContext sessionContext = binding.getOperationContext().getSessionContext();

‎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

0 commit comments

Comments
 (0)