-
Notifications
You must be signed in to change notification settings - Fork 98
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
base: master
Are you sure you want to change the base?
Conversation
Co-authored-by: Kevin Albertson <[email protected]>
@@ -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); |
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.
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.
Add in-place retry API to better support drivers that fan out KMS requests.