Revert "Update the RetryPolicy and ShouldRetry customization logic to allow loosening the retry condition. (Azure#5656)"

This reverts commit f1d9552.

Author: antkmsft

sdk/core/azure-core/CHANGELOG.md

@@ -27,7 +27,6 @@ Thank you to our developer community members who helped to make Azure Core bette
 ### 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 `false`. The
+       * @remark Unless overriden, the default implementation is to always return true. 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 false;
+  return true;
 }
 
 std::unique_ptr<RawResponse> RetryPolicy::Send(
@@ -145,27 +145,19 @@ std::unique_ptr<RawResponse> 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))
+      // 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))
       {
+        // 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;
       }
 
-      // 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))
+      // 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 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;
       }
     }
@@ -235,6 +227,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;

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

@@ -125,91 +125,21 @@ class RetryPolicyTest final : public RetryPolicy {
   }
 };
 
-class RetryPolicyTest_CustomRetry_True final : public RetryPolicy {
+class RetryPolicyTest_CustomRetry_False final : public RetryPolicy {
 public:
-  RetryPolicyTest_CustomRetry_True(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {}
+  RetryPolicyTest_CustomRetry_False(RetryOptions const& retryOptions) : RetryPolicy(retryOptions) {}
 
   std::unique_ptr<HttpPolicy> Clone() const override
   {
-    return std::make_unique<RetryPolicyTest_CustomRetry_True>(*this);
+    return std::make_unique<RetryPolicyTest_CustomRetry_False>(*this);
   }
 
 protected:
-  bool ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) const override
+  bool ShouldRetry(std::unique_ptr<RawResponse> const& response, RetryOptions const& retryOptions)
+      const override
   {
-    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;
-        }
-      }
-    }
+    (void)response;
+    (void)retryOptions;
     return false;
   }
 };
@@ -220,11 +150,10 @@ 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.
+  // The default ShouldRetry implementation is to always return true,
+  // which means we will retry up to the retry count in the options.
   {
-    Azure::Core::Context context = Azure::Core::Context();
+    Azure::Core::Context context = Azure::Core::Context::ApplicationContext;
     auto policy = RetryPolicy(retryOptions);
 
     RawResponse const* responsePtrSent = nullptr;
@@ -234,33 +163,7 @@ 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;
-    }));
-
-    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 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_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_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;
@@ -275,73 +178,20 @@ TEST(RetryPolicy, ShouldRetry)
     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.
+  // Overriding ShouldRetry to return false will mean we won't retry.
   {
     Azure::Core::Context context = Azure::Core::Context();
-    auto policy = RetryPolicy(retryOptions);
+    auto policy = RetryPolicyTest_CustomRetry_False(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;
-    }));
-
-    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);
-  }
-
-  // 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;
+    policies.emplace_back(std::make_unique<RetryPolicyTest_CustomRetry_False>(policy));
 
-    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;
+      auto response = std::make_unique<RawResponse>(1, 1, HttpStatusCode::Ok, "Test");
 
-    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;
@@ -353,7 +203,7 @@ TEST(RetryPolicy, ShouldRetry)
     pipeline.Send(request, context);
 
     EXPECT_NE(responsePtrSent, nullptr);
-    EXPECT_EQ(retryCounter, 3);
+    EXPECT_EQ(retryCounter, 0);
   }
 }