Skip to content

Fix race condition in retry logic. #1377

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
May 7, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -186,16 +186,20 @@ private void doAdvanceOrThrow(final Throwable attemptException,
final boolean onlyRuntimeExceptions) throws Throwable {
assertTrue(attempt() < attempts);
assertNotNull(attemptException);
if (attemptException instanceof MongoOperationTimeoutException) {
throw attemptException;
}
if (onlyRuntimeExceptions) {
assertTrue(isRuntime(attemptException));
}
assertTrue(!isFirstAttempt() || previouslyChosenException == null);
Throwable newlyChosenException = transformException(previouslyChosenException, attemptException, onlyRuntimeExceptions, exceptionTransformer);

if (isLastAttempt()) {
/*
* A MongoOperationTimeoutException indicates that the operation timed out, either during command execution or server selection.
* The timeout for server selection is determined by the computedServerSelectionMS = min(serverSelectionTimeoutMS, timeoutMS).
*
* The isLastAttempt() method checks if the timeoutMS has expired, which could be greater than the computedServerSelectionMS.
* Therefore, it's important to check if the exception is an instance of MongoOperationTimeoutException to detect a timeout.
*/
if (isLastAttempt() || attemptException instanceof MongoOperationTimeoutException) {
previouslyChosenException = newlyChosenException;
/*
* The function of isLastIteration() is to indicate if retrying has been explicitly halted. Such a stop is not interpreted as
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import com.mongodb.internal.async.function.LoopState.AttachmentKey;
import com.mongodb.internal.operation.retry.AttachmentKeys;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
Expand Down Expand Up @@ -268,6 +269,68 @@ void advanceOrThrowPredicateFalse(final TimeoutContext timeoutContext) {
assertThrows(attemptException.getClass(), () -> retryState.advanceOrThrow(attemptException, (e1, e2) -> e2, (rs, e) -> false));
}

@ParameterizedTest
@MethodSource({"infiniteTimeout"})
@DisplayName("should rethrow detected timeout exception even if timeout in retry state is not expired")
void advanceReThrowDetectedTimeoutExceptionEvenIfTimeoutInRetryStateIsNotExpired(final TimeoutContext timeoutContext) {
RetryState retryState = new RetryState(timeoutContext);

MongoOperationTimeoutException expectedTimeoutException = TimeoutContext.createMongoTimeoutException("Server selection failed");
MongoOperationTimeoutException actualTimeoutException =
assertThrows(expectedTimeoutException.getClass(), () -> retryState.advanceOrThrow(expectedTimeoutException,
(e1, e2) -> expectedTimeoutException,
(rs, e) -> false));

Assertions.assertEquals(actualTimeoutException, expectedTimeoutException);
}

@Test
@DisplayName("should throw timeout exception from retry, when transformer swallows original timeout exception")
void advanceThrowTimeoutExceptionWhenTransformerSwallowOriginalTimeoutException() {
RetryState retryState = new RetryState(TIMEOUT_CONTEXT_INFINITE_GLOBAL_TIMEOUT);
RuntimeException previousAttemptException = new RuntimeException() {
};
MongoOperationTimeoutException expectedTimeoutException = TimeoutContext.createMongoTimeoutException("Server selection failed");

retryState.advanceOrThrow(previousAttemptException,
(e1, e2) -> previousAttemptException,
(rs, e) -> true);

MongoOperationTimeoutException actualTimeoutException =
assertThrows(expectedTimeoutException.getClass(), () -> retryState.advanceOrThrow(expectedTimeoutException,
(e1, e2) -> previousAttemptException,
(rs, e) -> false));

Assertions.assertNotEquals(actualTimeoutException, expectedTimeoutException);
Assertions.assertEquals("Retry attempt timed out.", actualTimeoutException.getMessage());
Assertions.assertEquals(previousAttemptException, actualTimeoutException.getCause(),
"Retry timeout exception should have a cause if transformer returned non-timeout exception.");
}


@Test
@DisplayName("should throw original timeout exception from retry, when transformer returns original timeout exception")
void advanceThrowOriginalTimeoutExceptionWhenTransformerReturnsOriginalTimeoutException() {
RetryState retryState = new RetryState(TIMEOUT_CONTEXT_INFINITE_GLOBAL_TIMEOUT);
RuntimeException previousAttemptException = new RuntimeException() {
};
MongoOperationTimeoutException expectedTimeoutException = TimeoutContext
.createMongoTimeoutException("Server selection failed");

retryState.advanceOrThrow(previousAttemptException,
(e1, e2) -> previousAttemptException,
(rs, e) -> true);

MongoOperationTimeoutException actualTimeoutException =
assertThrows(expectedTimeoutException.getClass(), () -> retryState.advanceOrThrow(expectedTimeoutException,
(e1, e2) -> expectedTimeoutException,
(rs, e) -> false));

Assertions.assertEquals(actualTimeoutException, expectedTimeoutException);
Assertions.assertNull(actualTimeoutException.getCause(),
"Original timeout exception should not have a cause if transformer already returned timeout exception.");
}

@Test
void advanceOrThrowPredicateTrueAndLastAttempt() {
RetryState retryState = RetryState.withNonRetryableState();
Expand Down