Skip to content
Merged
Show file tree
Hide file tree
Changes from 16 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
3 changes: 0 additions & 3 deletions config/spotbugs/exclude.xml
Original file line number Diff line number Diff line change
Expand Up @@ -280,17 +280,14 @@
<!-- Ignoring await return; intended to be used in a loop -->
<Match>
<Class name="com.mongodb.internal.time.Timeout"/>
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, good to know

<Method name="awaitOn"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED_BAD_PRACTICE"/>
</Match>
<Match>
<Class name="com.mongodb.internal.time.Timeout"/>
<Method name="awaitOn"/>
<Bug pattern="RV_RETURN_VALUE_IGNORED"/>
</Match>
<Match>
<Class name="com.mongodb.internal.time.Timeout"/>
<Method name="awaitOn"/>
<Bug pattern="WA_AWAIT_NOT_IN_LOOP"/>
</Match>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ public final class MongoOperationTimeoutException extends MongoTimeoutException
* @param message the message
*/
public MongoOperationTimeoutException(final String message) {
// TODO (CSOT) - JAVA-5248 move all messages here as constants to ensure consistency?
super(message);
}

Expand Down
1 change: 1 addition & 0 deletions driver-core/src/main/com/mongodb/internal/Locks.java
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.mongodb.internal;

import com.mongodb.MongoInterruptedException;
import com.mongodb.internal.function.CheckedSupplier;

import java.util.concurrent.locks.Lock;
import java.util.concurrent.locks.ReentrantLock;
Expand Down
149 changes: 71 additions & 78 deletions driver-core/src/main/com/mongodb/internal/TimeoutContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@
import com.mongodb.session.ClientSession;

import java.util.Objects;
import java.util.Optional;
import java.util.function.Supplier;
import java.util.function.LongConsumer;

import static com.mongodb.assertions.Assertions.assertNotNull;
import static com.mongodb.assertions.Assertions.assertNull;
import static com.mongodb.assertions.Assertions.isTrue;
import static com.mongodb.assertions.Assertions.notNull;
import static com.mongodb.internal.VisibleForTesting.AccessModifier.*;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static java.util.concurrent.TimeUnit.NANOSECONDS;

/**
* Timeout Context.
Expand All @@ -48,16 +48,21 @@ public class TimeoutContext {
private Timeout computedServerSelectionTimeout;
private long minRoundTripTimeMS = 0;

private MaxTimeSupplier maxTimeSupplier = this::calculateMaxTimeMS;
@Nullable
private MaxTimeSupplier maxTimeSupplier = null;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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).


public static MongoOperationTimeoutException createMongoTimeoutException() {
return createMongoTimeoutException("Remaining timeoutMS is less than the servers minimum round trip time.");
public static MongoOperationTimeoutException createMongoRoundTripTimeoutException() {
return createMongoTimeoutException("Remaining timeoutMS is less than the server's minimum round trip time.");
}

public static MongoOperationTimeoutException createMongoTimeoutException(final String message) {
return new MongoOperationTimeoutException(message);
}

public static void throwMongoTimeoutException(final String message) {
throw new MongoOperationTimeoutException(message);
}

public static MongoOperationTimeoutException createMongoTimeoutException(final Throwable cause) {
return createMongoTimeoutException("Operation timed out: " + cause.getMessage(), cause);
}
Expand All @@ -70,7 +75,7 @@ public static MongoOperationTimeoutException createMongoTimeoutException(final S
}

public static TimeoutContext createMaintenanceTimeoutContext(final TimeoutSettings timeoutSettings) {
return new TimeoutContext(true, timeoutSettings, calculateTimeout(timeoutSettings.getTimeoutMS()));
return new TimeoutContext(true, timeoutSettings, startTimeout(timeoutSettings.getTimeoutMS()));
}

public static TimeoutContext createTimeoutContext(final ClientSession session, final TimeoutSettings timeoutSettings) {
Expand Down Expand Up @@ -101,7 +106,7 @@ public static TimeoutContext createTimeoutContext(final ClientSession session, f
}

public TimeoutContext(final TimeoutSettings timeoutSettings) {
this(false, timeoutSettings, calculateTimeout(timeoutSettings.getTimeoutMS()));
this(false, timeoutSettings, startTimeout(timeoutSettings.getTimeoutMS()));
}

public TimeoutContext(final TimeoutSettings timeoutSettings, @Nullable final Timeout timeout) {
Expand Down Expand Up @@ -129,8 +134,8 @@ public boolean hasTimeoutMS() {
* @return true if the timeout has been set and it has expired
*/
public boolean hasExpired() {
// Use timeout.remaining instead of timeout.hasExpired that measures in nanoseconds.
return timeout != null && !timeout.isInfinite() && timeout.remaining(MILLISECONDS) <= 0;
// TODO (CSOT) this method leaks Timeout internals, should be removed (not inlined, but inverted using lambdas)
return Timeout.nullAsInfinite(timeout).call(NANOSECONDS, () -> false, (ns) -> false, () -> true);
}

