From 9ccd206ff88f1550ecf94139806085b69a341cd2 Mon Sep 17 00:00:00 2001 From: Ahson Khan Date: Thu, 11 Jul 2024 20:39:37 -0400 Subject: [PATCH] Update the RetryPolicy for the GA release, keeping ShouldRetry extension point hidden. (#5771) * Update the RetryPolicy for the GA release, keeping ShouldRetry extension point hidden. * Mark test helper virtual functions private, so they aren't accessible/callable by callers. * Update the changelog. * Update CL. --- sdk/core/azure-core/CHANGELOG.md | 1 + .../inc/azure/core/http/policies/policy.hpp | 21 +- .../azure-core/test/ut/retry_policy_test.cpp | 411 +++++++++--------- 3 files changed, 229 insertions(+), 204 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index 456ca77c7..7c0b68b6f 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -11,6 +11,7 @@ ### Other Changes - Updated JSON library to 3.11.3. +- Hide methods on the `RetryPolicy` that are not intended for public use. ## 1.13.0-beta.1 (2024-06-06) 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 7eae35118..302c34b88 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,6 +30,14 @@ #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. @@ -363,7 +371,16 @@ 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 { +#if defined(_azure_TESTING_BUILD) + // make tests classes friends + friend class Azure::Core::Test::RetryPolicyTest; + friend class Azure::Core::Test::RetryLogic; +#endif private: RetryOptions m_retryOptions; @@ -398,7 +415,7 @@ namespace Azure { namespace Core { namespace Http { namespace Policies { */ static int32_t GetRetryCount(Context const& context); - protected: + private: 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 5c5b069ac..cbfe5cd99 100644 --- a/sdk/core/azure-core/test/ut/retry_policy_test.cpp +++ b/sdk/core/azure-core/test/ut/retry_policy_test.cpp @@ -13,207 +13,214 @@ using namespace Azure::Core::Http; using namespace Azure::Core::Http::Policies; using namespace Azure::Core::Http::Policies::_internal; -namespace { -class TestTransportPolicy final : public HttpPolicy { -private: - std::function()> m_send; +namespace Azure { namespace Core { namespace Test { + class TestTransportPolicy final : public HttpPolicy { + private: + std::function()> m_send; -public: - TestTransportPolicy(std::function()> send) : m_send(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 Send( + Request&, + NextHttpPolicy, + Azure::Core::Context const&) const override + { + return m_send(); + } - std::unique_ptr Clone() const override - { - return std::make_unique(*this); - } -}; + std::unique_ptr Clone() const override + { + return std::make_unique(*this); + } + }; -class RetryPolicyTest final : public RetryPolicy { -private: - std::function - m_shouldRetryOnTransportFailure; + 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; + 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); - } + 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; - } -}; - -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) + protected: + bool ShouldRetry(std::unique_ptr const&, RetryOptions const&) const override { return true; } + }; - if (static_cast>( - response->GetStatusCode()) - >= 400) + class RetryPolicyTest_CustomRetry_Throw final : public RetryPolicy { + public: + RetryPolicyTest_CustomRetry_Throw(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) { - 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) + std::unique_ptr Clone() const override { - 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; + return std::make_unique(*this); + } - if (error == "InternalError" || error == "OperationTimedOut" || error == "ServerBusy") + 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()) { - return true; + 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; } - return false; - } -}; -} // namespace + }; +}}} // namespace Azure::Core::Test + +using namespace Azure::Core::Test; TEST(RetryPolicy, ShouldRetry) { @@ -589,38 +596,38 @@ TEST(RetryPolicy, ShouldRetryOnTransportFailure) EXPECT_EQ(jitterReceived, -1); } -namespace { -class RetryLogic final : private RetryPolicy { - RetryLogic() : RetryPolicy(RetryOptions()) {} - ~RetryLogic() {} +namespace Azure { namespace Core { namespace Test { + 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 + RetryLogic const RetryLogic::g_retryPolicy; +}}} // namespace Azure::Core::Test TEST(RetryPolicy, Exponential) {