Replace Timeout methods with branching "run" methods #1349

katcharov · 2024-03-20T18:25:32Z

This removes all production usages of the methods. Remaining work pending feedback regarding approach. Please look for TODO-CSOT when reviewing, and treat these as PR comments - some are notes for work remaining to be done, some indicate places where the approach runs into issues, and some are questions/confirmations regarding CSOT.

JAVA-5376

rozza

This its looking good and its going to be a good improvement.

Added some comments and feedback.

driver-core/src/main/com/mongodb/MongoOperationTimeoutException.java

driver-core/src/main/com/mongodb/internal/CheckedConsumer.java

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

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

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

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

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

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

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

vbabanin · 2024-03-23T06:33:34Z

driver-sync/src/main/com/mongodb/client/internal/KeyManagementService.java

+                    (ns) -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout)),
+                    () -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout)),
+                    () -> socket.getInputStream());
+        }


The Timeout API addresses the issue of race conditions, enhancing reliability. However, the cognitive load required to understand the current implementation seems to be high, particularly due to the ambiguity surrounding the roles of the first and last lambda expressions. Without directly navigating to the method definitions in an IDE, discerning the distinctions between these lambda expressions is challenging.

return Timeout.checkedRun(operationTimeout, NANOSECONDS, () -> socket.getInputStream(), (ns) -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout)), () -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout)), () -> socket.getInputStream()); }

To enhance readability, I suggest making the code more descriptive and self-explanatory. Introducing named methods that clearly convey the actions being performed can simplify comprehension for anyone reviewing the code.

For instance, consider the following revised implementation:

return Timeout.checkedRun(operationTimeout, NANOSECONDS, onInfinite(socket::getInputStream), onNull(socket::getInputStream), onRemaining((ns) -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout))), onExpired(() -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout)));

This approach could involve a set of static methods that return specific Supplier/Consumer types, such as InfiniteSupplier or NullableSupplier. Alternatively, we could design a TimeoutStep class that accumulates lambdas until an execute method is called, providing a fluent interface for configuring timeout behavior:

return Timeout.of(operationTimeout, NANOSECONDS) .onInfinite(socket::getInputStream) .onNull(socket::getInputStream) .onRemaining((ns) -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout))) .onExpired(() -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout))) .execute();

It might make the code more more intuitive and also facilitate easier modifications and debugging in the future. @rozza @katcharov what do you think?

P.S.: I don't have a strong preference for the specific approach to making the code more descriptive; my primary concern is that the current state can be confusing.

Perhaps the underlying problem that you identify, of the first and last params, is solved by adding nullAsInfinite, which I have pushed up:

return Timeout.nullAsInfinite(operationTimeout).checkedRun(NANOSECONDS, () -> socket.getInputStream(), (ns) -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout)), () -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout))); }

This makes it explicit why the first and last are the same, and removes redundant code. It also facilitates extracting that wrapping, which I think belongs outside of the method. It also means we can get rid of static variants.

I found it helpful when implementing to think of params as going from "least expired to most expired" (infinite never expires, then remaining, then expired). I am not sure how helpful this is for others, but I do not recall needing to check param order when making PR changes. Perhaps this "order mnemonic" addresses remaining readability issues? (Early on, @stIncMale and I briefly discussed the fluent approach. I think it would take more work and design, and would have its own downsides.)

I agree with @vbabanin using named handlers over unnamed parameters makes the code self explaining / readable. That said this pattern is similar to how a reactive stream Subscriber has named events, but then libraries (Reactor) often add helpers eg: subscribe(Subscriber) gets extended out to subscribe(OnNext), subscribe(OnNext, onError), subscribe(OnNext, OnError, OnComplete) . Once you start using it and get familiar with it you often use the convience methods. 🤷

To clarify my point - I think its fine to leave as is as its an internal API.

I understand the rationale behind the "least expired to most expired" parameter organization. However, concerns arise regarding its long-term clarity and potential for error as this implicit understanding might not be as apparent in the future.

