From a5ff474118e2d502ff05ffdfd4386ea20ce14724 Mon Sep 17 00:00:00 2001 From: Victor Vazquez Date: Wed, 3 Mar 2021 20:51:19 +0000 Subject: [PATCH] Http request counter (#1738) * Add counter to request about the retry number using the Context --- sdk/core/azure-core/CHANGELOG.md | 1 + .../azure-core/inc/azure/core/http/http.hpp | 13 ++- .../azure-core/inc/azure/core/http/policy.hpp | 13 +++ sdk/core/azure-core/src/http/retry_policy.cpp | 41 +++++++- sdk/core/azure-core/test/ut/policy.cpp | 93 +++++++++++++++++++ sdk/core/azure-core/test/ut/url.cpp | 11 ++- 6 files changed, 165 insertions(+), 7 deletions(-) diff --git a/sdk/core/azure-core/CHANGELOG.md b/sdk/core/azure-core/CHANGELOG.md index e20094593..e363ef56e 100644 --- a/sdk/core/azure-core/CHANGELOG.md +++ b/sdk/core/azure-core/CHANGELOG.md @@ -14,6 +14,7 @@ - Moved `NullBodyStream` to internal usage only. It is not meant for public use. - Removed `LimitBodyStream`. - Introduced `Azure::Core::CaseInsensitiveMap` which is now used to store headers in `Azure::Core::Http::Request` and `Azure::Core::Http::RawResponse`. +- Removed `StartTry()` from `Azure::Core::Http::Request`. ### Bug Fixes diff --git a/sdk/core/azure-core/inc/azure/core/http/http.hpp b/sdk/core/azure-core/inc/azure/core/http/http.hpp index 0447176f7..b0339d3e1 100644 --- a/sdk/core/azure-core/inc/azure/core/http/http.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/http.hpp @@ -29,6 +29,9 @@ namespace Azure { namespace Core { namespace Test { class TestHttp_getters_Test; class TestHttp_query_parameter_Test; + class TestHttp_RequestStartTry_Test; + class TestURL_getters_Test; + class TestURL_query_parameter_Test; }}} // namespace Azure::Core::Test #endif @@ -445,6 +448,9 @@ namespace Azure { namespace Core { namespace Http { // make tests classes friends to validate set Retry friend class Azure::Core::Test::TestHttp_getters_Test; friend class Azure::Core::Test::TestHttp_query_parameter_Test; + friend class Azure::Core::Test::TestHttp_RequestStartTry_Test; + friend class Azure::Core::Test::TestURL_getters_Test; + friend class Azure::Core::Test::TestURL_query_parameter_Test; #endif private: @@ -464,6 +470,10 @@ namespace Azure { namespace Core { namespace Http { // adapter will decide chunk size. int64_t m_uploadChunkSize = 0; + // Expected to be called by a Retry policy to reset all headers set after this function was + // previously called + void StartTry(); + public: /** * @brief Construct an #Azure::Core::Http::Request. @@ -583,9 +593,6 @@ namespace Azure { namespace Core { namespace Http { * @brief Get URL. */ Url const& GetUrl() const { return this->m_url; } - // Expected to be called by a Retry policy to reset all headers set after this function was - // previously called - void StartTry(); }; /** diff --git a/sdk/core/azure-core/inc/azure/core/http/policy.hpp b/sdk/core/azure-core/inc/azure/core/http/policy.hpp index cab0c880d..d2366a0f1 100644 --- a/sdk/core/azure-core/inc/azure/core/http/policy.hpp +++ b/sdk/core/azure-core/inc/azure/core/http/policy.hpp @@ -216,6 +216,19 @@ namespace Azure { namespace Core { namespace Http { Context const& ctx, Request& request, NextHttpPolicy nextHttpPolicy) const override; + + /** + * @brief Get the Retry Count from the context. + * + * @remark The sentinel `-1` is returned if there is no information in the \p Context about + * #RetryPolicy is trying to send a request. Then `0` is returned for the first try of sending a + * request by the #RetryPolicy. Any subsequent retry will be referenced with a number greater + * than 0. + * + * @param context The context used to call send request. + * @return A positive number indicating the current intent to send the request. + */ + static int GetRetryNumber(Context const& context); }; /** diff --git a/sdk/core/azure-core/src/http/retry_policy.cpp b/sdk/core/azure-core/src/http/retry_policy.cpp index 0638a1f10..c1cf66ece 100644 --- a/sdk/core/azure-core/src/http/retry_policy.cpp +++ b/sdk/core/azure-core/src/http/retry_policy.cpp @@ -122,15 +122,49 @@ bool ShouldRetryOnResponse( return true; } + +static constexpr char RetryKey[] = "AzureSdkRetryPolicyCounter"; + +/** + * @brief Creates a new #Context node from \p parent with the information about the retrying while + * sending an Http request. + * + * @param parent The parent context for the new created. + * @return Context with information about retry counter. + */ +Context inline CreateRetryContext(Context const& parent) +{ + // First try as default + int retryCount = 0; + if (parent.HasKey(RetryKey)) + { + retryCount = parent[RetryKey].Get() + 1; + } + return parent.WithValue(RetryKey, retryCount); +} } // namespace +int Azure::Core::Http::RetryPolicy::GetRetryNumber(Context const& context) +{ + if (!context.HasKey(RetryKey)) + { + // Context with no data abut sending request with retry policy = -1 + // First try = 0 + // Second try = 1 + // third try = 2 + // ... + return -1; + } + return context[RetryKey].Get(); +} + std::unique_ptr Azure::Core::Http::RetryPolicy::Send( Context const& ctx, Request& request, NextHttpPolicy nextHttpPolicy) const { auto const shouldLog = Logging::Internal::ShouldLog(Logging::LogLevel::Informational); - + auto retryContext = CreateRetryContext(ctx); for (RetryNumber attempt = 1;; ++attempt) { Delay retryAfter{}; @@ -140,7 +174,7 @@ std::unique_ptr Azure::Core::Http::RetryPolicy::Send( try { - auto response = nextHttpPolicy.Send(ctx, request); + auto response = nextHttpPolicy.Send(retryContext, request); // 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. @@ -179,5 +213,8 @@ std::unique_ptr Azure::Core::Http::RetryPolicy::Send( // Restore the original query parameters before next retry request.GetUrl().SetQueryParameters(std::move(originalQueryParameters)); + + // Update retry number + retryContext = CreateRetryContext(retryContext); } } diff --git a/sdk/core/azure-core/test/ut/policy.cpp b/sdk/core/azure-core/test/ut/policy.cpp index 1143acaf7..03a70c973 100644 --- a/sdk/core/azure-core/test/ut/policy.cpp +++ b/sdk/core/azure-core/test/ut/policy.cpp @@ -23,6 +23,58 @@ public: return nullptr; } }; + +// A policy to test retry state +static int retryCounterState = 0; +struct TestRetryPolicySharedState : public Azure::Core::Http::HttpPolicy +{ + std::unique_ptr Clone() const override + { + return std::make_unique(*this); + } + + std::unique_ptr Send( + Azure::Core::Context const& ctx, + Azure::Core::Http::Request& request, + Azure::Core::Http::NextHttpPolicy nextHttpPolicy) const override + { + EXPECT_EQ(retryCounterState, Azure::Core::Http::RetryPolicy::GetRetryNumber(ctx)); + retryCounterState += 1; + return nextHttpPolicy.Send(ctx, request); + } +}; + +class SuccessAfter : public Azure::Core::Http::HttpPolicy { +private: + int m_successAfter; // Always success + +public: + std::unique_ptr Clone() const override + { + return std::make_unique(*this); + } + + SuccessAfter(int successAfter = 1) : m_successAfter(successAfter) {} + + std::unique_ptr Send( + Azure::Core::Context const& context, + Azure::Core::Http::Request&, + Azure::Core::Http::NextHttpPolicy) const override + { + auto retryNumber = Azure::Core::Http::RetryPolicy::GetRetryNumber(context); + if (retryNumber == m_successAfter) + { + auto response = std::make_unique( + 1, 1, Azure::Core::Http::HttpStatusCode::Ok, "All Fine"); + return response; + } + + auto retryResponse = std::make_unique( + 1, 1, Azure::Core::Http::HttpStatusCode::ServiceUnavailable, "retry please :)"); + return retryResponse; + } +}; + } // namespace TEST(Policy, throwWhenNoTransportPolicy) @@ -88,3 +140,44 @@ TEST(Policy, ValuePolicy) ASSERT_EQ(headers, decltype(headers)({{"hdrkey1", "HdrVal1"}, {"hdrkey2", "HdrVal2"}})); ASSERT_EQ(queryParams, decltype(queryParams)({{"QryKey1", "QryVal1"}, {"QryKey2", "QryVal2"}})); } + +TEST(Policy, RetryPolicyCounter) +{ + using namespace Azure::Core; + using namespace Azure::Core::Http; + using namespace Azure::Core::Internal::Http; + + // Check when there's no info about retry on the context + auto initialContext = GetApplicationContext(); + EXPECT_EQ(-1, RetryPolicy::GetRetryNumber(initialContext)); + + // Pipeline with retry test + std::vector> policies; + RetryOptions opt; + policies.push_back(std::make_unique(opt)); + policies.push_back(std::make_unique()); + policies.push_back(std::make_unique()); + + HttpPipeline pipeline(policies); + Request request(HttpMethod::Get, Url("url")); + pipeline.Send(initialContext, request); +} + +TEST(Policy, RetryPolicyRetryCycle) +{ + using namespace Azure::Core; + using namespace Azure::Core::Http; + using namespace Azure::Core::Internal::Http; + + // Pipeline with retry test + std::vector> policies; + RetryOptions opt; + opt.RetryDelay = std::chrono::milliseconds(10); + policies.push_back(std::make_unique(opt)); + policies.push_back(std::make_unique()); + policies.push_back(std::make_unique(3)); + + HttpPipeline pipeline(policies); + Request request(HttpMethod::Get, Url("url")); + pipeline.Send(GetApplicationContext(), request); +} diff --git a/sdk/core/azure-core/test/ut/url.cpp b/sdk/core/azure-core/test/ut/url.cpp index 17aff0ed3..bb544b061 100644 --- a/sdk/core/azure-core/test/ut/url.cpp +++ b/sdk/core/azure-core/test/ut/url.cpp @@ -5,11 +5,18 @@ #include +namespace Azure { namespace Core { namespace Test { + + class TestURL : public ::testing::Test { + }; + +}}} // namespace Azure::Core::Test + using namespace Azure::Core; namespace Azure { namespace Core { namespace Test { - TEST(URL, getters) + TEST(TestURL, getters) { Http::HttpMethod httpMethod = Http::HttpMethod::Get; Http::Url url("http://test.url.com"); @@ -79,7 +86,7 @@ namespace Azure { namespace Core { namespace Test { EXPECT_FALSE(headers.count("newheader")); } - TEST(URL, query_parameter) + TEST(TestURL, query_parameter) { Http::HttpMethod httpMethod = Http::HttpMethod::Put; Http::Url url("http://test.com");