Skip to content

Commit b75a413

Browse files
Jinming-HuCopilot
andauthored
Core: refactor RetryPolicy into RetryPolicyBase + final RetryPolicy (#7196)
Introduces an abstract RetryPolicyBase that holds the retry loop and protected ShouldRetryOnResponse/ShouldRetryOnTransportFailure hook points without a Clone() implementation. Service SDKs (e.g. Storage) can derive from RetryPolicyBase to customize retry behavior and must provide their own Clone(), which prevents object slicing. RetryPolicy remains a final concrete policy for existing users. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 813c66a commit b75a413

5 files changed

Lines changed: 52 additions & 27 deletions

File tree

sdk/core/azure-core/CHANGELOG.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44

55
### Features Added
66

7+
- [[#7196]](https://github.com/Azure/azure-sdk-for-cpp/pull/7196) Added `Azure::Core::Http::Policies::_internal::RetryPolicyBase`, an abstract base class that exposes the retry loop and the protected `ShouldRetryOnResponse`/`ShouldRetryOnTransportFailure` hook points. Service SDKs can derive from `RetryPolicyBase` to customize retry behavior; `RetryPolicy` remains a `final` concrete policy.
8+
79
### Breaking Changes
810

911
### Bugs Fixed

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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -171,7 +171,7 @@ namespace Azure { namespace Core { namespace Http {
171171
}; // extensible enum HttpMethod
172172

173173
namespace Policies { namespace _internal {
174-
class RetryPolicy;
174+
class RetryPolicyBase;
175175
}} // namespace Policies::_internal
176176

177177
/**
@@ -181,7 +181,7 @@ namespace Azure { namespace Core { namespace Http {
181181
* resource, the URL of the resource, and the protocol version in use.
182182
*/
183183
class Request final {
184-
friend class Azure::Core::Http::Policies::_internal::RetryPolicy;
184+
friend class Azure::Core::Http::Policies::_internal::RetryPolicyBase;
185185
#if defined(_azure_TESTING_BUILD)
186186
// make tests classes friends to validate set Retry
187187
friend class Azure::Core::Test::TestHttp_getters_Test;

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

Lines changed: 31 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -388,28 +388,26 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
388388
};
389389

390390
/**
391-
* @brief HTTP retry policy.
391+
* @brief Base class for HTTP retry policies.
392+
*
393+
* @details Provides the retry loop implementation together with the protected hook points
394+
* (#ShouldRetryOnResponse, #ShouldRetryOnTransportFailure) that derived classes may override
395+
* to customize retry behavior. This class intentionally does not implement `Clone()`, which
396+
* keeps it abstract and forces every concrete derived policy to provide its own `Clone()`.
397+
* That prevents object slicing when a derived policy adds state of its own.
392398
*/
393-
class RetryPolicy
394-
#if !defined(_azure_TESTING_BUILD)
395-
final
396-
#endif
397-
: public HttpPolicy {
399+
class RetryPolicyBase : public HttpPolicy {
398400
private:
399401
RetryOptions m_retryOptions;
400402

401403
public:
402404
/**
403-
* Constructs HTTP retry policy with the provided #Azure::Core::Http::Policies::RetryOptions.
405+
* Constructs an HTTP retry policy base with the provided
406+
* #Azure::Core::Http::Policies::RetryOptions.
404407
*
405408
* @param options #Azure::Core::Http::Policies::RetryOptions.
406409
*/
407-
explicit RetryPolicy(RetryOptions options) : m_retryOptions(std::move(options)) {}
408-
409-
std::unique_ptr<HttpPolicy> Clone() const override
410-
{
411-
return std::make_unique<RetryPolicy>(*this);
412-
}
410+
explicit RetryPolicyBase(RetryOptions options) : m_retryOptions(std::move(options)) {}
413411

414412
std::unique_ptr<RawResponse> Send(
415413
Request& request,
@@ -420,8 +418,8 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
420418
* @brief Get the Retry Count from the context.
421419
*
422420
* @remark The sentinel `-1` is returned if there is no information in the \p Context about
423-
* #RetryPolicy is trying to send a request. Then `0` is returned for the first try of sending
424-
* a request by the #RetryPolicy. Any subsequent retry will be referenced with a number
421+
* #RetryPolicyBase is trying to send a request. Then `0` is returned for the first try of
422+
* sending a request by the policy. Any subsequent retry will be referenced with a number
425423
* greater than 0.
426424
*
427425
* @param context A context to control the request lifetime.
@@ -444,6 +442,24 @@ namespace Azure { namespace Core { namespace Http { namespace Policies {
444442
double jitterFactor = -1) const;
445443
};
446444

445+
/**
446+
* @brief HTTP retry policy.
447+
*/
448+
class RetryPolicy final : public RetryPolicyBase {
449+
public:
450+
/**
451+
* Constructs HTTP retry policy with the provided #Azure::Core::Http::Policies::RetryOptions.
452+
*
453+
* @param options #Azure::Core::Http::Policies::RetryOptions.
454+
*/
455+
explicit RetryPolicy(RetryOptions options) : RetryPolicyBase(std::move(options)) {}
456+
457+
std::unique_ptr<HttpPolicy> Clone() const override
458+
{
459+
return std::make_unique<RetryPolicy>(*this);
460+
}
461+
};
462+
447463
/**
448464
* @brief HTTP Request ID policy.
449465
*

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -103,7 +103,7 @@ bool WasLastAttempt(RetryOptions const& retryOptions, int32_t attempt)
103103
Context::Key const RetryKey;
104104
} // namespace
105105

106-
int32_t RetryPolicy::GetRetryCount(Context const& context)
106+
int32_t RetryPolicyBase::GetRetryCount(Context const& context)
107107
{
108108
int32_t number = -1;
109109

@@ -118,7 +118,7 @@ int32_t RetryPolicy::GetRetryCount(Context const& context)
118118
return *ptr;
119119
}
120120

121-
std::unique_ptr<RawResponse> RetryPolicy::Send(
121+
std::unique_ptr<RawResponse> RetryPolicyBase::Send(
122122
Request& request,
123123
NextHttpPolicy nextPolicy,
124124
Context const& context) const
@@ -189,7 +189,7 @@ std::unique_ptr<RawResponse> RetryPolicy::Send(
189189
}
190190
}
191191

192-
bool RetryPolicy::ShouldRetryOnTransportFailure(
192+
bool RetryPolicyBase::ShouldRetryOnTransportFailure(
193193
RetryOptions const& retryOptions,
194194
int32_t attempt,
195195
std::chrono::milliseconds& retryAfter,
@@ -205,7 +205,7 @@ bool RetryPolicy::ShouldRetryOnTransportFailure(
205205
return true;
206206
}
207207

208-
bool RetryPolicy::ShouldRetryOnResponse(
208+
bool RetryPolicyBase::ShouldRetryOnResponse(
209209
RawResponse const& response,
210210
RetryOptions const& retryOptions,
211211
int32_t attempt,

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

Lines changed: 13 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -35,7 +35,7 @@ class TestTransportPolicy final : public HttpPolicy {
3535
}
3636
};
3737

38-
class RetryPolicyTest final : public RetryPolicy {
38+
class RetryPolicyTest final : public RetryPolicyBase {
3939
private:
4040
std::function<bool(RetryOptions const&, int32_t, std::chrono::milliseconds&, double)>
4141
m_shouldRetryOnTransportFailure;
@@ -51,7 +51,7 @@ class RetryPolicyTest final : public RetryPolicy {
5151
std::chrono::milliseconds& retryAfter,
5252
double jitterFactor) const
5353
{
54-
return RetryPolicy::ShouldRetryOnTransportFailure(
54+
return RetryPolicyBase::ShouldRetryOnTransportFailure(
5555
retryOptions, attempt, retryAfter, jitterFactor);
5656
}
5757

@@ -62,15 +62,15 @@ class RetryPolicyTest final : public RetryPolicy {
6262
std::chrono::milliseconds& retryAfter,
6363
double jitterFactor) const
6464
{
65-
return RetryPolicy::ShouldRetryOnResponse(
65+
return RetryPolicyBase::ShouldRetryOnResponse(
6666
response, retryOptions, attempt, retryAfter, jitterFactor);
6767
}
6868

6969
RetryPolicyTest(
7070
RetryOptions const& retryOptions,
7171
decltype(m_shouldRetryOnTransportFailure) shouldRetryOnTransportFailure,
7272
decltype(m_shouldRetryOnResponse) shouldRetryOnResponse)
73-
: RetryPolicy(retryOptions),
73+
: RetryPolicyBase(retryOptions),
7474
m_shouldRetryOnTransportFailure(
7575
shouldRetryOnTransportFailure != nullptr //
7676
? shouldRetryOnTransportFailure
@@ -359,10 +359,17 @@ TEST(RetryPolicy, ShouldRetryOnTransportFailure)
359359
}
360360

361361
namespace {
362-
class RetryLogic final : private RetryPolicy {
363-
RetryLogic() : RetryPolicy(RetryOptions()) {}
362+
class RetryLogic final : private RetryPolicyBase {
363+
RetryLogic() : RetryPolicyBase(RetryOptions()) {}
364364
~RetryLogic() {}
365365

366+
std::unique_ptr<HttpPolicy> Clone() const override
367+
{
368+
// Not used; RetryLogic is only instantiated as a static helper for invoking the protected
369+
// hook methods.
370+
std::abort();
371+
}
372+
366373
static RetryLogic const g_retryPolicy;
367374

368375
public:

0 commit comments

Comments
 (0)