The approach with fluent API, while initially demanding more effort, promises maintainability in the future, making it easier to understand, modify, and debug.

Given that @stIncMale and @rozza are comfortable with the current implementation and the existing approvals on the PR, I'm willing to support the consensus.

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

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

# Conflicts: # driver-core/src/main/com/mongodb/internal/TimeoutContext.java # driver-core/src/main/com/mongodb/internal/operation/AbortTransactionOperation.java # driver-core/src/main/com/mongodb/internal/operation/AsyncCommandBatchCursor.java # driver-core/src/main/com/mongodb/internal/operation/BaseFindAndModifyOperation.java # driver-core/src/main/com/mongodb/internal/operation/CommandBatchCursor.java # driver-core/src/main/com/mongodb/internal/operation/CommandBatchCursorHelper.java # driver-core/src/main/com/mongodb/internal/operation/CommandOperationHelper.java # driver-core/src/main/com/mongodb/internal/operation/CountOperation.java # driver-core/src/main/com/mongodb/internal/operation/CreateIndexesOperation.java # driver-core/src/main/com/mongodb/internal/operation/DistinctOperation.java # driver-core/src/main/com/mongodb/internal/operation/DropIndexOperation.java # driver-core/src/main/com/mongodb/internal/operation/EstimatedDocumentCountOperation.java # driver-core/src/main/com/mongodb/internal/operation/OperationHelper.java

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

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

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

katcharov · 2024-03-28T19:25:51Z

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

 import com.mongodb.internal.time.StartTime;
 import com.mongodb.internal.time.Timeout;
 import com.mongodb.lang.Nullable;
 import com.mongodb.session.ClientSession;

 import java.util.Objects;
-import java.util.Optional;
-import java.util.function.Supplier;
+import java.util.concurrent.TimeUnit;


Please see https://github.com/mongodb/mongo-java-driver/pull/1215/files#r1543521955 (@rozza or @vbabanin)

katcharov · 2024-03-28T21:08:28Z

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

