Ignore maxWaitTime when CSOT is enabled. #1744

vbabanin · 2025-06-19T06:50:17Z

JAVA-5409

stIncMale · 2025-06-23T15:08:49Z

driver-core/src/main/com/mongodb/internal/time/Timeout.java

+ * Implementations of this interface must be immutable.
+ *
 * @see TimePoint
 */
 public interface Timeout {


Is this change needed because of a change done in this PR, or is it simply something that you noticed and improved?

Shouldn't the same be documented for StartTime?

This wasn’t required by changes in this PR, just a small improvement I noticed while reviewing the Timeout code. I thought documenting immutability in the Javadoc would be useful for anyone double-checking this detail, without needing to examine the implementations.

I’ve also added the note for StartTime as you suggested.

stIncMale · 2025-06-23T19:13:30Z

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

@@ -208,7 +210,7 @@ public InternalConnection get(final OperationContext operationContext) {
    @Override
    public void getAsync(final OperationContext operationContext, final SingleResultCallback<InternalConnection> callback) {
        StartTime checkoutStart = connectionCheckoutStarted(operationContext);
-        Timeout maxWaitTimeout = checkoutStart.timeoutAfterOrInfiniteIfNegative(settings.getMaxWaitTime(NANOSECONDS), NANOSECONDS);
+        Timeout maxWaitTimeout = operationContext.getTimeoutContext().startWaitQueueTimeout(checkoutStart);


maxWaitTimeout is called waitQueueTimeout in the get method. They should have equal names, and given that we obtain them by calling startWaitQueueTimeout, the name waitQueueTimeout seems to be more suitable. If it's not, maybe we should rename the method also?

Renamed to maxWaitTimeout everywhere in DefaultConnectionPool, as that’s the prevailing naming convention in the codebase. There are still some discrepancies in the javadocs, but this naming keeps it consistent with most usages internally.

stIncMale · 2025-06-23T19:26:51Z

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

-    private PooledConnection getPooledConnection(final Timeout waitQueueTimeout, final StartTime startTime) throws MongoTimeoutException {
+    private PooledConnection getPooledConnection(final Timeout waitQueueTimeout,
+                                                 final StartTime startTime,
+                                                 final OperationContext operationContext) throws MongoTimeoutException {


Given that this methods uses only operationContext.getTimeoutContext(), let's accept TimeoutContext instead of OperationContext? The same applies to the acquirePermitOrGetAvailableOpenedConnection method and the constructor of DefaultConnectionPool.Task.

Interestingly, createTimeoutException rightly accepts TimeoutContext, not OperationContext.

stIncMale · 2025-06-23T19:38:21Z

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

-                    elapsedMs, serverId.getAddress()));
+            String errorMessage = format("Timed out after %d ms while waiting for a connection to server %s.",
+                    elapsedMs, serverId.getAddress());
+            return timeoutContext.hasTimeoutMS() ? createMongoTimeoutException(errorMessage) : new MongoTimeoutException(errorMessage);


Let's extract the errorMessage variable to a place before the if block, initialize it in the if and the else blocks, and then do return timeoutContext.hasTimeoutMS() ? createMongoTimeoutException(errorMessage) : new MongoTimeoutException(errorMessage); as the last statement in the createTimeoutException method. This way we will avoid duplicating this last statement.

driver-core/src/test/functional/com/mongodb/internal/connection/DefaultConnectionPoolTest.java

stIncMale · 2025-06-23T21:30:56Z

...ync/src/test/functional/com/mongodb/client/AbstractClientSideOperationsTimeoutProseTest.java

+     */
+    @Test
+    @DisplayName("Should ignore waitQueueTimeoutMS when timeoutMS is set")
+    public void shouldUseWaitQueueTimeoutMSWhenTimeoutIsNotSet() {


@DisplayName does not match the name/behavior of the test.

stIncMale · 2025-06-24T13:47:01Z

driver-core/src/test/functional/com/mongodb/internal/connection/DefaultConnectionPoolTest.java

+
+        // when
+        TimeoutTrackingConnectionGetter connectionGetter = new TimeoutTrackingConnectionGetter(provider, timeoutSettings);
+        new Thread(connectionGetter).start();


Instead of starting a new thread and never making sure it terminates, let's use cachedExecutor, which is properly terminated in cleanup.

stIncMale · 2025-06-24T14:33:20Z

...ync/src/test/functional/com/mongodb/client/AbstractClientSideOperationsTimeoutProseTest.java

+                    .getCollection(namespace.getCollectionName());
+
+            collectionHelper.runAdminCommand("{"
+                    + "    configureFailPoint: \"failCommand\","


We have com.mongodb.client.FailPoint for creating fail points in a way that cleans them up. Let's use it here and in other new tests.

There are cases where the failpoint must be configured after a specific command has executed (see, for example, commit transactions here). In that case, using the FailPoint class would require another nested try, which would make the test a bit harder to read. I do agree that using FailPoint in a try at the beginning of the test is more explicit and clear, which i assume the majority of use-cases do.

In current tests case, we could certainly use FailPoint to make it explicit and clear, but it would introduce inconsistency in this class and might raise questions of what to use for others adding new tests to this class.

Ideally, it would be beneficial if a FailPoint instance could be injected with an annotation before each test and cleaned up automatically, so that we could enable it at any point in the test as needed without try/catch via FailPoint.enable() method.

I suggest we address this in a separate ticket to unify the approach in this (and probably other test classes), and to consider automatically injected FailPoint.

What do you think?

stIncMale · 2025-06-24T14:36:53Z

...ync/src/test/functional/com/mongodb/client/AbstractClientSideOperationsTimeoutProseTest.java

+                    + "    }"
+                    + "}");
+
+            ExecutorService executor = Executors.newSingleThreadExecutor();


Let's properly shut executor down, and wait for shutting down to complete. We may also do that in an @AfterEach method, whatever seems more appropriate. This suggestion applies to all new tests in this file.

stIncMale · 2025-06-24T14:39:18Z

...ync/src/test/functional/com/mongodb/client/AbstractClientSideOperationsTimeoutProseTest.java

+        try (MongoClient mongoClient = createMongoClient(getMongoClientSettingsBuilder()
+                .timeout(100, TimeUnit.MILLISECONDS)
+                .applyToConnectionPoolSettings(builder -> builder
+                        .maxWaitTime(1, TimeUnit.MILLISECONDS)


Why do we set maxWaitTime here?

Good catch! Even though it is ignored, we don't need it here. It was a leftover from coping settings from the test above. Removed.

Add JavaDoc to StartTime. Shutdown executor properly.

Ignore maxWaitTime when CSOT is enabled.

6b3ac5f

JAVA-5409

vbabanin self-assigned this Jun 19, 2025

Remove commented-out test.

28b72c6

katcharov marked this pull request as ready for review June 19, 2025 15:06

katcharov requested a review from a team as a code owner June 19, 2025 15:06

katcharov requested review from nhachicha, a team and stIncMale and removed request for a team and nhachicha June 19, 2025 15:06

vbabanin added 2 commits June 19, 2025 09:03

Update maxWaitTimeout handling in getAsync method

2cf076b

Fix test race condition.

440891e

stIncMale requested changes Jun 24, 2025

View reviewed changes

vbabanin added 2 commits June 26, 2025 17:52

Use maxWaitTimeout consistently.

1fc6b7d

Add JavaDoc to StartTime. Shutdown executor properly.

Adjust linebreaks.

820b131

vbabanin requested a review from stIncMale June 27, 2025 01:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Ignore maxWaitTime when CSOT is enabled. #1744

Ignore maxWaitTime when CSOT is enabled. #1744

Uh oh!

vbabanin commented Jun 19, 2025

Uh oh!

stIncMale Jun 23, 2025

Uh oh!

vbabanin Jun 26, 2025 •

edited

Loading

Uh oh!

stIncMale Jun 23, 2025

Uh oh!

vbabanin Jun 27, 2025 •

edited

Loading

Uh oh!

stIncMale Jun 23, 2025

Uh oh!

stIncMale Jun 23, 2025

Uh oh!

Uh oh!

stIncMale Jun 23, 2025

Uh oh!

stIncMale Jun 24, 2025

Uh oh!

stIncMale Jun 24, 2025

Uh oh!

vbabanin Jun 27, 2025 •

edited

Loading

Uh oh!

stIncMale Jun 24, 2025

Uh oh!

stIncMale Jun 24, 2025

Uh oh!

vbabanin Jun 27, 2025 •

edited

Loading

Uh oh!

Uh oh!

Ignore maxWaitTime when CSOT is enabled. #1744

Are you sure you want to change the base?

Ignore maxWaitTime when CSOT is enabled. #1744

Uh oh!

Conversation

vbabanin commented Jun 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbabanin Jun 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbabanin Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbabanin Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vbabanin Jun 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vbabanin Jun 26, 2025 •

edited

Loading

vbabanin Jun 27, 2025 •

edited

Loading

vbabanin Jun 27, 2025 •

edited

Loading

vbabanin Jun 27, 2025 •

edited

Loading