Update the RetryPolicy and ShouldRetry customization logic to allow l…

…oosening the retry condition. (#5656)

* Update the RetryPolicy and ShouldRetry customization logic to allow
loosen the retry condition.

* Add CL entry and update doc comment.

* Move WasLastAttempt check out, and add more tests.

* Address PR feedback, remove use of void and unused params.

sdk/core/azure-core/CHANGELOG.md

@@ -13,6 +13,7 @@
 ### Other Changes
 
 - [[#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)_)
+- [[#5515]](https://github.com/Azure/azure-sdk-for-cpp/issues/5515) Add a `ShouldRetry` virtual method to the retry policy to enable customization of service-specific retry logic.
 
 ### Acknowledgments
 

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

@@ -420,7 +420,7 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
        * 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 true. The
+       * @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.
        *

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

@@ -120,7 +120,7 @@ int32_t RetryPolicy::GetRetryCount(Context const& context)
 
 bool RetryPolicy::ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) const
 {
-  return true;
+  return false;
 }
 
 std::unique_ptr<RawResponse> RetryPolicy::Send(
@@ -145,19 +145,27 @@ std::unique_ptr<RawResponse> RetryPolicy::Send(
     {
       auto response = nextPolicy.Send(request, retryContext);
 
-      // 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 ShouldRetryOnResponse returns false.
-      if (!ShouldRetryOnResponse(*response.get(), m_retryOptions, attempt, retryAfter))
+      // 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))
       {
-        // 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
         return response;
       }
 
-      // Service SDKs can inject custom logic to define whether the request should be retried,
-      // based on the response. The default is true.
-      if (!ShouldRetry(response, m_retryOptions))
+      // 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 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
         return response;
       }
     }
@@ -227,12 +235,6 @@ 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;

sdk/core/azure-core/test/ut/retry_policy_test.cpp

@@ -125,21 +125,91 @@ class RetryPolicyTest final : public RetryPolicy {
   }
 };
 
-class RetryPolicyTest_CustomRetry_False final : public RetryPolicy {
+class RetryPolicyTest_CustomRetry_True final : public RetryPolicy {
 public:
-  RetryPolicyTest_CustomRetry_False(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {}
+  RetryPolicyTest_CustomRetry_True(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {}
 
   std::unique_ptr<HttpPolicy> Clone() const override
   {
-    return std::make_unique<RetryPolicyTest_CustomRetry_False>(*this);
+    return std::make_unique<RetryPolicyTest_CustomRetry_True>(*this);
   }
 
 protected:
-  bool ShouldRetry(std::unique_ptr<RawResponse> const& response, RetryOptions const& retryOptions)
-      const override
+  bool ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) const override
   {
-    (void)response;
-    (void)retryOptions;
+    return true;
+  }
+};
+
+class RetryPolicyTest_CustomRetry_Throw final : public RetryPolicy {
+public:
+  RetryPolicyTest_CustomRetry_Throw(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {}
+
+  std::unique_ptr<HttpPolicy> Clone() const override
+  {
+    return std::make_unique<RetryPolicyTest_CustomRetry_Throw>(*this);
+  }
+
+protected:
+  bool ShouldRetry(std::unique_ptr<RawResponse> 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<HttpPolicy> Clone() const override
+  {
+    return std::make_unique<RetryPolicyTest_CustomRetry_CopySource>(*this);
+  }
+
+protected:
+  bool ShouldRetry(std::unique_ptr<RawResponse> const& response, RetryOptions const&) const override
+  {
+    if (response == nullptr)
+    {
+      return true;
+    }
+
+    if (static_cast<std::underlying_type_t<Azure::Core::Http::HttpStatusCode>>(
+            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<std::underlying_type_t<Azure::Core::Http::HttpStatusCode>>(
+            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;
   }
 };
@@ -150,10 +220,11 @@ TEST(RetryPolicy, ShouldRetry)
   using namespace std::chrono_literals;
   RetryOptions const retryOptions{3, 10ms, 60s, {HttpStatusCode::Ok}};
 
-  // The default ShouldRetry implementation is to always return true,
-  // which means we will retry up to the retry count in the options.
+  // 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.
   {
-    Azure::Core::Context context = Azure::Core::Context::ApplicationContext;
+    Azure::Core::Context context = Azure::Core::Context();
     auto policy = RetryPolicy(retryOptions);
 
     RawResponse const* responsePtrSent = nullptr;
@@ -163,7 +234,6 @@ TEST(RetryPolicy, ShouldRetry)
     policies.emplace_back(std::make_unique<RetryPolicy>(policy));
     policies.emplace_back(std::make_unique<TestTransportPolicy>([&]() {
       auto response = std::make_unique<RawResponse>(1, 1, HttpStatusCode::Ok, "Test");
-
       responsePtrSent = response.get();
       retryCounter++;
       return response;
@@ -178,20 +248,46 @@ TEST(RetryPolicy, ShouldRetry)
     EXPECT_EQ(retryCounter, 3);
   }
 
-  // Overriding ShouldRetry to return false will mean we won't retry.
+  // If the status code is retriable based on the options, ShouldRetry doesn't get called.
+  // Testing short-circuit evaluation
   {
     Azure::Core::Context context = Azure::Core::Context();
-    auto policy = RetryPolicyTest_CustomRetry_False(retryOptions);
+    auto policy = RetryPolicyTest_CustomRetry_Throw(retryOptions);
 
     RawResponse const* responsePtrSent = nullptr;
     int32_t retryCounter = -1;
 
     std::vector<std::unique_ptr<Azure::Core::Http::Policies::HttpPolicy>> policies;
-    policies.emplace_back(std::make_unique<RetryPolicyTest_CustomRetry_False>(policy));
-
+    policies.emplace_back(std::make_unique<RetryPolicyTest_CustomRetry_Throw>(policy));
     policies.emplace_back(std::make_unique<TestTransportPolicy>([&]() {
       auto response = std::make_unique<RawResponse>(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);
+  }
 
+  // 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.
+  {
+    Azure::Core::Context context = Azure::Core::Context();
+    auto policy = RetryPolicy(retryOptions);
+
+    RawResponse const* responsePtrSent = nullptr;
+    int32_t retryCounter = -1;
+
+    std::vector<std::unique_ptr<Azure::Core::Http::Policies::HttpPolicy>> policies;
+    policies.emplace_back(std::make_unique<RetryPolicy>(policy));
+    policies.emplace_back(std::make_unique<TestTransportPolicy>([&]() {
+      auto response = std::make_unique<RawResponse>(1, 1, HttpStatusCode::Accepted, "Test");
       responsePtrSent = response.get();
       retryCounter++;
       return response;
@@ -205,6 +301,60 @@ TEST(RetryPolicy, ShouldRetry)
     EXPECT_NE(responsePtrSent, nullptr);
     EXPECT_EQ(retryCounter, 0);
   }
+
+  // Overriding ShouldRetry to return true will mean we will retry, when the status code isn't
+  // retriable based on the options.
+  {
+    Azure::Core::Context context = Azure::Core::Context();
+    auto policy = RetryPolicyTest_CustomRetry_True(retryOptions);
+
+    RawResponse const* responsePtrSent = nullptr;
+    int32_t retryCounter = -1;
+
+    std::vector<std::unique_ptr<Azure::Core::Http::Policies::HttpPolicy>> policies;
+    policies.emplace_back(std::make_unique<RetryPolicyTest_CustomRetry_True>(policy));
+    policies.emplace_back(std::make_unique<TestTransportPolicy>([&]() {
+      auto response = std::make_unique<RawResponse>(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)
+  {
+    Azure::Core::Context context = Azure::Core::Context();
+    auto policy = RetryPolicyTest_CustomRetry_CopySource(RetryOptions());
+
+    RawResponse const* responsePtrSent = nullptr;
+    int32_t retryCounter = -1;
+
+    std::vector<std::unique_ptr<Azure::Core::Http::Policies::HttpPolicy>> policies;
+    policies.emplace_back(std::make_unique<RetryPolicyTest_CustomRetry_CopySource>(policy));
+    policies.emplace_back(std::make_unique<TestTransportPolicy>([&]() {
+      auto response = std::make_unique<RawResponse>(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);
+
+    Request request(HttpMethod::Get, Azure::Core::Url("https://www.microsoft.com"));
+    pipeline.Send(request, context);
+
+    EXPECT_NE(responsePtrSent, nullptr);
+    EXPECT_EQ(retryCounter, 3);
+  }
 }
 
 TEST(RetryPolicy, ShouldRetryOnResponse)