-        if (timeoutMS == null) {
-            return alternativeTimeoutMS;
-        } else if (timeoutMS == 0) {
+        if (timeout == null) {


This change assumes that it is impossible for timeout to be null when timeoutSettings.getTimeoutMS() returns a non-null value. Please confirm.

The API doesn't guarantee this as the Timeout can be passed in as well as TimeOutSettings to a constructor.

It appears that in practice, based on usages, timeout is never null in such cases. The constructors are always invoked where the timeout is derived via startTimeout, or have a non-null value. Also, if this were not the case, then I think resetTimeout has a bug, because it derives the timeout based on timeoutSettings.getTimeoutMS.

Refactored a bit.

katcharov · 2024-03-28T21:13:43Z

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

@@ -166,29 +161,15 @@ private boolean hasTimedOutForCommandExecution() {
     * @return timeout to use.
     */
    public long timeoutOrAlternative(final long alternativeTimeoutMS) {


This timeoutOrAlternative method should ideally be inlined, with getMaxCommitTimeMS, getReadTimeoutMS, and getWriteTimeoutMS encapsulating the result using lambdas, as was done in runMaxTimeMSTimeout. This requires more associated refactoring than seems appropriate for this PR.

katcharov · 2024-03-28T21:15:24Z

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

@@ -48,16 +48,21 @@ public class TimeoutContext {
    private Timeout computedServerSelectionTimeout;
    private long minRoundTripTimeMS = 0;

-    private MaxTimeSupplier maxTimeSupplier = this::calculateMaxTimeMS;
+    @Nullable
+    private MaxTimeSupplier maxTimeSupplier = null;


Resetting to null to designate a default seemed clearer, and made the default call more obvious in the context of the logic where this was used (which is one place).

katcharov · 2024-03-28T21:29:31Z

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

-        return notNull("Should never be null", maxTimeSupplier.get());
-    }
+    public void runMaxTimeMSTimeout(final Runnable onInfinite, final LongConsumer onRemaining,
+            final Runnable onExpired) {


Considered and tried various alternatives:

Returning the remaining time leaks internals (which has led to bugs).

Returning a live Timeout causes unified tests to fail, because a timeout of 100ms will be sent to the server as 99ms as soon as any nanoseconds have elapsed.

Could return a fixed value under a new type of timeout, which would pass that fixed value to its run method (that is, it would not be a timeout, but a configuration value provider); but this is a stretch, can't be compared for "earlier" against other timeouts.

This chosen approach follows the pattern of existing timeout runs, and without leaking internals, breaking tests, or abusing the Timeout type.

rozza · 2024-04-02T14:59:06Z

config/spotbugs/exclude.xml

@@ -280,17 +280,14 @@
    <!-- Ignoring await return; intended to be used in a loop -->
    <Match>
        <Class name="com.mongodb.internal.time.Timeout"/>


Should these blocks be removed as there is no method attached?

Refactoring moved the issue into a lambda, and IIRC there is no way to match on a lambda due to tool limitations. This "match" applies to all methods in the class.

Ah, good to know

rozza · 2024-04-03T09:52:55Z

driver-sync/src/main/com/mongodb/client/internal/KeyManagementService.java

+                    (ns) -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout)),
+                    () -> new OperationTimeoutAwareInputStream(socket, assertNotNull(operationTimeout)),
+                    () -> socket.getInputStream());
+        }


To clarify my point - I think its fine to leave as is as its an internal API.

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

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

stIncMale

I have not reviewed everything (mostly the new API). Hopefully, the combination of all reviews covers the changes in full.

rozza

LGTM - any other items can be iterated on

# Conflicts: # driver-core/src/main/com/mongodb/internal/TimeoutContext.java

katcharov · 2024-04-09T14:51:26Z

Merged CSOT, 1 conflict: removed timeoutRemainingMS and its temp fix (see 4e97f21, "added temp fix for timeoutRemainingMS (scheduled to be removed)").

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

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

Replace Timeout methods with branching "run" methods

76e07d0

JAVA-5376

rozza reviewed Mar 21, 2024

View reviewed changes

rozza mentioned this pull request Mar 21, 2024

Override collection/database with infinite timeout. #1350

Merged

Fix tests (remove mocks)

64ae4d3

katcharov commented Mar 21, 2024

View reviewed changes

driver-core/src/main/com/mongodb/internal/connection/InternalStreamConnection.java Outdated Show resolved Hide resolved

katcharov added 2 commits March 22, 2024 15:18

temp

4050052

PR fixes, refactoring, fix typos

3017f07

vbabanin reviewed Mar 25, 2024

View reviewed changes

katcharov mentioned this pull request Mar 25, 2024

CSOT #1215

Merged

katcharov added 5 commits March 25, 2024 11:08

Use nullAsInfinite

9f7a9af

PR fixes

36cc604

Fix compile, checkstyle

22175a1

Refactor timeout calculations

c84adc4

Fix checkstyle

07a1454

rozza reviewed Mar 26, 2024

View reviewed changes

driver-core/src/main/com/mongodb/internal/TimeoutContext.java Outdated Show resolved Hide resolved

katcharov added 3 commits March 26, 2024 12:42

Post-merge resolutions of maxTimeMS; shortenBy only timeoutMS

d480fa0

Fix refactoring bug

42a55ab

stIncMale requested changes Mar 27, 2024

View reviewed changes

katcharov added 2 commits March 27, 2024 12:14

Accommodate timeouts that begin when used

d91e9d6

PR fixes

30f3e8e

katcharov commented Mar 28, 2024

View reviewed changes

katcharov added 2 commits March 28, 2024 14:10

PR fixes, rename

3feb9ed

Remove Timeout methods, remaining TODOs, PR fixes

fd8fb83

katcharov commented Mar 28, 2024

View reviewed changes

Cleanup, fixes

acfe871

katcharov requested a review from stIncMale March 28, 2024 22:51

katcharov requested review from vbabanin and rozza March 28, 2024 22:51

katcharov marked this pull request as ready for review March 28, 2024 22:51

katcharov added 2 commits March 28, 2024 17:49

Fix copy-paste error

1f89e94

Encapsulate constructors

5ef637f

rozza reviewed Apr 3, 2024

View reviewed changes

stIncMale requested changes Apr 4, 2024

View reviewed changes

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

driver-core/src/main/com/mongodb/internal/time/Timeout.java Outdated Show resolved Hide resolved

katcharov requested review from stIncMale and rozza April 4, 2024 14:26

katcharov force-pushed the JAVA-5376 branch from beae95b to d393a25 Compare April 4, 2024 14:31

PR fixes

b26c9ed

katcharov force-pushed the JAVA-5376 branch from d393a25 to b26c9ed Compare April 4, 2024 14:40

Checkstyle

46fd71c

stIncMale approved these changes Apr 5, 2024

View reviewed changes

rozza approved these changes Apr 9, 2024

View reviewed changes

Merge remote-tracking branch 'rozza/CSOT' into JAVA-5376

42432ae

# Conflicts: # driver-core/src/main/com/mongodb/internal/TimeoutContext.java

Post-merge fixes

e842e4d

mongodb deleted a comment from ross Apr 10, 2024

vbabanin reviewed Apr 11, 2024

View reviewed changes

driver-core/src/main/com/mongodb/internal/time/Timeout.java Outdated Show resolved Hide resolved

vbabanin reviewed Apr 11, 2024

View reviewed changes

driver-core/src/main/com/mongodb/internal/TimeoutContext.java Show resolved Hide resolved

vbabanin approved these changes Apr 11, 2024

View reviewed changes

katcharov added 2 commits April 11, 2024 16:26

PR Fixes

bcb83a4

Checkstyle

1a944f4

katcharov merged commit 3b6ba32 into mongodb:CSOT Apr 15, 2024
56 of 58 checks passed

katcharov deleted the JAVA-5376 branch April 15, 2024 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Timeout methods with branching "run" methods #1349

Replace Timeout methods with branching "run" methods #1349

katcharov commented Mar 20, 2024

rozza left a comment

vbabanin Mar 23, 2024 •

edited

Loading

katcharov Mar 25, 2024 •

edited

Loading

rozza Mar 26, 2024 •

edited

Loading

rozza Apr 3, 2024

vbabanin Apr 10, 2024 •

edited

Loading

katcharov Mar 28, 2024

katcharov Mar 28, 2024

rozza Apr 2, 2024

katcharov Apr 2, 2024

katcharov Mar 28, 2024

katcharov Mar 28, 2024

katcharov Mar 28, 2024

rozza Apr 2, 2024

katcharov Apr 4, 2024

rozza Apr 8, 2024

rozza Apr 3, 2024

stIncMale left a comment

rozza left a comment

katcharov commented Apr 9, 2024

Replace Timeout methods with branching "run" methods #1349

Replace Timeout methods with branching "run" methods #1349

Conversation

katcharov commented Mar 20, 2024

rozza left a comment

Choose a reason for hiding this comment

vbabanin Mar 23, 2024 • edited Loading

Choose a reason for hiding this comment

katcharov Mar 25, 2024 • edited Loading

Choose a reason for hiding this comment

rozza Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vbabanin Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stIncMale left a comment

Choose a reason for hiding this comment

rozza left a comment

Choose a reason for hiding this comment

katcharov commented Apr 9, 2024

vbabanin Mar 23, 2024 •

edited

Loading

katcharov Mar 25, 2024 •

edited

Loading

rozza Mar 26, 2024 •

edited

Loading

vbabanin Apr 10, 2024 •

edited

Loading