Skip to content

MONGOCRYPT-763 add in-place retry API #1011

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

mdb-ad
Copy link
Contributor

@mdb-ad mdb-ad commented May 17, 2025

Add in-place retry API to better support drivers that fan out KMS requests.

@mdb-ad mdb-ad requested a review from a team as a code owner May 17, 2025 23:39
@mdb-ad mdb-ad requested review from a user and kevinAlbs and removed request for a team May 17, 2025 23:39
@mdb-ad mdb-ad requested a review from kevinAlbs June 18, 2025 01:43
@mdb-ad mdb-ad requested a review from a team as a code owner June 18, 2025 03:13
@blink1073 blink1073 removed the request for review from a team June 18, 2025 12:36
@mdb-ad mdb-ad requested a review from kevinAlbs July 3, 2025 18:53
@@ -1180,14 +1180,24 @@ MONGOCRYPT_EXPORT
bool mongocrypt_kms_ctx_feed(mongocrypt_kms_ctx_t *kms, mongocrypt_binary_t *bytes);

/**
* Indicate a network-level failure.
* Indicate a failure. Discards all data fed to this KMS context with @ref mongocrypt_kms_ctx_feed.
* The @ref mongocrypt_kms_ctx_t may be reused.
*
* @param[in] kms The @ref mongocrypt_kms_ctx_t.
* @return A boolean indicating whether the failed request may be retried.
*/
MONGOCRYPT_EXPORT
bool mongocrypt_kms_ctx_fail(mongocrypt_kms_ctx_t *kms);
Copy link
Contributor

Choose a reason for hiding this comment

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

mongocrypt_kms_ctx_fail currently still refers to "network error" in error messages and increments the retry attempts:

ASSERT_OK(mongocrypt_kms_ctx_feed(kms, retryable_http), kms); // Increments kms->attempts.
ASSERT(mongocrypt_kms_ctx_should_retry(kms));
ASSERT_OK(mongocrypt_kms_ctx_fail(kms), kms); // Increments kms->attempts again.

I like the direction towards simplifying. Suggest resetting the KMS parser in set_retry. I expect this will reset the parser for both HTTP and network error and could further simplify the retry pattern:

while (true) {
    try {
        // TODO: Write and read from socket.
    } catch (e : NetworkError) {
        // Indicate network error:
        mongocrypt_kms_ctx_fail (kms);
    } finally {
        if (mongocrypt_kms_ctx_should_retry (kms)) {
            // Can retry due to a HTTP or network error.
            continue;
        }
    }
}

That way, mongocrypt_kms_ctx_fail does not change its meaning (still means only network error).

Testing this change in 456d523 appears to work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants