Skip to content

CSOT: Ignore wTimeoutMS #1368

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 8 commits into from
Apr 22, 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 @@ -25,7 +25,7 @@
import java.util.function.Supplier;

/**
* <p>Design by contract assertions.</p> <p>This class is not part of the public API and may be removed or changed at any time.</p>
Copy link
Member Author

@vbabanin vbabanin Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement appears to be a duplicate of the one found on line 36.

* <p>Design by contract assertions.</p>
* All {@code assert...} methods throw {@link AssertionError} and should be used to check conditions which may be violated if and only if
* the driver code is incorrect. The intended usage of this methods is the same as of the
* <a href="https://docs.oracle.com/javase/8/docs/technotes/guides/language/assert.html">Java {@code assert} statement</a>. The reason
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ private void doAdvanceOrThrow(final Throwable attemptException,
*/
if (hasTimeoutMs() && !loopState.isLastIteration()) {
previouslyChosenException = createMongoTimeoutException(
"MongoDB operation timed out during a retry attempt",
"Retry attempt timed out.",
previouslyChosenException);
}
Copy link
Member Author

@vbabanin vbabanin Apr 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The retry could be triggered not only by transient errors in MongoDB operations but also within the scope of OIDC. Referring to it as a 'MongoDB operation' might lead to confusion among users.

throw previouslyChosenException;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.mongodb.Function;
import com.mongodb.WriteConcern;
import com.mongodb.internal.TimeoutContext;
import com.mongodb.lang.Nullable;
import org.bson.BsonDocument;

Expand Down Expand Up @@ -58,7 +59,7 @@ CommandCreator getCommandCreator() {
}

