Centralize appending of maxTimeMS

[vbabanin]

Description:

This PR tackles the challenge identified in JAVA-4055 related to the handling of maxTimeMS in the context of Client-Side Field Level Encryption (CSFLE) by moving appending of maxTimeMS to a centralized and lower level within the driver, specifically to the CommandMessage.

Implementation details

Operations handle maxTimeMS differently:

• A majority append maxTimeMS when it is non-zero.
• Some read operations check if cursor is tailable or non-tailable.
• Tailable await cursors append maxAwaitTimeMS as maxTimeMS to a command.

Therefore, this PR introduces MaxTimeMsSupplier in TimeoutContext. This ensures CommandMessage remains agnostic to the operation's logic, allowing operations to dictate it.

JAVA-5322
JAVA-5327

[rozza]

Nit: Long maxTimeMS

[vbabanin]

Done.

[vbabanin]

else if (timeoutMS == 0) is removed due to an issue where using an original infinite timeout (timeoutMS = 0) incorrectly led to always returning 0 from TimeoutSettings in timeoutOrAlternative, regardless of actual computedServerSelectionTimeout time left.

timeoutRemainingMS() handles both infinite and finite timeouts, checking against the timeout object itself:

For the reference:

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

[rozza]

Note: I added timeoutRemainingMS for review in JAVA-5351 as it can incorrectly return 0. (Nothing to fix in this ticket).

[vbabanin]

From the spec:

When sending a command to mongocryptd, drivers MUST NOT append a maxTimeMS field. This is to ensure that a maxTimeMS field can be safely appended to the command after it has been marked by mongocryptd and encrypted by libmongocrypt. To determine whether or not the server is a mongocryptd, drivers MUST check that the iscryptd field in the server's description is true.

Rationale: https://github.com/mongodb/specifications/blob/master/source/client-side-operations-timeout/client-side-operations-timeout.md#maxtimems-is-not-added-for-mongocryptd

[rozza]

Its looking good and is much cleaner than I initially thought it would be.

Found a couple of nits but I also recommend a couple of minor changes to the API:

A):

Have TimeoutContext.getMaxTimeMS() be the thing that consumes the supplier eg:

    public Long getMaxTimeMS() {
       return notNull("Should never be null", maxTimeoutSupplier.get());
    }

Convert the current getMaxTimeMS to be protected and used internally eg: rename it to defaultMaxTimeMS.

Add a timeoutContext.resetSupplier() that resets the supplier to the default. Rather than doing: timeoutContext.setMaxTimeSupplier(timeoutContext::getMaxTimeMS);

B):

Remove TimeoutContext#addTimeoutElements put the logic in command message.

C):

I think CommandOperationHelper#appendMaxTimeMs can now be removed.

What do you think?

[rozza]

Note: I added timeoutRemainingMS for review in JAVA-5351 as it can incorrectly return 0. (Nothing to fix in this ticket).

[vbabanin]

Its looking good and is much cleaner than I initially thought it would be.

Found a couple of nits but I also recommend a couple of minor changes to the API:

A):

Have TimeoutContext.getMaxTimeMS() be the thing that consumes the supplier eg:

    public Long getMaxTimeMS() {
       return notNull("Should never be null", maxTimeoutSupplier.get());
    }

Convert the current getMaxTimeMS to be protected and used internally eg: rename it to defaultMaxTimeMS.

Add a timeoutContext.resetSupplier() that resets the supplier to the default. Rather than doing: timeoutContext.setMaxTimeSupplier(timeoutContext::getMaxTimeMS);

B):

Remove TimeoutContext#addTimeoutElements put the logic in command message.

C):

I think CommandOperationHelper#appendMaxTimeMs can now be removed.

What do you think?

Thank you for the suggestions @rozza!

A) Agreed. This change will maintain consistency in the TimeoutContext API with other methods. Implemented.

B) Implemented as suggested.

C) Agreed and removed.

[rozza]

@vbabanin I think one of the mocks needs updating as its causing all the tests to fail.