Replace Timeout implementation with interface, introduce StartTime interface #1194

katcharov · 2023-09-11T21:04:13Z

orca-security-us

Orca Security Scan Summary

Status	Check	Issues by priority
Passed	Infrastructure as Code	0 0 0 0	View in Orca
Passed	Vulnerabilities	0 0 0 0	View in Orca
Passed	Secrets	0 0 0 0	View in Orca

katcharov · 2023-09-11T21:07:37Z

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

                }
-                long remainingNanos = timeout.remainingOrInfinite(NANOSECONDS);
                while (permits == 0
                        // the absence of short-circuiting is of importance
                        & !stateAndGeneration.throwIfClosedOrPaused()
                        & (availableConnection = tryGetAvailable ? tryGetAvailableConnection() : null) == null) {
-                    if (Timeout.expired(remainingNanos)) {
-                        throw createTimeoutException(timeout);
+                    if (timeout.hasExpired()) {
+                        throw createTimeoutException(startTime);
                    }
-                    remainingNanos = awaitNanos(permitAvailableOrHandedOverOrClosedOrPausedCondition, remainingNanos);
+                    timeout.awaitOn(permitAvailableOrHandedOverOrClosedOrPausedCondition);
                }


Note that the timeout is now checked directly, rather than the value from await.

katcharov · 2023-09-11T21:08:47Z

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

-         * @return The remaining duration as per {@link Timeout#remainingOrInfinite(TimeUnit)} if waiting ended early either
-         * spuriously or because of receiving a signal.
-         */
-        private long awaitNanos(final Condition condition, final long timeoutNanos) throws MongoInterruptedException {


This has been moved into the Deadline.

katcharov · 2023-09-11T21:11:03Z

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

+/**
+ * @see TimePoint
+ */
+public interface Deadline {


I created the Deadline and StartPoint interfaces on TimePoint. These hide TimePoint methods that are not relevant or used, and also make it clear what "type of" TimePoint we are using.

katcharov · 2023-09-11T21:23:49Z

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

+     * The number of whole time units that remain until this TimePoint
+     * {@link #hasExpired()}. This should not be used to check for expiry,
+     * but can be used to supply a remaining value, in the finest-grained
+     * TimeUnit available, to some method that may time out.
+     * This method must not be used with infinite TimePoints.


This changes the behaviour of the remaining method.

Previously, if there were 100ns remaining, and you asked how many hours were remaining, it would return 1 hour, because returning 0 might suggest that the deadline is expired. However, this does not match the behaviour of Duration, and I do not think rounding up is correct, for the expected usage. For example, I might get the remaining time in hours because that is the granularity of some method that I will invoke, and get back 1, and wait on the method for the next hour, which is not correct. As far as "hours" go, the deadline is indeed expired, and I do not think we should undertake operations that might (because of granularity differences) exceed the timeout. The same logic applies to milliseconds, though less intuitively.

I spoke with @stIncMale about this, and he explained that the existing rounding-up behaviour is generally consistent with the timeout interpretation elsewhere in Java, of "try at least this long and then time out as fast as possible". I agree this is not an incorrect interpretation, but we also agree that the appropriate way to address this particular issue is to use nanoseconds (as Java SE does), rather than rounding the value up and down to some granularity. I won't address this in this PR (that is, refactoring our existing methods). I will leave this rounding-up interpretation, since it is consistent with how "elapsed" works, and the inconsistency with Java SE is not crucial (and should ideally be addressed in a different way regardless).

katcharov · 2023-09-11T21:26:36Z

driver-sync/src/test/functional/com/mongodb/client/ServerDiscoveryAndMonitoringProseTests.java

        Set<Class<?>> encountered = new HashSet<>();
        while (true) {
            Object element;
-            if (timeout.isImmediate()) {
+            if (deadline.hasExpired()) {


This removes the concept of "immediate" timepoints: they are just expired.

katcharov · 2023-09-11T21:29:32Z

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

+    /**
+     * Returns an {@linkplain Deadline#isInfinite() infinite} deadline if
+     * {@code timeoutValue} is negative, an expired deadline if
+     * {@code timeoutValue} is 0, otherwise a deadline in {@code durationNanos}.
+     * <p>
+     * Note that some code might ignore a deadline, and attempt to perform
+     * the operation in question at least once.</p>
+     * <p>
+     * Note that the contract of this method is also used in some places to
+     * specify the behavior of methods that accept {@code (long timeout, TimeUnit unit)},
+     * e.g., {@link com.mongodb.internal.connection.ConcurrentPool#get(long, TimeUnit)},
+     * so it cannot be changed without updating those methods.</p>
+     *
+     * @see TimePoint#fromNowOrInfiniteIfNegative(long, TimeUnit)
+     */
+    Deadline fromNowOrInfiniteIfNegative(long timeoutValue, TimeUnit timeUnit);


I think perhaps this method should not be added, but I have included it because otherwise, it seems I would need to change existing locations that take long/TimeUnit in this PR. I have at least made the name long and noticeable.

rozza

Looking good - would be worthwhile having @stIncMale review as well as he authored the original Timeout class.

rozza · 2023-09-13T13:02:01Z

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

                }
-                long remainingNanos = timeout.remainingOrInfinite(NANOSECONDS);
                while (permits == 0
                        // the absence of short-circuiting is of importance
                        & !stateAndGeneration.throwIfClosedOrPaused()
                        & (availableConnection = tryGetAvailable ? tryGetAvailableConnection() : null) == null) {
-                    if (Timeout.expired(remainingNanos)) {
-                        throw createTimeoutException(timeout);
+                    if (timeout.hasExpired()) {
+                        throw createTimeoutException(startTime);
                    }
-                    remainingNanos = awaitNanos(permitAvailableOrHandedOverOrClosedOrPausedCondition, remainingNanos);
+                    timeout.awaitOn(permitAvailableOrHandedOverOrClosedOrPausedCondition);
                }


rozza · 2023-09-13T13:02:09Z

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

-         * @return The remaining duration as per {@link Timeout#remainingOrInfinite(TimeUnit)} if waiting ended early either
-         * spuriously or because of receiving a signal.
-         */
-        private long awaitNanos(final Condition condition, final long timeoutNanos) throws MongoInterruptedException {


rozza · 2023-09-13T13:04:41Z

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

+     * The number of whole time units that remain until this TimePoint
+     * {@link #hasExpired()}. This should not be used to check for expiry,
+     * but can be used to supply a remaining value, in the finest-grained
+     * TimeUnit available, to some method that may time out.
+     * This method must not be used with infinite TimePoints.


rozza · 2023-09-13T13:16:45Z

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

+            } else {
+                // the deadline will track this remaining time
+                //noinspection ResultOfMethodCallIgnored
+                condition.awaitNanos(remaining(NANOSECONDS));


I think the documentation should be clarified to ensure that hasExpired is checked after calling awaitOn as the condition may not be satisfied with in the deadline.

I added docs. It seems it is usually checked before calling await (though, this is generally in a loop).

I considered changing the method to perform the check, but there were some complications that I ran into elsewhere (that might be noted in a future PR).

stIncMale · 2023-09-13T16:14:01Z

The idea behind JAVA-5112 was that instead of having two sets of APIs, Timeout and TimePoint, we only use one TimePoint API (a quote from the ticket: "If we do this, we won't need to have two APIs, Timeout and TimePoint, we will only need TimePoint."). That was supposed to simplify things. This PR does not do that, it still leaves two APIs (StartTime, Deadline), with a third program element TimePoint that implements them. Furthermore, the Deadline API is hardly different from the Timeout API.

As far as I can see, this PR does not do the improvement that was intended by JAVA-5112. What is the motivation behind this PR?

katcharov · 2023-09-19T21:25:53Z

(From earlier discussion, to track responses to the points above.)

It seems beneficial, whatever we do, to have a single implementation (more maintainable and testable), and also a single abstraction (a plain point in time), and my opinion is that these are the primary benefits. The outstanding point is whether we should differentiate two "types of" TimePoint (how we decide what counts as a single API is tangential). I believe there is little harm in differentiating because there is no significant API duplication between the interfaces, and a benefit is that the compiler now prevents us from confusing the two, or using methods from one unsuitable in the other (that is, TimePoints are always strictly either Deadlines or StartTimes).

katcharov · 2023-09-19T21:35:20Z

I renamed Deadlines to Timeouts because, while the term "deadline" is clearer, it would be burdensome to change existing usages, and would conflict with public-facing terminology. This is a naming change only - Timeouts are still trimmed down, no longer have start-times built in, etc.

Also changed target to the CSOT feature branch.

rozza

Looks good - main comment is to add a little more back ground to StartTime but I like it :)

rozza · 2023-09-20T16:08:22Z

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

+     * The number of whole time units that remain until this TimePoint
+     * {@link #hasExpired()}. This should not be used to check for expiry,
+     * but can be used to supply a remaining value, in the finest-grained
+     * TimeUnit available, to some method that may time out.
+     * This method must not be used with infinite TimePoints.


rozza · 2023-09-20T16:09:27Z

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

@@ -75,7 +75,7 @@ public boolean hasTimeoutMS() {
     * @return true if the timeout has been set and it has expired
     */
    public boolean expired() {


We should change this to hasExpired at some point - just to keep the nomenclature consistent. Doesn't have to be done in this PR.

rozza · 2023-09-20T16:12:19Z

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

+import java.util.concurrent.TimeUnit;
+
+/**
+ * @see TimePoint


I think perhaps there should be more docs - just explaining why this interface exists.

As TimePoint.now could be added and all functionality is covered by Timepoint.

Added. Both StartTime and Timeout are intended to remove functionality to provide the (now-documented) typesafe guarantees, and to clarify the purpose or intended use of any value of that type.

driver-core/src/test/unit/com/mongodb/internal/time/TimeoutTest.java

rozza · 2023-09-22T12:35:15Z

@katcharov I think you need to push a commit?

rozza

LGTM

katcharov requested a review from rozza September 11, 2023 21:04

orca-security-us bot reviewed Sep 11, 2023

View reviewed changes

katcharov commented Sep 11, 2023

View reviewed changes

katcharov force-pushed the JAVA-5112 branch 2 times, most recently from c0e5a15 to 65ceb29 Compare September 12, 2023 20:55

rozza requested changes Sep 13, 2023

View reviewed changes

katcharov requested a review from stIncMale September 13, 2023 15:17

katcharov added 2 commits September 19, 2023 11:30

Remove Timeout, introduce StartTime and Deadline

9e38707

Rename Deadline to Timeout

ed06cc0

katcharov force-pushed the JAVA-5112 branch from 65ceb29 to ed06cc0 Compare September 19, 2023 20:53

katcharov changed the title ~~Remove Timeout, introduce StartTime and Deadline~~ Replace Timeout implementation with interface, introduce StartTime interface Sep 19, 2023

katcharov changed the base branch from master to CSOT September 19, 2023 20:54

rozza requested changes Sep 20, 2023

View reviewed changes

katcharov requested a review from rozza September 21, 2023 22:09

PR fixes, rename

5dce66e

rozza approved these changes Sep 22, 2023

View reviewed changes

katcharov merged commit 8f710f3 into mongodb:CSOT Sep 22, 2023

katcharov deleted the JAVA-5112 branch September 22, 2023 15:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace Timeout implementation with interface, introduce StartTime interface #1194

Replace Timeout implementation with interface, introduce StartTime interface #1194

katcharov commented Sep 11, 2023

orca-security-us bot left a comment

katcharov Sep 11, 2023

rozza Sep 13, 2023

katcharov Sep 11, 2023

rozza Sep 13, 2023

katcharov Sep 11, 2023

katcharov Sep 11, 2023

rozza Sep 13, 2023

katcharov Sep 14, 2023

rozza Sep 20, 2023

katcharov Sep 11, 2023

katcharov Sep 11, 2023

rozza left a comment

rozza Sep 13, 2023

rozza Sep 13, 2023

rozza Sep 13, 2023

rozza Sep 13, 2023

katcharov Sep 21, 2023

stIncMale commented Sep 13, 2023 •

edited

Loading

katcharov commented Sep 19, 2023

katcharov commented Sep 19, 2023

rozza left a comment

rozza Sep 20, 2023

rozza Sep 20, 2023

rozza Sep 20, 2023

katcharov Sep 21, 2023 •

edited

Loading

rozza commented Sep 22, 2023

rozza left a comment

Replace Timeout implementation with interface, introduce StartTime interface #1194

Replace Timeout implementation with interface, introduce StartTime interface #1194

Conversation

katcharov commented Sep 11, 2023

orca-security-us bot left a comment

Choose a reason for hiding this comment

Orca Security Scan Summary

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

rozza left a 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 commented Sep 13, 2023 • edited Loading

katcharov commented Sep 19, 2023

katcharov commented Sep 19, 2023

rozza left a 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

katcharov Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

rozza commented Sep 22, 2023

rozza left a comment

Choose a reason for hiding this comment

stIncMale commented Sep 13, 2023 •

edited

Loading

katcharov Sep 21, 2023 •

edited

Loading