From e3e178ce56b98ecdb5e1ffb174971a72ff8661f5 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Wed, 22 May 2024 12:03:19 -0700 Subject: [PATCH 1/4] Update the RetryPolicy and ShouldRetry customization logic to allow loosen the retry condition. --- sdk/core/azure-core/src/http/retry_policy.cpp | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/sdk/core/azure-core/src/http/retry_policy.cpp b/sdk/core/azure-core/src/http/retry_policy.cpp index 29eeae8e4a..cc8048c91e 100644 --- a/sdk/core/azure-core/src/http/retry_policy.cpp +++ b/sdk/core/azure-core/src/http/retry_policy.cpp @@ -120,7 +120,7 @@ int32_t RetryPolicy::GetRetryCount(Context const& context) bool RetryPolicy::ShouldRetry(std::unique_ptr const&, RetryOptions const&) const { - return true; + return false; } std::unique_ptr RetryPolicy::Send( @@ -147,17 +147,14 @@ std::unique_ptr RetryPolicy::Send( // If we are out of retry attempts, if a response is non-retriable (or simply 200 OK, i.e // doesn't need to be retried), then ShouldRetryOnResponse returns false. - if (!ShouldRetryOnResponse(*response.get(), m_retryOptions, attempt, retryAfter)) - { - // If this is the second attempt and StartTry was called, we need to stop it. Otherwise - // trying to perform same request would use last retry query/headers - return response; - } // Service SDKs can inject custom logic to define whether the request should be retried, - // based on the response. The default is true. - if (!ShouldRetry(response, m_retryOptions)) + // based on the response. The default of `ShouldRetry` is false. + if (!ShouldRetryOnResponse(*response.get(), m_retryOptions, attempt, retryAfter) + && !ShouldRetry(response, m_retryOptions)) { + // If this is the second attempt and StartTry was called, we need to stop it. Otherwise + // trying to perform same request would use last retry query/headers return response; } } From e31502ec5c11468494be10a89c79de99a2830617 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Wed, 22 May 2024 12:15:48 -0700 Subject: [PATCH 2/4] Add CL entry and update doc comment. --- sdk/core/azure-core/CHANGELOG.md | 1 + sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index cbdd45fad8..f8da956861 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -13,6 +13,7 @@ ### Other Changes - [[#5622]](https://github.com/Azure/azure-sdk-for-cpp/pull/5622) Documentation fix for building the SDK with specific OpenSSL version. (A community contribution, courtesy of _[ByteYue](https://github.com/ByteYue)_) +- [[#5515]](https://github.com/Azure/azure-sdk-for-cpp/issues/5515) Add a `ShouldRetry` virtual method to the retry policy to enable customization of service-specific retry logic. ### Acknowledgments diff --git a/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp b/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp index e626dbc48b..7eae35118a 100644 --- a/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp @@ -420,7 +420,7 @@ namespace Azure { namespace Core { namespace Http { namespace Policies { * request. Custom implementations of this method that override the retry behavior, should * handle that error case, if that needs to be customized. * - * @remark Unless overriden, the default implementation is to always return true. The + * @remark Unless overriden, the default implementation is to always return `false`. The * non-retriable errors, including those specified in the RetryOptions, remain evaluated * before calling ShouldRetry. * From 9cc6a2a6259d9582938234483497a4010644c5b5 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Wed, 29 May 2024 16:09:31 -0700 Subject: [PATCH 3/4] Move WasLastAttempt check out, and add more tests. --- sdk/core/azure-core/src/http/retry_policy.cpp | 25 ++- .../azure-core/test/ut/retry_policy_test.cpp | 181 ++++++++++++++++-- 2 files changed, 185 insertions(+), 21 deletions(-) diff --git a/sdk/core/azure-core/src/http/retry_policy.cpp b/sdk/core/azure-core/src/http/retry_policy.cpp index cc8048c91e..32067e5726 100644 --- a/sdk/core/azure-core/src/http/retry_policy.cpp +++ b/sdk/core/azure-core/src/http/retry_policy.cpp @@ -145,11 +145,22 @@ std::unique_ptr RetryPolicy::Send( { auto response = nextPolicy.Send(request, retryContext); - // If we are out of retry attempts, if a response is non-retriable (or simply 200 OK, i.e - // doesn't need to be retried), then ShouldRetryOnResponse returns false. + // Are we out of retry attempts? + // Checking this first, before checking the response so that the extension point of + // ShouldRetry doesn't have the responsibility of checking the number of retries (again). + if (WasLastAttempt(m_retryOptions, attempt)) + { + return response; + } - // Service SDKs can inject custom logic to define whether the request should be retried, - // based on the response. The default of `ShouldRetry` is false. + // If a response is non-retriable (or simply 200 OK, i.e doesn't need to be retried), then + // ShouldRetryOnResponse returns false. Service SDKs can inject custom logic to define whether + // the request should be retried, based on the response. The default of `ShouldRetry` is + // false. + // Because of boolean short-circuit evaluation, if ShouldRetryOnResponse returns true, the + // overriden ShouldRetry is not called. This is expected, since overriding ShouldRetry enables + // loosening the retry conditions (retrying where otherwise the request wouldn't be), but not + // strengthening it. if (!ShouldRetryOnResponse(*response.get(), m_retryOptions, attempt, retryAfter) && !ShouldRetry(response, m_retryOptions)) { @@ -224,12 +235,6 @@ bool RetryPolicy::ShouldRetryOnResponse( using Azure::Core::Diagnostics::Logger; using Azure::Core::Diagnostics::_internal::Log; - // Are we out of retry attempts? - if (WasLastAttempt(retryOptions, attempt)) - { - return false; - } - // Should we retry on the given response retry code? { auto const& statusCodes = retryOptions.StatusCodes; diff --git a/sdk/core/azure-core/test/ut/retry_policy_test.cpp b/sdk/core/azure-core/test/ut/retry_policy_test.cpp index 2679d78e0a..df72b2eec6 100644 --- a/sdk/core/azure-core/test/ut/retry_policy_test.cpp +++ b/sdk/core/azure-core/test/ut/retry_policy_test.cpp @@ -125,13 +125,13 @@ class RetryPolicyTest final : public RetryPolicy { } }; -class RetryPolicyTest_CustomRetry_False final : public RetryPolicy { +class RetryPolicyTest_CustomRetry_True final : public RetryPolicy { public: - RetryPolicyTest_CustomRetry_False(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {} + RetryPolicyTest_CustomRetry_True(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {} std::unique_ptr Clone() const override { - return std::make_unique(*this); + return std::make_unique(*this); } protected: @@ -140,6 +140,85 @@ class RetryPolicyTest_CustomRetry_False final : public RetryPolicy { { (void)response; (void)retryOptions; + return true; + } +}; + +class RetryPolicyTest_CustomRetry_Throw final : public RetryPolicy { +public: + RetryPolicyTest_CustomRetry_Throw(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {} + + std::unique_ptr Clone() const override + { + return std::make_unique(*this); + } + +protected: + bool ShouldRetry(std::unique_ptr const& response, RetryOptions const& retryOptions) + const override + { + (void)response; + (void)retryOptions; + throw TransportException("Short-circuit evaluation means this should never be called."); + } +}; + +class RetryPolicyTest_CustomRetry_CopySource final : public RetryPolicy { +public: + RetryPolicyTest_CustomRetry_CopySource(RetryOptions const& retryOptions) + : RetryPolicy(retryOptions) + { + } + + std::unique_ptr Clone() const override + { + return std::make_unique(*this); + } + +protected: + bool ShouldRetry(std::unique_ptr const& response, RetryOptions const& retryOptions) + const override + { + (void)retryOptions; + + if (response == nullptr) + { + return true; + } + + if (static_cast>( + response->GetStatusCode()) + >= 400) + { + const auto& headers = response->GetHeaders(); + auto ite = headers.find("x-ms-error-code"); + if (ite != headers.end()) + { + const std::string error = ite->second; + + if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy") + { + return true; + } + } + } + + if (static_cast>( + response->GetStatusCode()) + >= 400) + { + const auto& headers = response->GetHeaders(); + auto ite = headers.find("x-ms-copy-source-error-code"); + if (ite != headers.end()) + { + const std::string error = ite->second; + + if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy") + { + return true; + } + } + } return false; } }; @@ -150,10 +229,11 @@ TEST(RetryPolicy, ShouldRetry) using namespace std::chrono_literals; RetryOptions const retryOptions{3, 10ms, 60s, {HttpStatusCode::Ok}}; - // The default ShouldRetry implementation is to always return true, - // which means we will retry up to the retry count in the options. + // The default ShouldRetry implementation is to always return false, + // which means we will retry up to the retry count in the options, unless the status code isn't + // one that is retriable. { - Azure::Core::Context context = Azure::Core::Context::ApplicationContext; + Azure::Core::Context context = Azure::Core::Context(); auto policy = RetryPolicy(retryOptions); RawResponse const* responsePtrSent = nullptr; @@ -163,7 +243,6 @@ TEST(RetryPolicy, ShouldRetry) policies.emplace_back(std::make_unique(policy)); policies.emplace_back(std::make_unique([&]() { auto response = std::make_unique(1, 1, HttpStatusCode::Ok, "Test"); - responsePtrSent = response.get(); retryCounter++; return response; @@ -178,20 +257,46 @@ TEST(RetryPolicy, ShouldRetry) EXPECT_EQ(retryCounter, 3); } - // Overriding ShouldRetry to return false will mean we won't retry. + // If the status code is retriable based on the options, ShouldRetry doesn't get called. + // Testing short-circuit evaluation { Azure::Core::Context context = Azure::Core::Context(); - auto policy = RetryPolicyTest_CustomRetry_False(retryOptions); + auto policy = RetryPolicyTest_CustomRetry_Throw(retryOptions); RawResponse const* responsePtrSent = nullptr; int32_t retryCounter = -1; std::vector> policies; - policies.emplace_back(std::make_unique(policy)); - + policies.emplace_back(std::make_unique(policy)); policies.emplace_back(std::make_unique([&]() { auto response = std::make_unique(1, 1, HttpStatusCode::Ok, "Test"); + responsePtrSent = response.get(); + retryCounter++; + return response; + })); + + Azure::Core::Http::_internal::HttpPipeline pipeline(policies); + + Request request(HttpMethod::Get, Azure::Core::Url("https://www.microsoft.com")); + pipeline.Send(request, context); + EXPECT_NE(responsePtrSent, nullptr); + EXPECT_EQ(retryCounter, 3); + } + + // If the status code isn't retriable based on the options, only then does ShouldRetry get called. + // Since the default of ShouldRetry is false, we don't expect any retries. + { + Azure::Core::Context context = Azure::Core::Context(); + auto policy = RetryPolicy(retryOptions); + + RawResponse const* responsePtrSent = nullptr; + int32_t retryCounter = -1; + + std::vector> policies; + policies.emplace_back(std::make_unique(policy)); + policies.emplace_back(std::make_unique([&]() { + auto response = std::make_unique(1, 1, HttpStatusCode::Accepted, "Test"); responsePtrSent = response.get(); retryCounter++; return response; @@ -205,6 +310,60 @@ TEST(RetryPolicy, ShouldRetry) EXPECT_NE(responsePtrSent, nullptr); EXPECT_EQ(retryCounter, 0); } + + // Overriding ShouldRetry to return true will mean we will retry, when the status code isn't + // retriable based on the options. + { + Azure::Core::Context context = Azure::Core::Context(); + auto policy = RetryPolicyTest_CustomRetry_True(retryOptions); + + RawResponse const* responsePtrSent = nullptr; + int32_t retryCounter = -1; + + std::vector> policies; + policies.emplace_back(std::make_unique(policy)); + policies.emplace_back(std::make_unique([&]() { + auto response = std::make_unique(1, 1, HttpStatusCode::Accepted, "Test"); + responsePtrSent = response.get(); + retryCounter++; + return response; + })); + + Azure::Core::Http::_internal::HttpPipeline pipeline(policies); + + Request request(HttpMethod::Get, Azure::Core::Url("https://www.microsoft.com")); + pipeline.Send(request, context); + + EXPECT_NE(responsePtrSent, nullptr); + EXPECT_EQ(retryCounter, 3); + } + + // Copy Status Code scenario (non-retriable HTTP status code) + { + Azure::Core::Context context = Azure::Core::Context(); + auto policy = RetryPolicyTest_CustomRetry_CopySource(RetryOptions()); + + RawResponse const* responsePtrSent = nullptr; + int32_t retryCounter = -1; + + std::vector> policies; + policies.emplace_back(std::make_unique(policy)); + policies.emplace_back(std::make_unique([&]() { + auto response = std::make_unique(1, 1, HttpStatusCode::Conflict, "Test"); + response->SetHeader("x-ms-copy-source-error-code", "OperationTimedOut"); + responsePtrSent = response.get(); + retryCounter++; + return response; + })); + + Azure::Core::Http::_internal::HttpPipeline pipeline(policies); + + Request request(HttpMethod::Get, Azure::Core::Url("https://www.microsoft.com")); + pipeline.Send(request, context); + + EXPECT_NE(responsePtrSent, nullptr); + EXPECT_EQ(retryCounter, 3); + } } TEST(RetryPolicy, ShouldRetryOnResponse) From 68769e718286270dbbc73b866e9e6d176dc0b0a4 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Wed, 29 May 2024 17:10:04 -0700 Subject: [PATCH 4/4] Address PR feedback, remove use of void and unused params. --- sdk/core/azure-core/test/ut/retry_policy_test.cpp | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/sdk/core/azure-core/test/ut/retry_policy_test.cpp b/sdk/core/azure-core/test/ut/retry_policy_test.cpp index df72b2eec6..5c5b069ac0 100644 --- a/sdk/core/azure-core/test/ut/retry_policy_test.cpp +++ b/sdk/core/azure-core/test/ut/retry_policy_test.cpp @@ -135,11 +135,8 @@ class RetryPolicyTest_CustomRetry_True final : public RetryPolicy { } protected: - bool ShouldRetry(std::unique_ptr const& response, RetryOptions const& retryOptions) - const override + bool ShouldRetry(std::unique_ptr const&, RetryOptions const&) const override { - (void)response; - (void)retryOptions; return true; } }; @@ -154,11 +151,8 @@ class RetryPolicyTest_CustomRetry_Throw final : public RetryPolicy { } protected: - bool ShouldRetry(std::unique_ptr const& response, RetryOptions const& retryOptions) - const override + bool ShouldRetry(std::unique_ptr const&, RetryOptions const&) const override { - (void)response; - (void)retryOptions; throw TransportException("Short-circuit evaluation means this should never be called."); } }; @@ -176,11 +170,8 @@ class RetryPolicyTest_CustomRetry_CopySource final : public RetryPolicy { } protected: - bool ShouldRetry(std::unique_ptr const& response, RetryOptions const& retryOptions) - const override + bool ShouldRetry(std::unique_ptr const& response, RetryOptions const&) const override { - (void)retryOptions; - if (response == nullptr) { return true;