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. * diff --git a/sdk/core/azure-core/src/http/retry_policy.cpp b/sdk/core/azure-core/src/http/retry_policy.cpp index 29eeae8e4a..32067e5726 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( @@ -145,19 +145,27 @@ 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. - if (!ShouldRetryOnResponse(*response.get(), m_retryOptions, attempt, retryAfter)) + // 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)) { - // 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)) + // 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)) { + // 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; } } @@ -227,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..5c5b069ac0 100644 --- a/sdk/core/azure-core/test/ut/retry_policy_test.cpp +++ b/sdk/core/azure-core/test/ut/retry_policy_test.cpp @@ -125,21 +125,91 @@ 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: - 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; + } +}; + +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&, RetryOptions const&) const override + { + 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&) const override + { + 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 +220,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 +234,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 +248,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 +301,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)