/**
Expand All @@ -144,19 +149,9 @@ public TimeoutContext minRoundTripTimeMS(final long minRoundTripTimeMS) {
return this;
}

public Optional<MongoOperationTimeoutException> validateHasTimedOutForCommandExecution() {
if (hasTimedOutForCommandExecution()) {
return Optional.of(createMongoTimeoutException());
}
return Optional.empty();
}

private boolean hasTimedOutForCommandExecution() {
if (timeout == null || timeout.isInfinite()) {
return false;
}
long remaining = timeout.remaining(MILLISECONDS);
return remaining <= 0 || minRoundTripTimeMS > remaining;
@Nullable
public Timeout timeoutIncludingRoundTrip() {
return timeout == null ? null : timeout.shortenBy(minRoundTripTimeMS, MILLISECONDS);
}

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

Choose a reason for hiding this comment

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

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.

Long timeoutMS = timeoutSettings.getTimeoutMS();
if (timeoutMS == null) {
return alternativeTimeoutMS;
}
return timeoutRemainingMS();
}

/**
* Calculates the minimum timeout value between two possible timeouts.
*
* @param alternativeTimeoutMS the alternative timeout
* @return the minimum value to use.
*/
public long calculateMin(final long alternativeTimeoutMS) {
Long timeoutMS = timeoutSettings.getTimeoutMS();
if (timeoutMS == null) {
return alternativeTimeoutMS;
} else if (timeoutMS == 0) {
if (timeout == null) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

return alternativeTimeoutMS;
} else if (alternativeTimeoutMS == 0) {
return timeoutRemainingMS();
} else {
return Math.min(timeoutRemainingMS(), alternativeTimeoutMS);
return timeout.call(MILLISECONDS,
() -> 0L,
(ms) -> ms,
() -> {
throw createMongoTimeoutException("The operation timeout has expired.");
});
}
}

Expand All @@ -200,30 +181,43 @@ public long getMaxAwaitTimeMS() {
return timeoutSettings.getMaxAwaitTimeMS();
}

public long getMaxTimeMS() {
return notNull("Should never be null", maxTimeSupplier.get());
}
public void runMaxTimeMSTimeout(final Runnable onInfinite, final LongConsumer onRemaining,
final Runnable onExpired) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Considered and tried various alternatives:

  1. Returning the remaining time leaks internals (which has led to bugs).
  2. 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.
  3. 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.

if (maxTimeSupplier != null) {
runWithFixedTimout(maxTimeSupplier.get(), onInfinite, onRemaining);
return;
}

