From 7e04728bcc5e32047dc8678a7746bba960bff02e Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk Date: Fri, 12 Jul 2024 11:10:19 -0700 Subject: [PATCH 1/4] Revert "Update the RetryPolicy for the GA release, keeping ShouldRetry extension point hidden. (#5771)" This reverts commit 9ccd206ff88f1550ecf94139806085b69a341cd2. --- sdk/core/azure-core/CHANGELOG.md | 1 - .../inc/azure/core/http/policies/policy.hpp | 21 +- .../azure-core/test/ut/retry_policy_test.cpp | 407 +++++++++--------- 3 files changed, 202 insertions(+), 227 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index d7b182f073..9f0c487341 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -9,7 +9,6 @@ ### Other Changes - Updated JSON library to 3.11.3. -- Hide methods on the `RetryPolicy` that are not intended for public use. - [[#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)_) ### 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 302c34b880..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 @@ -30,14 +30,6 @@ #include #include -#if defined(_azure_TESTING_BUILD) -// Define the classes used from tests -namespace Azure { namespace Core { namespace Test { - class RetryPolicyTest; - class RetryLogic; -}}} // namespace Azure::Core::Test -#endif - /** * A function that should be implemented and linked to the end-user application in order to override * an HTTP transport implementation provided by Azure SDK with custom implementation. @@ -371,16 +363,7 @@ namespace Azure { namespace Core { namespace Http { namespace Policies { /** * @brief HTTP retry policy. */ - class RetryPolicy -#if !defined(_azure_TESTING_BUILD) - final -#endif - : public HttpPolicy { -#if defined(_azure_TESTING_BUILD) - // make tests classes friends - friend class Azure::Core::Test::RetryPolicyTest; - friend class Azure::Core::Test::RetryLogic; -#endif + class RetryPolicy : public HttpPolicy { private: RetryOptions m_retryOptions; @@ -415,7 +398,7 @@ namespace Azure { namespace Core { namespace Http { namespace Policies { */ static int32_t GetRetryCount(Context const& context); - private: + protected: virtual bool ShouldRetryOnTransportFailure( RetryOptions const& retryOptions, int32_t attempt, 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 cbfe5cd99f..5c5b069ac0 100644 --- a/sdk/core/azure-core/test/ut/retry_policy_test.cpp +++ b/sdk/core/azure-core/test/ut/retry_policy_test.cpp @@ -13,214 +13,207 @@ using namespace Azure::Core::Http; using namespace Azure::Core::Http::Policies; using namespace Azure::Core::Http::Policies::_internal; -namespace Azure { namespace Core { namespace Test { - class TestTransportPolicy final : public HttpPolicy { - private: - std::function()> m_send; - - public: - TestTransportPolicy(std::function()> send) : m_send(send) {} - - std::unique_ptr Send( - Request&, - NextHttpPolicy, - Azure::Core::Context const&) const override - { - return m_send(); - } +namespace { +class TestTransportPolicy final : public HttpPolicy { +private: + std::function()> m_send; + +public: + TestTransportPolicy(std::function()> send) : m_send(send) {} + + std::unique_ptr Send( + Request&, + NextHttpPolicy, + Azure::Core::Context const&) const override + { + return m_send(); + } - std::unique_ptr Clone() const override - { - return std::make_unique(*this); - } - }; - - class RetryPolicyTest final : public RetryPolicy { - private: - std::function - m_shouldRetryOnTransportFailure; - - std::function< - bool(RawResponse const&, RetryOptions const&, int32_t, std::chrono::milliseconds&, double)> - m_shouldRetryOnResponse; - - public: - bool BaseShouldRetryOnTransportFailure( - RetryOptions const& retryOptions, - int32_t attempt, - std::chrono::milliseconds& retryAfter, - double jitterFactor) const - { - return RetryPolicy::ShouldRetryOnTransportFailure( - retryOptions, attempt, retryAfter, jitterFactor); - } + std::unique_ptr Clone() const override + { + return std::make_unique(*this); + } +}; + +class RetryPolicyTest final : public RetryPolicy { +private: + std::function + m_shouldRetryOnTransportFailure; + + std::function< + bool(RawResponse const&, RetryOptions const&, int32_t, std::chrono::milliseconds&, double)> + m_shouldRetryOnResponse; + +public: + bool BaseShouldRetryOnTransportFailure( + RetryOptions const& retryOptions, + int32_t attempt, + std::chrono::milliseconds& retryAfter, + double jitterFactor) const + { + return RetryPolicy::ShouldRetryOnTransportFailure( + retryOptions, attempt, retryAfter, jitterFactor); + } - bool BaseShouldRetryOnResponse( - RawResponse const& response, - RetryOptions const& retryOptions, - int32_t attempt, - std::chrono::milliseconds& retryAfter, - double jitterFactor) const - { - return RetryPolicy::ShouldRetryOnResponse( - response, retryOptions, attempt, retryAfter, jitterFactor); - } + bool BaseShouldRetryOnResponse( + RawResponse const& response, + RetryOptions const& retryOptions, + int32_t attempt, + std::chrono::milliseconds& retryAfter, + double jitterFactor) const + { + return RetryPolicy::ShouldRetryOnResponse( + response, retryOptions, attempt, retryAfter, jitterFactor); + } - RetryPolicyTest( - RetryOptions const& retryOptions, - decltype(m_shouldRetryOnTransportFailure) shouldRetryOnTransportFailure, - decltype(m_shouldRetryOnResponse) shouldRetryOnResponse) - : RetryPolicy(retryOptions), - m_shouldRetryOnTransportFailure( - shouldRetryOnTransportFailure != nullptr // - ? shouldRetryOnTransportFailure - : static_cast( // - [this](auto options, auto attempt, auto retryAfter, auto jitter) { - retryAfter = std::chrono::milliseconds(0); - auto ignore = decltype(retryAfter)(); - return this->BaseShouldRetryOnTransportFailure( - options, attempt, ignore, jitter); - })), - m_shouldRetryOnResponse( - shouldRetryOnResponse != nullptr // - ? shouldRetryOnResponse - : static_cast( // - [this]( - RawResponse const& response, - auto options, - auto attempt, - auto retryAfter, - auto jitter) { - retryAfter = std::chrono::milliseconds(0); - auto ignore = decltype(retryAfter)(); - return this->BaseShouldRetryOnResponse( - response, options, attempt, ignore, jitter); - })) - { - } + RetryPolicyTest( + RetryOptions const& retryOptions, + decltype(m_shouldRetryOnTransportFailure) shouldRetryOnTransportFailure, + decltype(m_shouldRetryOnResponse) shouldRetryOnResponse) + : RetryPolicy(retryOptions), + m_shouldRetryOnTransportFailure( + shouldRetryOnTransportFailure != nullptr // + ? shouldRetryOnTransportFailure + : static_cast( // + [this](auto options, auto attempt, auto retryAfter, auto jitter) { + retryAfter = std::chrono::milliseconds(0); + auto ignore = decltype(retryAfter)(); + return this->BaseShouldRetryOnTransportFailure( + options, attempt, ignore, jitter); + })), + m_shouldRetryOnResponse( + shouldRetryOnResponse != nullptr // + ? shouldRetryOnResponse + : static_cast( // + [this]( + RawResponse const& response, + auto options, + auto attempt, + auto retryAfter, + auto jitter) { + retryAfter = std::chrono::milliseconds(0); + auto ignore = decltype(retryAfter)(); + return this->BaseShouldRetryOnResponse( + response, options, attempt, ignore, jitter); + })) + { + } - std::unique_ptr Clone() const override - { - return std::make_unique(*this); - } + std::unique_ptr Clone() const override + { + return std::make_unique(*this); + } - protected: - bool ShouldRetryOnTransportFailure( - RetryOptions const& retryOptions, - int32_t attempt, - std::chrono::milliseconds& retryAfter, - double jitterFactor) const override - { - return m_shouldRetryOnTransportFailure(retryOptions, attempt, retryAfter, jitterFactor); - } +protected: + bool ShouldRetryOnTransportFailure( + RetryOptions const& retryOptions, + int32_t attempt, + std::chrono::milliseconds& retryAfter, + double jitterFactor) const override + { + return m_shouldRetryOnTransportFailure(retryOptions, attempt, retryAfter, jitterFactor); + } - bool ShouldRetryOnResponse( - RawResponse const& response, - RetryOptions const& retryOptions, - int32_t attempt, - std::chrono::milliseconds& retryAfter, - double jitterFactor) const override - { - return m_shouldRetryOnResponse(response, retryOptions, attempt, retryAfter, jitterFactor); - } - }; + bool ShouldRetryOnResponse( + RawResponse const& response, + RetryOptions const& retryOptions, + int32_t attempt, + std::chrono::milliseconds& retryAfter, + double jitterFactor) const override + { + return m_shouldRetryOnResponse(response, retryOptions, attempt, retryAfter, jitterFactor); + } +}; - class RetryPolicyTest_CustomRetry_True final : public RetryPolicy { - public: - RetryPolicyTest_CustomRetry_True(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) - { - } +class RetryPolicyTest_CustomRetry_True final : public RetryPolicy { +public: + RetryPolicyTest_CustomRetry_True(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {} - std::unique_ptr Clone() const override - { - return std::make_unique(*this); - } + std::unique_ptr Clone() const override + { + return std::make_unique(*this); + } - protected: - bool ShouldRetry(std::unique_ptr const&, RetryOptions const&) const override - { - return true; - } - }; +protected: + bool ShouldRetry(std::unique_ptr const&, RetryOptions const&) const override + { + return true; + } +}; - class RetryPolicyTest_CustomRetry_Throw final : public RetryPolicy { - public: - RetryPolicyTest_CustomRetry_Throw(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) - { - } +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); - } + 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."); - } - }; +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) - { - } +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); + } - std::unique_ptr Clone() const override +protected: + bool ShouldRetry(std::unique_ptr const& response, RetryOptions const&) const override + { + if (response == nullptr) { - return std::make_unique(*this); + return true; } - protected: - bool ShouldRetry(std::unique_ptr const& response, RetryOptions const&) - const override + if (static_cast>( + response->GetStatusCode()) + >= 400) { - if (response == nullptr) + const auto& headers = response->GetHeaders(); + auto ite = headers.find("x-ms-error-code"); + if (ite != headers.end()) { - return true; - } + const std::string error = ite->second; - if (static_cast>( - response->GetStatusCode()) - >= 400) - { - const auto& headers = response->GetHeaders(); - auto ite = headers.find("x-ms-error-code"); - if (ite != headers.end()) + if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy") { - const std::string error = ite->second; - - if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy") - { - return true; - } + return true; } } + } - if (static_cast>( - response->GetStatusCode()) - >= 400) + 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 auto& headers = response->GetHeaders(); - auto ite = headers.find("x-ms-copy-source-error-code"); - if (ite != headers.end()) - { - const std::string error = ite->second; + const std::string error = ite->second; - if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy") - { - return true; - } + if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy") + { + return true; } } - return false; } - }; -}}} // namespace Azure::Core::Test - -using namespace Azure::Core::Test; + return false; + } +}; +} // namespace TEST(RetryPolicy, ShouldRetry) { @@ -596,38 +589,38 @@ TEST(RetryPolicy, ShouldRetryOnTransportFailure) EXPECT_EQ(jitterReceived, -1); } -namespace Azure { namespace Core { namespace Test { - class RetryLogic final : private RetryPolicy { - RetryLogic() : RetryPolicy(RetryOptions()) {} - ~RetryLogic() {} +namespace { +class RetryLogic final : private RetryPolicy { + RetryLogic() : RetryPolicy(RetryOptions()) {} + ~RetryLogic() {} - static RetryLogic const g_retryPolicy; + static RetryLogic const g_retryPolicy; - public: - static bool TestShouldRetryOnTransportFailure( - RetryOptions const& retryOptions, - int32_t attempt, - std::chrono::milliseconds& retryAfter, - double jitterFactor) - { - return g_retryPolicy.ShouldRetryOnTransportFailure( - retryOptions, attempt, retryAfter, jitterFactor); - } +public: + static bool TestShouldRetryOnTransportFailure( + RetryOptions const& retryOptions, + int32_t attempt, + std::chrono::milliseconds& retryAfter, + double jitterFactor) + { + return g_retryPolicy.ShouldRetryOnTransportFailure( + retryOptions, attempt, retryAfter, jitterFactor); + } - static bool TestShouldRetryOnResponse( - RawResponse const& response, - RetryOptions const& retryOptions, - int32_t attempt, - std::chrono::milliseconds& retryAfter, - double jitterFactor) - { - return g_retryPolicy.ShouldRetryOnResponse( - response, retryOptions, attempt, retryAfter, jitterFactor); - } - }; + static bool TestShouldRetryOnResponse( + RawResponse const& response, + RetryOptions const& retryOptions, + int32_t attempt, + std::chrono::milliseconds& retryAfter, + double jitterFactor) + { + return g_retryPolicy.ShouldRetryOnResponse( + response, retryOptions, attempt, retryAfter, jitterFactor); + } +}; - RetryLogic const RetryLogic::g_retryPolicy; -}}} // namespace Azure::Core::Test +RetryLogic const RetryLogic::g_retryPolicy; +} // namespace TEST(RetryPolicy, Exponential) { From 195aeb2b3a0b11dcf209535ce5eed163b2f84f4f Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk Date: Fri, 12 Jul 2024 11:11:55 -0700 Subject: [PATCH 2/4] Revert "Update the RetryPolicy and ShouldRetry customization logic to allow loosening the retry condition. (#5656)" This reverts commit f1d95520d14d828e49f1531e8b425bd3d7107279. --- sdk/core/azure-core/CHANGELOG.md | 1 - .../inc/azure/core/http/policies/policy.hpp | 2 +- sdk/core/azure-core/src/http/retry_policy.cpp | 32 ++-- .../azure-core/test/ut/retry_policy_test.cpp | 180 ++---------------- 4 files changed, 31 insertions(+), 184 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 9f0c487341..42930eda25 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -27,7 +27,6 @@ Thank you to our developer community members who helped to make Azure Core bette ### 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 7eae35118a..e626dbc48b 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 `false`. The + * @remark Unless overriden, the default implementation is to always return true. 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 32067e5726..29eeae8e4a 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 false; + return true; } std::unique_ptr RetryPolicy::Send( @@ -145,27 +145,19 @@ std::unique_ptr RetryPolicy::Send( { auto response = nextPolicy.Send(request, retryContext); - // 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 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; } - // 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)) + // 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 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; } } @@ -235,6 +227,12 @@ 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 5c5b069ac0..2679d78e0a 100644 --- a/sdk/core/azure-core/test/ut/retry_policy_test.cpp +++ b/sdk/core/azure-core/test/ut/retry_policy_test.cpp @@ -125,91 +125,21 @@ class RetryPolicyTest final : public RetryPolicy { } }; -class RetryPolicyTest_CustomRetry_True final : public RetryPolicy { +class RetryPolicyTest_CustomRetry_False final : public RetryPolicy { public: - RetryPolicyTest_CustomRetry_True(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {} + RetryPolicyTest_CustomRetry_False(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&, RetryOptions const&) const override + bool ShouldRetry(std::unique_ptr const& response, RetryOptions const& retryOptions) + const override { - 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; - } - } - } + (void)response; + (void)retryOptions; return false; } }; @@ -220,11 +150,10 @@ TEST(RetryPolicy, ShouldRetry) using namespace std::chrono_literals; RetryOptions const retryOptions{3, 10ms, 60s, {HttpStatusCode::Ok}}; - // 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. + // The default ShouldRetry implementation is to always return true, + // which means we will retry up to the retry count in the options. { - Azure::Core::Context context = Azure::Core::Context(); + Azure::Core::Context context = Azure::Core::Context::ApplicationContext; auto policy = RetryPolicy(retryOptions); RawResponse const* responsePtrSent = nullptr; @@ -234,33 +163,7 @@ 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; - })); - - 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 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_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([&]() { - auto response = std::make_unique(1, 1, HttpStatusCode::Ok, "Test"); responsePtrSent = response.get(); retryCounter++; return response; @@ -275,73 +178,20 @@ TEST(RetryPolicy, ShouldRetry) 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. + // Overriding ShouldRetry to return false will mean we won't retry. { Azure::Core::Context context = Azure::Core::Context(); - auto policy = RetryPolicy(retryOptions); + auto policy = RetryPolicyTest_CustomRetry_False(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, 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; + policies.emplace_back(std::make_unique(policy)); - 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; + auto response = std::make_unique(1, 1, HttpStatusCode::Ok, "Test"); - 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; @@ -353,7 +203,7 @@ TEST(RetryPolicy, ShouldRetry) pipeline.Send(request, context); EXPECT_NE(responsePtrSent, nullptr); - EXPECT_EQ(retryCounter, 3); + EXPECT_EQ(retryCounter, 0); } } From 3b1aa6ab25f7fdcf143f01bd868a46e49c09124b Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com> Date: Fri, 12 Jul 2024 11:16:51 -0700 Subject: [PATCH 3/4] Do not remove changelog entry from a previous beta release --- sdk/core/azure-core/CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 42930eda25..9f0c487341 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -27,6 +27,7 @@ Thank you to our developer community members who helped to make Azure Core bette ### 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 From 9bc6f8b0c1e85020ad99dcb58f896a0e3503cf7f Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk Date: Fri, 12 Jul 2024 11:32:29 -0700 Subject: [PATCH 4/4] Revert "Add a virtual ShouldRetry method to the RetryPolicy for customization. (#5584)" This reverts commit ab90ef68b03684def81492e13542b02406160a85. --- .../inc/azure/core/http/policies/policy.hpp | 26 ++---- sdk/core/azure-core/src/http/retry_policy.cpp | 14 +--- .../azure-core/test/ut/retry_policy_test.cpp | 81 ------------------- 3 files changed, 6 insertions(+), 115 deletions(-) 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..511a1c3a7a 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 @@ -363,7 +363,11 @@ namespace Azure { namespace Core { namespace Http { namespace Policies { /** * @brief HTTP retry policy. */ - class RetryPolicy : public HttpPolicy { + class RetryPolicy +#if !defined(_azure_TESTING_BUILD) + final +#endif + : public HttpPolicy { private: RetryOptions m_retryOptions; @@ -411,26 +415,6 @@ namespace Azure { namespace Core { namespace Http { namespace Policies { int32_t attempt, std::chrono::milliseconds& retryAfter, double jitterFactor = -1) const; - - /** - * @brief Overriding this method customizes the logic of when the RetryPolicy will re-attempt - * a request, based on the returned HTTP response. - * - * @remark A null response pointer means there was no response received from the corresponding - * 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 - * non-retriable errors, including those specified in the RetryOptions, remain evaluated - * before calling ShouldRetry. - * - * @param response An HTTP response returned corresponding to the request sent by the policy. - * @param retryOptions The set of options provided to the RetryPolicy. - * @return Whether or not the HTTP request should be sent again through the pipeline. - */ - virtual bool ShouldRetry( - std::unique_ptr const& response, - RetryOptions const& retryOptions) const; }; /** diff --git a/sdk/core/azure-core/src/http/retry_policy.cpp b/sdk/core/azure-core/src/http/retry_policy.cpp index 29eeae8e4a..217e0bd898 100644 --- a/sdk/core/azure-core/src/http/retry_policy.cpp +++ b/sdk/core/azure-core/src/http/retry_policy.cpp @@ -118,11 +118,6 @@ int32_t RetryPolicy::GetRetryCount(Context const& context) return *ptr; } -bool RetryPolicy::ShouldRetry(std::unique_ptr const&, RetryOptions const&) const -{ - return true; -} - std::unique_ptr RetryPolicy::Send( Request& request, NextHttpPolicy nextPolicy, @@ -146,20 +141,13 @@ 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. + // doesn't need to be retried), then ShouldRetry 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)) - { - return response; - } } catch (const TransportException& e) { 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..b482418ce5 100644 --- a/sdk/core/azure-core/test/ut/retry_policy_test.cpp +++ b/sdk/core/azure-core/test/ut/retry_policy_test.cpp @@ -124,89 +124,8 @@ class RetryPolicyTest final : public RetryPolicy { return m_shouldRetryOnResponse(response, retryOptions, attempt, retryAfter, jitterFactor); } }; - -class RetryPolicyTest_CustomRetry_False final : public RetryPolicy { -public: - RetryPolicyTest_CustomRetry_False(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; - return false; - } -}; } // namespace -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. - { - Azure::Core::Context context = Azure::Core::Context::ApplicationContext; - 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::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); - } - - // Overriding ShouldRetry to return false will mean we won't retry. - { - Azure::Core::Context context = Azure::Core::Context(); - auto policy = RetryPolicyTest_CustomRetry_False(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::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, 0); - } -} - TEST(RetryPolicy, ShouldRetryOnResponse) { using namespace std::chrono_literals;