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

Revert commits related to the new RetryPolicy method (#5793)

Browse Source 
* Revert "Update the RetryPolicy for the GA release, keeping ShouldRetry extension point hidden. (#5771)"

This reverts commit 9ccd206ff8.

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

This reverts commit f1d95520d1.

* Do not remove changelog entry from a previous beta release

* Revert "Add a virtual ShouldRetry method to the RetryPolicy for customization. (#5584)"

This reverts commit ab90ef68b0.

---------

Co-authored-by: Anton Kolesnyk <antkmsft@users.noreply.github.com>
This commit is contained in:
Anton Kolesnyk 2024-07-12 12:03:43 -07:00 committed by GitHub
parent e8c7c559d9
commit 6b9e1cc691
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 131 additions and 417 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

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

@ -9,7 +9,6 @@
### Other Changes
- Updated JSON library to 3.11.3.
- Hide methods on the `RetryPolicy` that are not intended for public use.
- [[#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)_)
### Acknowledgments

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

@ -30,14 +30,6 @@
#include <utility>
#include <vector>
#if defined(_azure_TESTING_BUILD)
// Define the classes used from tests
namespace Azure { namespace Core { namespace Test {
class RetryPolicyTest;
class RetryLogic;
}}} // namespace Azure::Core::Test
#endif
/**
* A function that should be implemented and linked to the end-user application in order to override
* an HTTP transport implementation provided by Azure SDK with custom implementation.
@ -376,11 +368,6 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
final
#endif
: public HttpPolicy {
#if defined(_azure_TESTING_BUILD)
// make tests classes friends
friend class Azure::Core::Test::RetryPolicyTest;
friend class Azure::Core::Test::RetryLogic;
#endif
private:
RetryOptions m_retryOptions;
@ -415,7 +402,7 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
*/
static int32_t GetRetryCount(Context const& context);
private:
protected:
virtual bool ShouldRetryOnTransportFailure(
RetryOptions const& retryOptions,
int32_t attempt,
@ -428,26 +415,6 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor = -1) const;
/**
* @brief Overriding this method customizes the logic of when the RetryPolicy will re-attempt
* a request, based on the returned HTTP response.
*
* @remark A null response pointer means there was no response received from the corresponding
* 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
* non-retriable errors, including those specified in the RetryOptions, remain evaluated
* before calling ShouldRetry.
*
* @param response An HTTP response returned corresponding to the request sent by the policy.
* @param retryOptions The set of options provided to the RetryPolicy.
* @return Whether or not the HTTP request should be sent again through the pipeline.
*/
virtual bool ShouldRetry(
std::unique_ptr<RawResponse> const& response,
RetryOptions const& retryOptions) const;
};
/**

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

@ -118,11 +118,6 @@ int32_t RetryPolicy::GetRetryCount(Context const& context)
return *ptr;
}
bool RetryPolicy::ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) const
{
return false;
}
std::unique_ptr<RawResponse> RetryPolicy::Send(
Request& request,
NextHttpPolicy nextPolicy,
@ -145,24 +140,9 @@ 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))
{
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))
// 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))
{
// 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
@ -235,6 +215,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;

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

@ -13,356 +13,118 @@ using namespace Azure::Core::Http;
using namespace Azure::Core::Http::Policies;
using namespace Azure::Core::Http::Policies::_internal;
namespace Azure { namespace Core { namespace Test {
class TestTransportPolicy final : public HttpPolicy {
private:
std::function<std::unique_ptr<RawResponse>()> m_send;
namespace {
class TestTransportPolicy final : public HttpPolicy {
private:
std::function<std::unique_ptr<RawResponse>()> m_send;
public:
TestTransportPolicy(std::function<std::unique_ptr<RawResponse>()> send) : m_send(send) {}
public:
TestTransportPolicy(std::function<std::unique_ptr<RawResponse>()> send) : m_send(send) {}
std::unique_ptr<Azure::Core::Http::RawResponse> Send(
Request&,
NextHttpPolicy,
Azure::Core::Context const&) const override
{
return m_send();
}
std::unique_ptr<HttpPolicy> Clone() const override
{
return std::make_unique<TestTransportPolicy>(*this);
}
};
class RetryPolicyTest final : public RetryPolicy {
private:
std::function<bool(RetryOptions const&, int32_t, std::chrono::milliseconds&, double)>
m_shouldRetryOnTransportFailure;
std::function<
bool(RawResponse const&, RetryOptions const&, int32_t, std::chrono::milliseconds&, double)>
m_shouldRetryOnResponse;
public:
bool BaseShouldRetryOnTransportFailure(
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor) const
{
return RetryPolicy::ShouldRetryOnTransportFailure(
retryOptions, attempt, retryAfter, jitterFactor);
}
bool BaseShouldRetryOnResponse(
RawResponse const& response,
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor) const
{
return RetryPolicy::ShouldRetryOnResponse(
response, retryOptions, attempt, retryAfter, jitterFactor);
}
RetryPolicyTest(
RetryOptions const& retryOptions,
decltype(m_shouldRetryOnTransportFailure) shouldRetryOnTransportFailure,
decltype(m_shouldRetryOnResponse) shouldRetryOnResponse)
: RetryPolicy(retryOptions),
m_shouldRetryOnTransportFailure(
shouldRetryOnTransportFailure != nullptr //
? shouldRetryOnTransportFailure
: static_cast<decltype(m_shouldRetryOnTransportFailure)>( //
[this](auto options, auto attempt, auto retryAfter, auto jitter) {
retryAfter = std::chrono::milliseconds(0);
auto ignore = decltype(retryAfter)();
return this->BaseShouldRetryOnTransportFailure(
options, attempt, ignore, jitter);
})),
m_shouldRetryOnResponse(
shouldRetryOnResponse != nullptr //
? shouldRetryOnResponse
: static_cast<decltype(m_shouldRetryOnResponse)>( //
[this](
RawResponse const& response,
auto options,
auto attempt,
auto retryAfter,
auto jitter) {
retryAfter = std::chrono::milliseconds(0);
auto ignore = decltype(retryAfter)();
return this->BaseShouldRetryOnResponse(
response, options, attempt, ignore, jitter);
}))
{
}
std::unique_ptr<HttpPolicy> Clone() const override
{
return std::make_unique<RetryPolicyTest>(*this);
}
protected:
bool ShouldRetryOnTransportFailure(
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor) const override
{
return m_shouldRetryOnTransportFailure(retryOptions, attempt, retryAfter, jitterFactor);
}
bool ShouldRetryOnResponse(
RawResponse const& response,
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor) const override
{
return m_shouldRetryOnResponse(response, retryOptions, attempt, retryAfter, jitterFactor);
}
};
class RetryPolicyTest_CustomRetry_True final : public RetryPolicy {
public:
RetryPolicyTest_CustomRetry_True(RetryOptions const& retryOptions) : RetryPolicy(retryOptions)
{
}
std::unique_ptr<HttpPolicy> Clone() const override
{
return std::make_unique<RetryPolicyTest_CustomRetry_True>(*this);
}
protected:
bool ShouldRetry(std::unique_ptr<RawResponse> const&, RetryOptions const&) 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;
}
}
}
return false;
}
};
}}} // namespace Azure::Core::Test
using namespace Azure::Core::Test;
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.
std::unique_ptr<Azure::Core::Http::RawResponse> Send(
Request&,
NextHttpPolicy,
Azure::Core::Context const&) const override
{
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::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);
return m_send();
}
// If the status code is retriable based on the options, ShouldRetry doesn't get called.
// Testing short-circuit evaluation
std::unique_ptr<HttpPolicy> Clone() const override
{
Azure::Core::Context context = Azure::Core::Context();
auto policy = RetryPolicyTest_CustomRetry_Throw(retryOptions);
return std::make_unique<TestTransportPolicy>(*this);
}
};
RawResponse const* responsePtrSent = nullptr;
int32_t retryCounter = -1;
class RetryPolicyTest final : public RetryPolicy {
private:
std::function<bool(RetryOptions const&, int32_t, std::chrono::milliseconds&, double)>
m_shouldRetryOnTransportFailure;
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;
}));
std::function<
bool(RawResponse const&, RetryOptions const&, int32_t, std::chrono::milliseconds&, double)>
m_shouldRetryOnResponse;
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);
public:
bool BaseShouldRetryOnTransportFailure(
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor) const
{
return RetryPolicy::ShouldRetryOnTransportFailure(
retryOptions, attempt, retryAfter, jitterFactor);
}
// 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.
bool BaseShouldRetryOnResponse(
RawResponse const& response,
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor) const
{
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;
}));
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);
return RetryPolicy::ShouldRetryOnResponse(
response, retryOptions, attempt, retryAfter, jitterFactor);
}
// Overriding ShouldRetry to return true will mean we will retry, when the status code isn't
// retriable based on the options.
RetryPolicyTest(
RetryOptions const& retryOptions,
decltype(m_shouldRetryOnTransportFailure) shouldRetryOnTransportFailure,
decltype(m_shouldRetryOnResponse) shouldRetryOnResponse)
: RetryPolicy(retryOptions),
m_shouldRetryOnTransportFailure(
shouldRetryOnTransportFailure != nullptr //
? shouldRetryOnTransportFailure
: static_cast<decltype(m_shouldRetryOnTransportFailure)>( //
[this](auto options, auto attempt, auto retryAfter, auto jitter) {
retryAfter = std::chrono::milliseconds(0);
auto ignore = decltype(retryAfter)();
return this->BaseShouldRetryOnTransportFailure(
options, attempt, ignore, jitter);
})),
m_shouldRetryOnResponse(
shouldRetryOnResponse != nullptr //
? shouldRetryOnResponse
: static_cast<decltype(m_shouldRetryOnResponse)>( //
[this](
RawResponse const& response,
auto options,
auto attempt,
auto retryAfter,
auto jitter) {
retryAfter = std::chrono::milliseconds(0);
auto ignore = decltype(retryAfter)();
return this->BaseShouldRetryOnResponse(
response, options, attempt, ignore, jitter);
}))
{
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)
std::unique_ptr<HttpPolicy> Clone() const override
{
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);
return std::make_unique<RetryPolicyTest>(*this);
}
}
protected:
bool ShouldRetryOnTransportFailure(
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor) const override
{
return m_shouldRetryOnTransportFailure(retryOptions, attempt, retryAfter, jitterFactor);
}
bool ShouldRetryOnResponse(
RawResponse const& response,
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor) const override
{
return m_shouldRetryOnResponse(response, retryOptions, attempt, retryAfter, jitterFactor);
}
};
} // namespace
TEST(RetryPolicy, ShouldRetryOnResponse)
{
@ -596,38 +358,38 @@ TEST(RetryPolicy, ShouldRetryOnTransportFailure)
EXPECT_EQ(jitterReceived, -1);
}
namespace Azure { namespace Core { namespace Test {
class RetryLogic final : private RetryPolicy {
RetryLogic() : RetryPolicy(RetryOptions()) {}
~RetryLogic() {}
namespace {
class RetryLogic final : private RetryPolicy {
RetryLogic() : RetryPolicy(RetryOptions()) {}
~RetryLogic() {}
static RetryLogic const g_retryPolicy;
static RetryLogic const g_retryPolicy;
public:
static bool TestShouldRetryOnTransportFailure(
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor)
{
return g_retryPolicy.ShouldRetryOnTransportFailure(
retryOptions, attempt, retryAfter, jitterFactor);
}
public:
static bool TestShouldRetryOnTransportFailure(
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor)
{
return g_retryPolicy.ShouldRetryOnTransportFailure(
retryOptions, attempt, retryAfter, jitterFactor);
}
static bool TestShouldRetryOnResponse(
RawResponse const& response,
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor)
{
return g_retryPolicy.ShouldRetryOnResponse(
response, retryOptions, attempt, retryAfter, jitterFactor);
}
};
static bool TestShouldRetryOnResponse(
RawResponse const& response,
RetryOptions const& retryOptions,
int32_t attempt,
std::chrono::milliseconds& retryAfter,
double jitterFactor)
{
return g_retryPolicy.ShouldRetryOnResponse(
response, retryOptions, attempt, retryAfter, jitterFactor);
}
};
RetryLogic const RetryLogic::g_retryPolicy;
}}} // namespace Azure::Core::Test
RetryLogic const RetryLogic::g_retryPolicy;
} // namespace
TEST(RetryPolicy, Exponential)
{