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..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 @@ -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. @@ -376,11 +368,6 @@ namespace Azure { namespace Core { namespace Http { namespace Policies { 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; @@ -415,7 +402,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, @@ -428,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 `false`. 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 32067e5726..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 false; -} - std::unique_ptr RetryPolicy::Send( Request& request, NextHttpPolicy nextPolicy, @@ -145,24 +140,9 @@ 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)) - { - 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)) + // 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 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 @@ -235,6 +215,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 cbfe5cd99f..b482418ce5 100644 --- a/sdk/core/azure-core/test/ut/retry_policy_test.cpp +++ b/sdk/core/azure-core/test/ut/retry_policy_test.cpp @@ -13,356 +13,118 @@ 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(); - } - - 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); - } - - 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); - } - - 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); - } - }; - - 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); - } - - 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) - { - 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; - } - }; -}}} // namespace Azure::Core::Test - -using namespace Azure::Core::Test; - -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. +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 { - 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::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); + return m_send(); } - // If the status code is retriable based on the options, ShouldRetry doesn't get called. - // Testing short-circuit evaluation + std::unique_ptr Clone() const override { - 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; - })); + return std::make_unique(*this); + } +}; - Azure::Core::Http::_internal::HttpPipeline pipeline(policies); +class RetryPolicyTest final : public RetryPolicy { +private: + std::function + m_shouldRetryOnTransportFailure; - Request request(HttpMethod::Get, Azure::Core::Url("https://www.microsoft.com")); - pipeline.Send(request, context); + std::function< + bool(RawResponse const&, RetryOptions const&, int32_t, std::chrono::milliseconds&, double)> + m_shouldRetryOnResponse; - EXPECT_NE(responsePtrSent, nullptr); - EXPECT_EQ(retryCounter, 3); +public: + bool BaseShouldRetryOnTransportFailure( + RetryOptions const& retryOptions, + int32_t attempt, + std::chrono::milliseconds& retryAfter, + double jitterFactor) const + { + return RetryPolicy::ShouldRetryOnTransportFailure( + retryOptions, attempt, retryAfter, jitterFactor); } - // 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. + bool BaseShouldRetryOnResponse( + RawResponse const& response, + RetryOptions const& retryOptions, + int32_t attempt, + std::chrono::milliseconds& retryAfter, + double jitterFactor) const { - 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; - })); - - 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); + return RetryPolicy::ShouldRetryOnResponse( + response, retryOptions, attempt, retryAfter, jitterFactor); } - // Overriding ShouldRetry to return true will mean we will retry, when the status code isn't - // retriable based on the options. + 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); + })) { - 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) + std::unique_ptr Clone() const override { - 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); + return std::make_unique(*this); + } - Request request(HttpMethod::Get, Azure::Core::Url("https://www.microsoft.com")); - pipeline.Send(request, context); +protected: + bool ShouldRetryOnTransportFailure( + RetryOptions const& retryOptions, + int32_t attempt, + std::chrono::milliseconds& retryAfter, + double jitterFactor) const override + { + return m_shouldRetryOnTransportFailure(retryOptions, attempt, retryAfter, jitterFactor); + } - EXPECT_NE(responsePtrSent, nullptr); - EXPECT_EQ(retryCounter, 3); + 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); } -} +}; +} // namespace TEST(RetryPolicy, ShouldRetryOnResponse) { @@ -596,38 +358,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) {