From 43dcc6c495683c59f5a22479fd47bb644576c150 Mon Sep 17 00:00:00 2001 From: Anton Kolesnyk <41349689+antkmsft@users.noreply.github.com> Date: Fri, 10 Jul 2020 00:23:40 -0700 Subject: [PATCH] Retry policy (#241) --- .clang-format | 2 +- sdk/core/azure-core/CMakeLists.txt | 5 +- .../inc/credentials/credentials.hpp | 33 +--- sdk/core/azure-core/inc/http/http.hpp | 2 +- sdk/core/azure-core/inc/http/policy.hpp | 26 ++- .../src/credentials/credentials.cpp | 2 +- sdk/core/azure-core/src/http/response.cpp | 5 +- sdk/core/azure-core/src/http/retry_policy.cpp | 173 ++++++++++++++++++ .../src/azure_core_with_curl_bodyBuffer.cpp | 4 +- .../src/azure_core_with_curl_bodyStream.cpp | 4 +- 10 files changed, 207 insertions(+), 49 deletions(-) create mode 100644 sdk/core/azure-core/src/http/retry_policy.cpp diff --git a/.clang-format b/.clang-format index 0e09041b0..b74f533b6 100644 --- a/.clang-format +++ b/.clang-format @@ -33,7 +33,7 @@ CompactNamespaces: true Cpp11BracedListStyle: true FixNamespaceComments: true IndentWidth: 2 -IncludeBlocks: Regroup +IncludeBlocks: Preserve IndentCaseLabels: true NamespaceIndentation: Inner PointerAlignment: Left diff --git a/sdk/core/azure-core/CMakeLists.txt b/sdk/core/azure-core/CMakeLists.txt index c84b8b422..176b13443 100644 --- a/sdk/core/azure-core/CMakeLists.txt +++ b/sdk/core/azure-core/CMakeLists.txt @@ -18,12 +18,13 @@ add_library ( ${TARGET_NAME} src/context.cpp src/credentials/credentials.cpp - src/http/curl/curl.cpp src/credentials/policy/policies.cpp + src/http/body_stream.cpp + src/http/curl/curl.cpp src/http/policy.cpp src/http/request.cpp src/http/response.cpp - src/http/body_stream.cpp + src/http/retry_policy.cpp src/http/url.cpp src/http/winhttp/win_http_transport.cpp src/strings.cpp diff --git a/sdk/core/azure-core/inc/credentials/credentials.hpp b/sdk/core/azure-core/inc/credentials/credentials.hpp index 413485c82..5252fea0f 100644 --- a/sdk/core/azure-core/inc/credentials/credentials.hpp +++ b/sdk/core/azure-core/inc/credentials/credentials.hpp @@ -38,35 +38,10 @@ namespace Azure { namespace Core { namespace Credentials { std::string const m_clientSecret; public: - ClientSecretCredential( - std::string const& tenantId, - std::string const& clientId, - std::string const& clientSecret) - : m_tenantId(tenantId), m_clientId(clientId), m_clientSecret(clientSecret) - { - } - - ClientSecretCredential( - std::string const&& tenantId, - std::string const& clientId, - std::string const& clientSecret) - : m_tenantId(std::move(tenantId)), m_clientId(clientId), m_clientSecret(clientSecret) - { - } - - ClientSecretCredential( - std::string const&& tenantId, - std::string const&& clientId, - std::string const& clientSecret) - : m_tenantId(std::move(tenantId)), m_clientId(std::move(clientId)), - m_clientSecret(clientSecret) - { - } - - ClientSecretCredential( - std::string const&& tenantId, - std::string const&& clientId, - std::string const&& clientSecret) + explicit ClientSecretCredential( + std::string tenantId, + std::string clientId, + std::string clientSecret) : m_tenantId(std::move(tenantId)), m_clientId(std::move(clientId)), m_clientSecret(std::move(clientSecret)) { diff --git a/sdk/core/azure-core/inc/http/http.hpp b/sdk/core/azure-core/inc/http/http.hpp index 09c7736d7..44f65e45a 100644 --- a/sdk/core/azure-core/inc/http/http.hpp +++ b/sdk/core/azure-core/inc/http/http.hpp @@ -323,7 +323,7 @@ namespace Azure { namespace Core { namespace Http { int32_t GetMinorVersion() const { return this->m_minorVersion; } HttpStatusCode GetStatusCode() const; std::string const& GetReasonPhrase(); - std::map const& GetHeaders(); + std::map const& GetHeaders() const; std::unique_ptr GetBodyStream() { // If m_bodyStream was moved before. nullpr is returned diff --git a/sdk/core/azure-core/inc/http/policy.hpp b/sdk/core/azure-core/inc/http/policy.hpp index 5dd944f41..b79bb1bc4 100644 --- a/sdk/core/azure-core/inc/http/policy.hpp +++ b/sdk/core/azure-core/inc/http/policy.hpp @@ -8,6 +8,9 @@ #include "http.hpp" #include "transport.hpp" +#include +#include + namespace Azure { namespace Core { namespace Http { class NextHttpPolicy; @@ -72,8 +75,18 @@ namespace Azure { namespace Core { namespace Http { struct RetryOptions { - int16_t MaxRetries = 5; - int32_t RetryDelayMsec = 500; + int MaxRetries = 3; + + std::chrono::milliseconds RetryDelay = std::chrono::seconds(4); + decltype(RetryDelay) MaxRetryDelay = std::chrono::minutes(2); + + std::vector StatusCodes{ + HttpStatusCode::RequestTimeout, + HttpStatusCode::InternalServerError, + HttpStatusCode::BadGateway, + HttpStatusCode::ServiceUnavailable, + HttpStatusCode::GatewayTimeout, + }; }; class RetryPolicy : public HttpPolicy { @@ -81,17 +94,12 @@ namespace Azure { namespace Core { namespace Http { RetryOptions m_retryOptions; public: - explicit RetryPolicy(RetryOptions options) : m_retryOptions(options) {} + explicit RetryPolicy(RetryOptions options) : m_retryOptions(std::move(options)) {} HttpPolicy* Clone() const override { return new RetryPolicy(m_retryOptions); } std::unique_ptr Send(Context& ctx, Request& request, NextHttpPolicy nextHttpPolicy) - const override - { - // Do real work here - // nextPolicy->Process(ctx, message, ) - return nextHttpPolicy.Send(ctx, request); - } + const override; }; class RequestIdPolicy : public HttpPolicy { diff --git a/sdk/core/azure-core/src/credentials/credentials.cpp b/sdk/core/azure-core/src/credentials/credentials.cpp index a214e3005..8de5a0c1e 100644 --- a/sdk/core/azure-core/src/credentials/credentials.cpp +++ b/sdk/core/azure-core/src/credentials/credentials.cpp @@ -147,7 +147,7 @@ AccessToken ClientSecretCredential::GetToken( break; } - expiresInSeconds = (expiresInSeconds * 10) + (c - '0'); + expiresInSeconds = (expiresInSeconds * 10) + (static_cast(c) - '0'); } responseBodyPos = responseBody.find(':', responseBody.find(jsonAccessToken)); diff --git a/sdk/core/azure-core/src/http/response.cpp b/sdk/core/azure-core/src/http/response.cpp index d8d8c60a2..b83d4c4df 100644 --- a/sdk/core/azure-core/src/http/response.cpp +++ b/sdk/core/azure-core/src/http/response.cpp @@ -1,8 +1,9 @@ // Copyright (c) Microsoft Corporation. All rights reserved. // SPDX-License-Identifier: MIT -#include #include + +#include #include #include #include @@ -13,7 +14,7 @@ HttpStatusCode Response::GetStatusCode() const { return m_statusCode; } std::string const& Response::GetReasonPhrase() { return m_reasonPhrase; } -std::map const& Response::GetHeaders() { return this->m_headers; } +std::map const& Response::GetHeaders() const { return this->m_headers; } void Response::AddHeader(std::string const& header) { diff --git a/sdk/core/azure-core/src/http/retry_policy.cpp b/sdk/core/azure-core/src/http/retry_policy.cpp new file mode 100644 index 000000000..b0b9d6d2e --- /dev/null +++ b/sdk/core/azure-core/src/http/retry_policy.cpp @@ -0,0 +1,173 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// SPDX-License-Identifier: MIT + +#include + +#include +#include +#include +#include + +using namespace Azure::Core::Http; + +namespace { +typedef decltype(RetryOptions::RetryDelay) Delay; +typedef decltype(RetryOptions::MaxRetries) RetryNumber; + +bool GetResponseHeaderBasedDelay(Response const& response, Delay& retryAfter) +{ + // Try to find retry-after headers. There are several of them possible. + auto const& responseHeaders = response.GetHeaders(); + auto const responseHeadersEnd = responseHeaders.end(); + auto header = responseHeadersEnd; + if (((header = responseHeaders.find("retry-after-ms")) != responseHeadersEnd) + || ((header = responseHeaders.find("x-ms-retry-after-ms")) != responseHeadersEnd)) + { + // The headers above are in milliseconds. + retryAfter = std::chrono::milliseconds(std::stoi(header->second)); + return true; + } + + if ((header = responseHeaders.find("Retry-After")) != responseHeadersEnd) + { + // This header is in seconds. + retryAfter = std::chrono::seconds(std::stoi(header->second)); + return true; + + // Tracked by https://github.com/Azure/azure-sdk-for-cpp/issues/262 + // ---------------------------------------------------------------- + // + // To be accurate, the Retry-After header is EITHER seconds, or a DateTime. So we need to + // write a parser for that (and handle the case when parsing seconds fails). + // More info: + // * Retry-After header: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Retry-After + // * HTTP Date format: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Date + // * Parsing the date: https://en.cppreference.com/w/cpp/locale/time_get + // * Get system datetime: https://en.cppreference.com/w/cpp/chrono/system_clock/now + // * Subtract datetimes to get duration: + // https://en.cppreference.com/w/cpp/chrono/time_point/operator_arith2 + } + + return false; +} + +Delay CalculateExponentialDelay(RetryOptions const& retryOptions, RetryNumber attempt) +{ + constexpr auto beforeLastBit = std::numeric_limits::digits + - (std::numeric_limits::is_signed ? 1 : 0); + + // Scale exponentially: 1 x RetryDelay on 1st attempt, 2x on 2nd, 4x on 3rd, 8x on 4th ... all the + // way up to std::numeric_limits::max() * RetryDelay. + auto exponentialRetryAfter = retryOptions.RetryDelay + * ((attempt <= beforeLastBit) ? (1 << attempt) : std::numeric_limits::max()); + + // jitterFactor is a random double number in the range [0.8 .. 1.3) + auto jitterFactor = 0.8 + (static_cast(std::rand()) / RAND_MAX) * 0.5; + + // Multiply exponentialRetryAfter by jitterFactor + exponentialRetryAfter = Delay(static_cast( + (std::chrono::duration(exponentialRetryAfter) * jitterFactor) + .count())); + + return std::min(exponentialRetryAfter, retryOptions.MaxRetryDelay); +} + +bool WasLastAttempt(RetryOptions const& retryOptions, RetryNumber attempt) +{ + return attempt > retryOptions.MaxRetries; +} + +bool ShouldRetryOnTransportFailure( + RetryOptions const& retryOptions, + RetryNumber attempt, + Delay& retryAfter) +{ + // Are we out of retry attempts? + if (WasLastAttempt(retryOptions, attempt)) + { + return false; + } + + retryAfter = CalculateExponentialDelay(retryOptions, attempt); + return true; +} + +bool ShouldRetryOnResponse( + Response const& response, + RetryOptions const& retryOptions, + RetryNumber attempt, + Delay& retryAfter) +{ + // 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; + auto const statusCodesEnd = statusCodes.end(); + if (std::find(statusCodes.begin(), statusCodesEnd, response.GetStatusCode()) == statusCodesEnd) + { + return false; + } + + if (!GetResponseHeaderBasedDelay(response, retryAfter)) + { + retryAfter = CalculateExponentialDelay(retryOptions, attempt); + } + + return true; +} +} // namespace + +std::unique_ptr RetryPolicy::Send( + Context& ctx, + Request& request, + NextHttpPolicy nextHttpPolicy) const +{ + for (RetryNumber attempt = 1;; ++attempt) + { + Delay retryAfter{}; + try + { + auto response = nextHttpPolicy.Send(ctx, 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. + if (!ShouldRetryOnResponse(*response.get(), m_retryOptions, attempt, retryAfter)) + { + return response; + } + } + catch (CouldNotResolveHostException const&) + { + if (!ShouldRetryOnTransportFailure(m_retryOptions, attempt, retryAfter)) + { + throw; + } + } + catch (TransportException const&) + { + if (!ShouldRetryOnTransportFailure(m_retryOptions, attempt, retryAfter)) + { + throw; + } + } + + request.StartRetry(); + if (auto bodyStream = request.GetBodyStream()) + { + bodyStream->Rewind(); + } + + // Sleep(0) behavior is implementation-defined: it may yield, or may do nothing. Let's make sure + // we proceed immediately if it is 0. + if (retryAfter.count() > 0) + { + std::this_thread::sleep_for(retryAfter); + } + + ctx.ThrowIfCanceled(); + } +} diff --git a/sdk/samples/http_client/curl/src/azure_core_with_curl_bodyBuffer.cpp b/sdk/samples/http_client/curl/src/azure_core_with_curl_bodyBuffer.cpp index b326ea77c..91ace1da0 100644 --- a/sdk/samples/http_client/curl/src/azure_core_with_curl_bodyBuffer.cpp +++ b/sdk/samples/http_client/curl/src/azure_core_with_curl_bodyBuffer.cpp @@ -61,11 +61,11 @@ int main() doDeleteRequest(context, httpPipeline); doPatchRequest(context, httpPipeline); } - catch (Http::CouldNotResolveHostException& e) + catch (Http::CouldNotResolveHostException const& e) { cout << e.what() << endl; } - catch (Http::TransportException& e) + catch (Http::TransportException const& e) { cout << e.what() << endl; } diff --git a/sdk/samples/http_client/curl/src/azure_core_with_curl_bodyStream.cpp b/sdk/samples/http_client/curl/src/azure_core_with_curl_bodyStream.cpp index f4b5c09f8..d64011185 100644 --- a/sdk/samples/http_client/curl/src/azure_core_with_curl_bodyStream.cpp +++ b/sdk/samples/http_client/curl/src/azure_core_with_curl_bodyStream.cpp @@ -61,11 +61,11 @@ int main() doNoPathGetRequest(context, httpPipeline); doPutRequest(context, httpPipeline); } - catch (Http::CouldNotResolveHostException& e) + catch (Http::CouldNotResolveHostException const& e) { cout << e.what() << endl; } - catch (Http::TransportException& e) + catch (Http::TransportException const& e) { cout << e.what() << endl; }