private long calculateMaxTimeMS() {
long maxTimeMS = timeoutOrAlternative(timeoutSettings.getMaxTimeMS());
if (timeout == null || timeout.isInfinite()) {
return maxTimeMS;
if (timeout != null) {
timeout.shortenBy(minRoundTripTimeMS, MILLISECONDS)
.run(MILLISECONDS, onInfinite, onRemaining, onExpired);
} else {
runWithFixedTimout(timeoutSettings.getMaxTimeMS(), onInfinite, onRemaining);
}
if (minRoundTripTimeMS >= maxTimeMS) {
throw createMongoTimeoutException();
}

private static void runWithFixedTimout(final long ms, final Runnable onInfinite, final LongConsumer onRemaining) {
if (ms == 0) {
onInfinite.run();
} else {
onRemaining.accept(ms);
}
return maxTimeMS - minRoundTripTimeMS;
}

public void setMaxTimeSupplier(final MaxTimeSupplier maxTimeSupplier) {
this.maxTimeSupplier = maxTimeSupplier;
public void resetToDefaultMaxTime() {
this.maxTimeSupplier = null;
}

public void resetToDefaultMaxTimeSupplier() {
this.maxTimeSupplier = this::calculateMaxTimeMS;
public void setMaxTimeOverride(final long maxTimeMS) {
this.maxTimeSupplier = () -> maxTimeMS;
}

public Long getMaxCommitTimeMS() {
public void setMaxTimeOverrideToMaxCommitTime() {
this.maxTimeSupplier = () -> getMaxCommitTimeMS();
}

@VisibleForTesting(otherwise = PRIVATE)
public long getMaxCommitTimeMS() {
Long maxCommitTimeMS = timeoutSettings.getMaxCommitTimeMS();
return timeoutOrAlternative(maxCommitTimeMS != null ? maxCommitTimeMS : 0);
}
Expand All @@ -236,22 +230,30 @@ public long getWriteTimeoutMS() {
return timeoutOrAlternative(0);
}

public int getConnectTimeoutMs() {
return (int) calculateMin(getTimeoutSettings().getConnectTimeoutMS());
public Timeout createConnectTimeoutMs() {
// null timeout treated as infinite will be later than the other

return Timeout.earliest(
Timeout.expiresIn(getTimeoutSettings().getConnectTimeoutMS(), MILLISECONDS, Timeout.ZeroDurationIs.INFINITE),
Timeout.nullAsInfinite(timeout));
}

public void resetTimeout() {
assertNotNull(timeout);
timeout = calculateTimeout(timeoutSettings.getTimeoutMS());
timeout = startTimeout(timeoutSettings.getTimeoutMS());
}

/**
* Resets the timeout if this timeout context is being used by pool maintenance
*/
public void resetMaintenanceTimeout() {
if (isMaintenanceContext && timeout != null && !timeout.isInfinite()) {
timeout = calculateTimeout(timeoutSettings.getTimeoutMS());
if (!isMaintenanceContext) {
return;
}
timeout = Timeout.nullAsInfinite(timeout).call(NANOSECONDS,
() -> timeout,
(ms) -> startTimeout(timeoutSettings.getTimeoutMS()),
() -> startTimeout(timeoutSettings.getTimeoutMS()));
}

public TimeoutContext withAdditionalReadTimeout(final int additionalReadTimeout) {
Expand All @@ -267,14 +269,6 @@ public TimeoutContext withAdditionalReadTimeout(final int additionalReadTimeout)
return new TimeoutContext(timeoutSettings.withReadTimeoutMS(newReadTimeout > 0 ? newReadTimeout : Long.MAX_VALUE));
}

private long timeoutRemainingMS() {
assertNotNull(timeout);
if (timeout.hasExpired()) {
throw createMongoTimeoutException("The operation timeout has expired.");
}
return timeout.isInfinite() ? 0 : timeout.remaining(MILLISECONDS);
}

@Override
public String toString() {
return "TimeoutContext{"
Expand Down Expand Up @@ -306,9 +300,9 @@ public int hashCode() {
}

@Nullable
public static Timeout calculateTimeout(@Nullable final Long timeoutMS) {
public static Timeout startTimeout(@Nullable final Long timeoutMS) {
if (timeoutMS != null) {
return timeoutMS == 0 ? Timeout.infinite() : Timeout.expiresIn(timeoutMS, MILLISECONDS);
return Timeout.expiresIn(timeoutMS, MILLISECONDS, Timeout.ZeroDurationIs.INFINITE);
}
return null;
}
Expand All @@ -334,7 +328,7 @@ public Timeout computeServerSelectionTimeout() {
return serverSelectionTimeout;
}

if (serverSelectionTimeout.orEarlier(timeout) == timeout) {
if (timeout != null && Timeout.earliest(serverSelectionTimeout, timeout) == timeout) {
return timeout;
}

Expand Down Expand Up @@ -364,8 +358,7 @@ public Timeout getTimeout() {
return timeout;
}

public interface MaxTimeSupplier extends Supplier<Long> {
@Override
Long get();
public interface MaxTimeSupplier {
long get();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

/**
* This package contains cluster and connection event related classes
*/

@NonNullApi
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*/

/**
* This package contains cluster and connection event related classes
*/

@NonNullApi
Expand Down
Loading