-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Replace Timeout methods with branching "run" methods #1349
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
Changes from 9 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
76e07d0
Replace Timeout methods with branching "run" methods
katcharov 64ae4d3
Fix tests (remove mocks)
katcharov 4050052
temp
katcharov 3017f07
PR fixes, refactoring, fix typos
katcharov 9f7a9af
Use nullAsInfinite
katcharov 36cc604
PR fixes
katcharov 22175a1
Fix compile, checkstyle
katcharov c84adc4
Refactor timeout calculations
katcharov 07a1454
Fix checkstyle
katcharov 381ca6c
Merge remote-tracking branch 'rozza/CSOT' into JAVA-5376
katcharov d480fa0
Post-merge resolutions of maxTimeMS; shortenBy only timeoutMS
katcharov 42a55ab
Fix refactoring bug
katcharov d91e9d6
Accommodate timeouts that begin when used
katcharov 30f3e8e
PR fixes
katcharov 3feb9ed
PR fixes, rename
katcharov fd8fb83
Remove Timeout methods, remaining TODOs, PR fixes
katcharov acfe871
Cleanup, fixes
katcharov 1f89e94
Fix copy-paste error
katcharov 5ef637f
Encapsulate constructors
katcharov b26c9ed
PR fixes
katcharov 46fd71c
Checkstyle
katcharov 42432ae
Merge remote-tracking branch 'rozza/CSOT' into JAVA-5376
katcharov e842e4d
Post-merge fixes
katcharov bcb83a4
PR Fixes
katcharov 1a944f4
Checkstyle
katcharov File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,14 +21,16 @@ | |
import com.mongodb.internal.time.Timeout; | ||
import com.mongodb.lang.Nullable; | ||
import com.mongodb.session.ClientSession; | ||
import org.bson.BsonDocument; | ||
import org.bson.BsonInt64; | ||
|
||
import java.util.Objects; | ||
import java.util.Optional; | ||
|
||
import static com.mongodb.assertions.Assertions.assertNotNull; | ||
import static com.mongodb.assertions.Assertions.assertNull; | ||
import static com.mongodb.assertions.Assertions.isTrue; | ||
import static java.util.concurrent.TimeUnit.MILLISECONDS; | ||
import static java.util.concurrent.TimeUnit.NANOSECONDS; | ||
|
||
/** | ||
* Timeout Context. | ||
|
@@ -46,14 +48,18 @@ public class TimeoutContext { | |
private Timeout computedServerSelectionTimeout; | ||
private long minRoundTripTimeMS = 0; | ||
|
||
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); | ||
} | ||
|
@@ -125,8 +131,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 remove this method (do not inline, but use lambdas) | ||
return Timeout.nullAsInfinite(timeout).run(NANOSECONDS, () -> false, (ns) -> false, () -> true); | ||
rozza marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
|
@@ -140,19 +146,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); | ||
rozza marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
/** | ||
|
@@ -162,14 +158,13 @@ private boolean hasTimedOutForCommandExecution() { | |
* @return timeout to use. | ||
*/ | ||
public long timeoutOrAlternative(final long alternativeTimeoutMS) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This timeoutOrAlternative method should ideally be inlined, with |
||
Long timeoutMS = timeoutSettings.getTimeoutMS(); | ||
if (timeoutMS == null) { | ||
return alternativeTimeoutMS; | ||
} else if (timeoutMS == 0) { | ||
return timeoutMS; | ||
} else { | ||
return timeoutRemainingMS(); | ||
} | ||
Timeout t = timeout != null ? timeout : Timeout.expiresIn(alternativeTimeoutMS, MILLISECONDS); | ||
return t.run(MILLISECONDS, | ||
() -> 0L, | ||
(ms) -> ms, | ||
() -> { | ||
throw createMongoTimeoutException("The operation timeout has expired."); | ||
}); | ||
} | ||
|
||
/** | ||
|
@@ -178,17 +173,17 @@ public long timeoutOrAlternative(final long alternativeTimeoutMS) { | |
* @param alternativeTimeoutMS the alternative timeout | ||
* @return the minimum value to use. | ||
*/ | ||
// TODO (CSOT) JAVA-5385 used only by tests? | ||
public long calculateMin(final long alternativeTimeoutMS) { | ||
Long timeoutMS = timeoutSettings.getTimeoutMS(); | ||
if (timeoutMS == null) { | ||
return alternativeTimeoutMS; | ||
} else if (timeoutMS == 0) { | ||
return alternativeTimeoutMS; | ||
} else if (alternativeTimeoutMS == 0) { | ||
return timeoutRemainingMS(); | ||
} else { | ||
return Math.min(timeoutRemainingMS(), alternativeTimeoutMS); | ||
} | ||
Timeout minimum = Timeout.expiresIn(alternativeTimeoutMS, MILLISECONDS) | ||
.orEarlier(Timeout.nullAsInfinite(timeout)); | ||
|
||
return minimum.run(MILLISECONDS, | ||
() -> 0L, | ||
(ms) -> ms, | ||
() -> { | ||
throw createMongoTimeoutException("The operation timeout has expired."); | ||
}); | ||
} | ||
|
||
public TimeoutSettings getTimeoutSettings() { | ||
|
@@ -199,15 +194,31 @@ public long getMaxAwaitTimeMS() { | |
return timeoutSettings.getMaxAwaitTimeMS(); | ||
} | ||
|
||
// TODO (CSOT) JAVA-5385 used only by tests? | ||
public long getMaxTimeMS() { | ||
rozza marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
long maxTimeMS = timeoutOrAlternative(timeoutSettings.getMaxTimeMS()); | ||
if (timeout == null || timeout.isInfinite()) { | ||
return maxTimeMS; | ||
} | ||
if (minRoundTripTimeMS >= maxTimeMS) { | ||
throw createMongoTimeoutException(); | ||
} | ||
return maxTimeMS - minRoundTripTimeMS; | ||
return getMaxTimeTimeout().run(MILLISECONDS, | ||
() -> 0L, | ||
(ms) -> ms, | ||
() -> { | ||
throw createMongoRoundTripTimeoutException(); | ||
}); | ||
} | ||
|
||
private Timeout getMaxTimeTimeout() { | ||
Timeout t = timeout != null | ||
? timeout | ||
: Timeout.expiresIn(timeoutSettings.getMaxTimeMS(), MILLISECONDS); | ||
t = t.shortenBy(minRoundTripTimeMS, MILLISECONDS); | ||
return t; | ||
} | ||
|
||
public void putMaxTimeMS(final BsonDocument command) { | ||
getMaxTimeTimeout().run(MILLISECONDS, | ||
() -> {}, | ||
(ms) -> command.put("maxTimeMS", new BsonInt64(ms)), | ||
() -> { | ||
throw createMongoRoundTripTimeoutException(); | ||
}); | ||
} | ||
|
||
public Long getMaxCommitTimeMS() { | ||
|
@@ -223,8 +234,10 @@ 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.expiresIn(getTimeoutSettings().getConnectTimeoutMS(), MILLISECONDS) | ||
.orEarlier(Timeout.nullAsInfinite(timeout)); | ||
} | ||
|
||
public void resetTimeout() { | ||
|
@@ -236,9 +249,13 @@ public void resetTimeout() { | |
* 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).run(NANOSECONDS, | ||
() -> timeout, | ||
(ms) -> calculateTimeout(timeoutSettings.getTimeoutMS()), | ||
() -> calculateTimeout(timeoutSettings.getTimeoutMS())); | ||
} | ||
|
||
public TimeoutContext withAdditionalReadTimeout(final int additionalReadTimeout) { | ||
|
@@ -254,14 +271,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{" | ||
|
@@ -294,6 +303,7 @@ public int hashCode() { | |
|
||
@Nullable | ||
public static Timeout calculateTimeout(@Nullable final Long timeoutMS) { | ||
// TODO-CSOT rename to startTimeout? | ||
rozza marked this conversation as resolved.
Outdated
Show resolved
Hide resolved
|
||
if (timeoutMS != null) { | ||
return timeoutMS == 0 ? Timeout.infinite() : Timeout.expiresIn(timeoutMS, MILLISECONDS); | ||
} | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,6 @@ | |
*/ | ||
|
||
/** | ||
* This package contains cluster and connection event related classes | ||
*/ | ||
|
||
@NonNullApi | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,7 +15,6 @@ | |
*/ | ||
|
||
/** | ||
* This package contains cluster and connection event related classes | ||
*/ | ||
|
||
@NonNullApi | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good to know