github/azure-sdk-for-cpp
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

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

Browse Source 
* 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.
This commit is contained in:
Ahson Khan 2024-05-29 20:08:56 -07:00 committed by GitHub
parent b7a751dd53
commit f1d95520d1
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 184 additions and 31 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
sdk/core/azure-core/CHANGELOG.md
View File

@ -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

2
sdk/core/azure-core/inc/azure/core/http/policies/policy.hpp
View File

@ -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.
*

32
sdk/core/azure-core/src/http/retry_policy.cpp
View File

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

180
sdk/core/azure-core/test/ut/retry_policy_test.cpp
View File

@ -125,21 +125,91 @@ protected:
}
};
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)