@Override
protected Function<BsonDocument, BsonDocument> getRetryCommandModifier() {
protected Function<BsonDocument, BsonDocument> getRetryCommandModifier(final TimeoutContext timeoutContext) {
return cmd -> cmd;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ public final class AsyncOperations<TDocument> {
public AsyncOperations(final MongoNamespace namespace, final Class<TDocument> documentClass, final ReadPreference readPreference,
final CodecRegistry codecRegistry, final ReadConcern readConcern, final WriteConcern writeConcern,
final boolean retryWrites, final boolean retryReads, final TimeoutSettings timeoutSettings) {
this.operations = new Operations<>(namespace, documentClass, readPreference, codecRegistry, readConcern, writeConcern,
WriteConcern writeConcernToUse = writeConcern;
if (timeoutSettings.getTimeoutMS() != null) {
writeConcernToUse = assertNotNull(WriteConcernHelper.cloneWithoutTimeout(writeConcern));
}
this.operations = new Operations<>(namespace, documentClass, readPreference, codecRegistry, readConcern, writeConcernToUse,
retryWrites, retryReads);
this.timeoutSettings = timeoutSettings;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import com.mongodb.MongoTimeoutException;
import com.mongodb.MongoWriteConcernException;
import com.mongodb.WriteConcern;
import com.mongodb.internal.TimeoutContext;
import com.mongodb.internal.async.SingleResultCallback;
import com.mongodb.internal.binding.AsyncWriteBinding;
import com.mongodb.internal.binding.WriteBinding;
Expand Down Expand Up @@ -125,7 +126,8 @@ CommandCreator getCommandCreator() {
};
if (alreadyCommitted) {
return (operationContext, serverDescription, connectionDescription) ->
getRetryCommandModifier().apply(creator.create(operationContext, serverDescription, connectionDescription));
getRetryCommandModifier(operationContext.getTimeoutContext())
.apply(creator.create(operationContext, serverDescription, connectionDescription));
} else if (recoveryToken != null) {
return (operationContext, serverDescription, connectionDescription) ->
creator.create(operationContext, serverDescription, connectionDescription)
Expand All @@ -136,10 +138,10 @@ CommandCreator getCommandCreator() {

@Override
@SuppressWarnings("deprecation") //wTimeout
protected Function<BsonDocument, BsonDocument> getRetryCommandModifier() {
protected Function<BsonDocument, BsonDocument> getRetryCommandModifier(final TimeoutContext timeoutContext) {
return command -> {
WriteConcern retryWriteConcern = getWriteConcern().withW("majority");
if (retryWriteConcern.getWTimeout(MILLISECONDS) == null) {
if (retryWriteConcern.getWTimeout(MILLISECONDS) == null && !timeoutContext.hasTimeoutMS()) {
retryWriteConcern = retryWriteConcern.withWTimeout(10000, MILLISECONDS);
}
command.put("writeConcern", retryWriteConcern.asDocument());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@

import java.util.List;

import static com.mongodb.assertions.Assertions.assertNotNull;
import static java.util.concurrent.TimeUnit.MILLISECONDS;

/**
Expand All @@ -82,7 +83,11 @@ public SyncOperations(final MongoNamespace namespace, final Class<TDocument> doc
public SyncOperations(@Nullable final MongoNamespace namespace, final Class<TDocument> documentClass, final ReadPreference readPreference,
final CodecRegistry codecRegistry, final ReadConcern readConcern, final WriteConcern writeConcern,
final boolean retryWrites, final boolean retryReads, final TimeoutSettings timeoutSettings) {
this.operations = new Operations<>(namespace, documentClass, readPreference, codecRegistry, readConcern, writeConcern,
WriteConcern writeConcernToUse = writeConcern;
if (timeoutSettings.getTimeoutMS() != null) {
writeConcernToUse = assertNotNull(WriteConcernHelper.cloneWithoutTimeout(writeConcern));
}
this.operations = new Operations<>(namespace, documentClass, readPreference, codecRegistry, readConcern, writeConcernToUse,
retryWrites, retryReads);
this.timeoutSettings = timeoutSettings;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import com.mongodb.Function;
import com.mongodb.WriteConcern;
import com.mongodb.internal.TimeoutContext;
import com.mongodb.internal.async.SingleResultCallback;
import com.mongodb.internal.binding.AsyncWriteBinding;
import com.mongodb.internal.binding.WriteBinding;
Expand Down Expand Up @@ -55,17 +56,19 @@ public WriteConcern getWriteConcern() {
@Override
public Void execute(final WriteBinding binding) {
isTrue("in transaction", binding.getOperationContext().getSessionContext().hasActiveTransaction());
TimeoutContext timeoutContext = binding.getOperationContext().getTimeoutContext();
return executeRetryableWrite(binding, "admin", null, new NoOpFieldNameValidator(),
new BsonDocumentCodec(), getCommandCreator(),
writeConcernErrorTransformer(binding.getOperationContext().getTimeoutContext()), getRetryCommandModifier());
writeConcernErrorTransformer(timeoutContext), getRetryCommandModifier(timeoutContext));
}

@Override
public void executeAsync(final AsyncWriteBinding binding, final SingleResultCallback<Void> callback) {
isTrue("in transaction", binding.getOperationContext().getSessionContext().hasActiveTransaction());
TimeoutContext timeoutContext = binding.getOperationContext().getTimeoutContext();
executeRetryableWriteAsync(binding, "admin", null, new NoOpFieldNameValidator(),
new BsonDocumentCodec(), getCommandCreator(),
writeConcernErrorTransformerAsync(binding.getOperationContext().getTimeoutContext()), getRetryCommandModifier(),
writeConcernErrorTransformerAsync(timeoutContext), getRetryCommandModifier(timeoutContext),
errorHandlingCallback(callback, LOGGER));
}

Expand All @@ -86,5 +89,5 @@ CommandCreator getCommandCreator() {
*/
protected abstract String getCommandName();

protected abstract Function<BsonDocument, BsonDocument> getRetryCommandModifier();
protected abstract Function<BsonDocument, BsonDocument> getRetryCommandModifier(TimeoutContext timeoutContext);
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,12 @@
import com.mongodb.bulk.WriteConcernError;
import com.mongodb.internal.TimeoutContext;
import com.mongodb.internal.connection.ProtocolHelper;
import com.mongodb.lang.Nullable;
import org.bson.BsonArray;
import org.bson.BsonDocument;
import org.bson.BsonString;

import java.util.concurrent.TimeUnit;
import java.util.stream.Collectors;

import static com.mongodb.internal.operation.CommandOperationHelper.addRetryableWriteErrorLabel;
Expand All @@ -42,6 +44,21 @@ public static void appendWriteConcernToCommand(final WriteConcern writeConcern,
commandDocument.put("writeConcern", writeConcern.asDocument());
}
}
@Nullable
public static WriteConcern cloneWithoutTimeout(@Nullable final WriteConcern writeConcern) {
if (writeConcern == null || writeConcern.getWTimeout(TimeUnit.MILLISECONDS) == null) {
return writeConcern;
}

WriteConcern mapped;
Object w = writeConcern.getWObject();
if (w == null) {
mapped = WriteConcern.ACKNOWLEDGED;
} else {
mapped = w instanceof Integer ? new WriteConcern((Integer) w) : new WriteConcern((String) w);
}
return mapped.withJournal(writeConcern.getJournal());
}

public static void throwOnWriteConcernError(final BsonDocument result, final ServerAddress serverAddress,
final int maxWireVersion, final TimeoutContext timeoutContext) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.mongodb.MongoClientException;
import com.mongodb.ServerAddress;
import com.mongodb.TransactionOptions;
import com.mongodb.WriteConcern;
import com.mongodb.internal.TimeoutContext;
import com.mongodb.internal.TimeoutSettings;
import com.mongodb.internal.binding.ReferenceCounted;
Expand All @@ -29,6 +30,7 @@
import org.bson.BsonDocument;
import org.bson.BsonTimestamp;

import java.util.concurrent.TimeUnit;
import java.util.concurrent.atomic.AtomicBoolean;

import static com.mongodb.assertions.Assertions.assertTrue;
Expand All @@ -55,6 +57,14 @@ public class BaseClientSessionImpl implements ClientSession {
@Nullable
private TimeoutContext timeoutContext;

protected static boolean hasTimeoutMS(@Nullable final TimeoutContext timeoutContext) {
return timeoutContext != null && timeoutContext.hasTimeoutMS();
}

protected static boolean hasWTimeoutMS(@Nullable final WriteConcern writeConcern) {
return writeConcern != null && writeConcern.getWTimeout(TimeUnit.MILLISECONDS) != null;
}

public BaseClientSessionImpl(final ServerSessionPool serverSessionPool, final Object originator, final ClientSessionOptions options) {
this.serverSessionPool = serverSessionPool;
this.originator = originator;
Expand Down Expand Up @@ -228,4 +238,8 @@ protected TimeoutSettings getTimeoutSettings(final TransactionOptions transactio
.withMaxCommitMS(transactionOptions.getMaxCommitTime(MILLISECONDS))
.withTimeout(timeoutMS, MILLISECONDS);
}

protected enum TransactionState {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

NONE, IN, COMMITTED, ABORTED
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
import java.util.concurrent.locks.Condition;
import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
import java.util.function.Predicate;
import java.util.stream.Collectors;

import static com.mongodb.ClusterFixture.TIMEOUT;
Expand Down Expand Up @@ -152,15 +153,28 @@ public List<CommandStartedEvent> getCommandStartedEvents() {
return getEvents(CommandStartedEvent.class, Integer.MAX_VALUE);
}

public List<CommandStartedEvent> getCommandStartedEvents(final String commandName) {
return getEvents(CommandStartedEvent.class,
commandEvent -> commandEvent.getCommandName().equals(commandName),
Integer.MAX_VALUE);
}

public List<CommandSucceededEvent> getCommandSucceededEvents() {
return getEvents(CommandSucceededEvent.class, Integer.MAX_VALUE);
}

private <T extends CommandEvent> List<T> getEvents(final Class<T> type, final int maxEvents) {
return getEvents(type, e -> true, maxEvents);
}

private <T extends CommandEvent> List<T> getEvents(final Class<T> type,
final Predicate<? super CommandEvent> filter,
final int maxEvents) {
lock.lock();
try {
return getEvents().stream()
.filter(e -> e.getClass() == type)
.filter(filter)
.map(type::cast)
.limit(maxEvents).collect(Collectors.toList());
} finally {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,7 @@
},
{
"description": "commitTransaction ignores wTimeoutMS if timeoutMS is set",
"comment": "Moved timeoutMS from commitTransaction to startTransaction manually, as commitTransaction does not support a timeoutMS option.",
"operations": [
{
"name": "createEntities",
Expand Down Expand Up @@ -186,7 +187,10 @@
},
{
"name": "startTransaction",
"object": "session"
"object": "session",
"arguments": {
"timeoutMS": 10000
}
},
{
"name": "countDocuments",
Expand All @@ -198,10 +202,7 @@
},
{
"name": "commitTransaction",
"object": "session",
"arguments": {
"timeoutMS": 10000
}
"object": "session"
}
],
"expectEvents": [
Expand Down Expand Up @@ -430,6 +431,7 @@
},
{
"description": "abortTransaction ignores wTimeoutMS if timeoutMS is set",
"comment": "Moved timeoutMS from abortTransaction to startTransaction manually, as abortTransaction does not support a timeoutMS option.",
"operations": [
{
"name": "createEntities",
Expand Down Expand Up @@ -475,7 +477,10 @@
},
{
"name": "startTransaction",
"object": "session"
"object": "session",
"arguments": {
"timeoutMS": 10000
}
},
{
"name": "countDocuments",
Expand All @@ -487,10 +492,7 @@
},
{
"name": "abortTransaction",
"object": "session",
"arguments": {
"timeoutMS": 10000
}
"object": "session"
}
],
"expectEvents": [
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ final class RetryStateTest {

private static final TimeoutContext TIMEOUT_CONTEXT_INFINITE_GLOBAL_TIMEOUT = new TimeoutContext(new TimeoutSettings(0L, 0L,
0L, 0L, 0L));
private static final String EXPECTED_TIMEOUT_MESSAGE = "MongoDB operation timed out during a retry attempt";
private static final String EXPECTED_TIMEOUT_MESSAGE = "Retry attempt timed out.";

static Stream<Arguments> infiniteTimeout() {
return Stream.of(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
* Copyright 2008-present MongoDB, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package com.mongodb.internal.operation;

import com.mongodb.WriteConcern;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.MethodSource;

import java.util.concurrent.TimeUnit;

import static com.mongodb.assertions.Assertions.assertNull;
import static org.junit.jupiter.api.Assertions.assertEquals;

class WriteConcernHelperTest {

static WriteConcern[] shouldRemoveWtimeout(){
return new WriteConcern[]{
WriteConcern.ACKNOWLEDGED,
WriteConcern.MAJORITY,
WriteConcern.W1,
WriteConcern.W2,
WriteConcern.W3,
WriteConcern.UNACKNOWLEDGED,
WriteConcern.JOURNALED,

WriteConcern.ACKNOWLEDGED.withWTimeout(100, TimeUnit.MILLISECONDS),
WriteConcern.MAJORITY.withWTimeout(100, TimeUnit.MILLISECONDS),
WriteConcern.W1.withWTimeout(100, TimeUnit.MILLISECONDS),
WriteConcern.W2.withWTimeout(100, TimeUnit.MILLISECONDS),
WriteConcern.W3.withWTimeout(100, TimeUnit.MILLISECONDS),
WriteConcern.UNACKNOWLEDGED.withWTimeout(100, TimeUnit.MILLISECONDS),
WriteConcern.JOURNALED.withWTimeout(100, TimeUnit.MILLISECONDS),
};
}

@MethodSource
@ParameterizedTest
void shouldRemoveWtimeout(final WriteConcern writeConcern){
//when
WriteConcern clonedWithoutTimeout = WriteConcernHelper.cloneWithoutTimeout(writeConcern);

//then
assertEquals(writeConcern.getWObject(), clonedWithoutTimeout.getWObject());
assertEquals(writeConcern.getJournal(), clonedWithoutTimeout.getJournal());
assertNull(clonedWithoutTimeout.getWTimeout(TimeUnit.MILLISECONDS));
}
}
Loading