Retry policy (#241)

.clang-format

@@ -33,7 +33,7 @@ CompactNamespaces: true
 Cpp11BracedListStyle: true
 FixNamespaceComments: true
 IndentWidth: 2
-IncludeBlocks: Regroup
+IncludeBlocks: Preserve
 IndentCaseLabels: true
 NamespaceIndentation: Inner
 PointerAlignment: Left

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

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))
     {

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<std::string, std::string> const& GetHeaders();
+    std::map<std::string, std::string> const& GetHeaders() const;
     std::unique_ptr<BodyStream> GetBodyStream()
     {
       // If m_bodyStream was moved before. nullpr is returned

sdk/core/azure-core/inc/http/policy.hpp

@@ -8,6 +8,9 @@
 #include "http.hpp"
 #include "transport.hpp"
 
+#include <chrono>
+#include <utility>
+
 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<HttpStatusCode> 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<Response> 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 {

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<long long>(c) - '0');
     }
 
     responseBodyPos = responseBody.find(':', responseBody.find(jsonAccessToken));

sdk/core/azure-core/src/http/response.cpp

@@ -1,8 +1,9 @@
 // Copyright (c) Microsoft Corporation. All rights reserved.
 // SPDX-License-Identifier: MIT
 
-#include <cctype>
 #include <http/http.hpp>
+
+#include <cctype>
 #include <map>
 #include <string>
 #include <vector>
@@ -13,7 +14,7 @@ HttpStatusCode Response::GetStatusCode() const { return m_statusCode; }
 
 std::string const& Response::GetReasonPhrase() { return m_reasonPhrase; }
 
-std::map<std::string, std::string> const& Response::GetHeaders() { return this->m_headers; }
+std::map<std::string, std::string> const& Response::GetHeaders() const { return this->m_headers; }
 
 void Response::AddHeader(std::string const& header)
 {

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 <http/policy.hpp>
+
+#include <algorithm>
+#include <cstdlib>
+#include <limits>
+#include <thread>
+
+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<RetryNumber>::digits
+      - (std::numeric_limits<RetryNumber>::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<RetryNumber>::max() * RetryDelay.
+  auto exponentialRetryAfter = retryOptions.RetryDelay
+      * ((attempt <= beforeLastBit) ? (1 << attempt) : std::numeric_limits<RetryNumber>::max());
+
+  // jitterFactor is a random double number in the range [0.8 .. 1.3)
+  auto jitterFactor = 0.8 + (static_cast<double>(std::rand()) / RAND_MAX) * 0.5;
+
+  // Multiply exponentialRetryAfter by jitterFactor
+  exponentialRetryAfter = Delay(static_cast<Delay::rep>(
+      (std::chrono::duration<double, Delay::period>(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<Response> 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();
+  }
+}

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;
   }

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;
   }