CSOT

[katcharov]

JAVA-3828

[katcharov]

Note to address all TODOs prior to merging. (All files in github can be loaded using document.querySelectorAll('.load-diff-button').forEach(node => node.click()) in console, and then ctrl-f for TODO)

[stIncMale]

I don't think this is a proper use of the @Tag functionality. We should either use something like com.mongodb.client.FailPoint, or implement automatic turning off of failpoints without using the @Tag functionality.

The original discussion is #1333 (comment).

[katcharov]

Here, we check hasExpired, but below in translateReadException, we check hasTimeoutMS, though it seems both methods should have the same logic.

[katcharov]

I found it more difficult to follow / work with code where an exception was created in one method, but thrown in another. The non-throwing usage of this method was:

        return operationContext.getTimeoutContext().validateHasTimedOutForCommandExecution().map(e -> {
            close();
            commandEventSender.sendFailedEvent(e);
            return e;
        });

I cannot immediately understand if close can be called multiple times for the same exception. This differs from the usual pattern of handling close in a catch.

I understand that this might save having to write a try-catch in async code, but I don't think the cost to readability is worth this benefit.

[katcharov]

As discussed here, consider moving all messages to a central set of fields. At least, the various messages should be checked for consistency, for example, there are at least the following ways that timeouts are described:

• exceeded the timeout limit
• Operation timed out:
• Remaining timeoutMS is less than the server's minimum round trip time.
• The operation timeout has expired
• Timeout occurred during socket write

It seems they should all use the same terminology.

[katcharov]

In these databaseWithTimeout methods, I would expect a null timeout to result in a database without a timeout, but below it returns the database itself. I think we should avoid making these with methods nullable.

[katcharov]

I found uses of Optional difficult to reason about when attempting to inline/invert the write timeout. I think the above section can be re-written as something like:

if (writeTimeoutMS == 0) {
   channel...addBefore...
}

[katcharov]

This method is used only to set the maxTimeMS value in CommandMessage#getExtraElements, when non-zero. The value is:

1. By default, calculateMaxTimeMS: if the timeout not present, use the legacy maxTimeMS setting. Otherwise, shorten the timeout by the round trip time, throwing a timeout exception as needed.
2. In CommitTransactionOperation, the supplier is overridden to use getMaxCommitTimeMS, which uses the timeout if present, but otherwise uses the legacy maxCommitTimeMS setting.
3. Elsewhere, the supplier is overridden to return a fixed value of 0, denoting infinite, or to a fixed value like MaxAwaitTimeMS.

In the second path, when the timeout is present, it is not shortened by the round trip time. In the first, it is shortened, which seems correct. Should it be shortened in the second path?

In the third path, the value (0, or maxAwaitTime) is not overriden by timeout. In the first and second paths, fixed values are overridden by the timeout. Should they be overridden on this third path?

[katcharov]

This change makes lastAttempt non-deterministic within a particular attempt. The following code calls lastAttempt:

if (currentBulkWriteTracker.lastAttempt()) {
    addRetryableWriteErrorLabel(writeConcernBasedError, maxWireVersion);
    addErrorLabelsToWriteConcern(result.getDocument("writeConcernError"), writeConcernBasedError.getErrorLabels());
} 

The intent is to add error labels on the last attempt. However, now, the above lastAttempt check can return "false", because the timeout has not yet timed out. So labels are not attached. At the end of the iteration, other code will check lastAttempt again, but now the timer will have run out, so no further iterations are performed, and labels are never attached. This appears to be a bug.

[katcharov]

See #1349 (comment) - clarify regarding use of times below 1ms.

[katcharov]

Consider changing to resetTimeoutIfPresent, with an if check on null rather than the assert. All prod usages perform this check before calling.