Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(common): handle expired policies in *RetryLoop #12301

Merged
merged 2 commits into from
Aug 3, 2023
Merged

fix(common): handle expired policies in *RetryLoop #12301

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
592a442
fix(common): handle expired policies in `*RetryLoop`
coryan Aug 2, 2023
2f28f7d
Address review comments
coryan Aug 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 39 additions & 25 deletions google/cloud/internal/rest_retry_loop.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "google/cloud/backoff_policy.h"
#include "google/cloud/idempotency.h"
#include "google/cloud/internal/invoke_result.h"
#include "google/cloud/internal/make_status.h"
#include "google/cloud/internal/opentelemetry.h"
#include "google/cloud/internal/rest_request.h"
#include "google/cloud/internal/retry_loop_helpers.h"
Expand Down Expand Up @@ -61,41 +62,35 @@ template <
typename std::enable_if<google::cloud::internal::is_invocable<
Functor, RestContext&, Request const&>::value,
int>::type = 0>
auto RestRetryLoopImpl(std::unique_ptr<RetryPolicy> retry_policy,
std::unique_ptr<BackoffPolicy> backoff_policy,
auto RestRetryLoopImpl(RetryPolicy& retry_policy, BackoffPolicy& backoff_policy,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are going to make this change, we should do it in RetryLoopImpl too.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Idempotency idempotency, Functor&& functor,
Request const& request, char const* location,
Sleeper sleeper)
-> google::cloud::internal::invoke_result_t<Functor, RestContext&,
Request const&> {
Status last_status;
while (!retry_policy->IsExhausted()) {
auto last_status = internal::DeadlineExceededError(
"Retry policy exhausted before first request attempt", GCP_ERROR_INFO());
while (!retry_policy.IsExhausted()) {
RestContext rest_context;
auto result = functor(rest_context, request);
if (result.ok()) {
return result;
}
if (result.ok()) return result;
last_status = internal::GetResultStatus(std::move(result));
if (idempotency == Idempotency::kNonIdempotent) {
return internal::RetryLoopError("Error in non-idempotent operation",
location, last_status);
}
if (!retry_policy->OnFailure(last_status)) {
// The retry policy is exhausted or the error is not retryable, either
// way, exit the loop.
break;
}
sleeper(backoff_policy->OnCompletion());
}
if (!retry_policy->IsExhausted()) {
// The last error cannot be retried, but it is not because the retry
// policy is exhausted, we call these "permanent errors", and they
// get a special message.
return internal::RetryLoopError("Permanent error in", location,
last_status);
// The retry policy is exhausted or the error is not retryable. Either
// way, exit the loop.
if (!retry_policy.OnFailure(last_status)) break;
sleeper(backoff_policy.OnCompletion());
}
return internal::RetryLoopError("Retry policy exhausted in", location,
last_status);
// The last error cannot be retried, but it is not because the retry
// policy is exhausted. We call these "permanent errors", and they
// get a special message.
char const* prefix = !retry_policy.IsExhausted()
? "Permanent error in"
: "Retry policy exhausted in";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// The last error cannot be retried, but it is not because the retry
// policy is exhausted. We call these "permanent errors", and they
// get a special message.
char const* prefix = !retry_policy.IsExhausted()
? "Permanent error in"
: "Retry policy exhausted in";
// Distinguish between a permanent error and too many transient errors.
char const* prefix = retry_policy.IsExhausted()
? "Retry policy exhausted in"
: "Permanent error in";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'd drop the negation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit nit: It looks like RetryLoopError() could well handle the " in " part itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove the negation(s). I am going to leave the in for a future PR.

return internal::RetryLoopError(prefix, location, last_status);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

observation: in the (rare) exhausted before start case, the error message is a bit redundant:

Retry policy exhausted in <location>:  Retry policy exhausted before first request attempt

I am fine with this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I will open a separate thread to shave this baby yak cleanup the error messages from retry loops.

}

/// @copydoc RestRetryLoopImpl
Expand All @@ -113,9 +108,28 @@ auto RestRetryLoop(std::unique_ptr<RetryPolicy> retry_policy,
std::function<void(std::chrono::milliseconds)> sleeper =
[](std::chrono::milliseconds p) { std::this_thread::sleep_for(p); };
sleeper = internal::MakeTracedSleeper(std::move(sleeper));
return RestRetryLoopImpl(std::move(retry_policy), std::move(backoff_policy),
idempotency, std::forward<Functor>(functor), request,
location, std::move(sleeper));
return RestRetryLoopImpl(*retry_policy, *backoff_policy, idempotency,
std::forward<Functor>(functor), request, location,
std::move(sleeper));
}

/// @copydoc RestRetryLoopImpl
template <
typename Functor, typename Request,
typename std::enable_if<google::cloud::internal::is_invocable<
Functor, RestContext&, Request const&>::value,
int>::type = 0>
auto RestRetryLoop(RetryPolicy& retry_policy, BackoffPolicy& backoff_policy,
Idempotency idempotency, Functor&& functor,
Request const& request, char const* location)
-> google::cloud::internal::invoke_result_t<Functor, RestContext&,
Request const&> {
std::function<void(std::chrono::milliseconds)> sleeper =
[](std::chrono::milliseconds p) { std::this_thread::sleep_for(p); };
sleeper = internal::MakeTracedSleeper(std::move(sleeper));
return RestRetryLoopImpl(retry_policy, backoff_policy, idempotency,
std::forward<Functor>(functor), request, location,
std::move(sleeper));
}

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
Expand Down
22 changes: 21 additions & 1 deletion google/cloud/internal/rest_retry_loop_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ namespace {

using ::google::cloud::Idempotency;
using ::google::cloud::testing_util::MockBackoffPolicy;
using ::google::cloud::testing_util::StatusIs;
using ::testing::ElementsAre;
using ::testing::HasSubstr;
using ::testing::Return;
Expand Down Expand Up @@ -129,8 +130,9 @@ TEST(RestRetryLoopTest, UsesBackoffPolicy) {
int counter = 0;
std::vector<ms> sleep_for;
internal::OptionsSpan span(Options{}.set<StringOption>("UsesBackoffPolicy"));
auto retry_policy = TestRetryPolicy();
StatusOr<int> actual = RestRetryLoopImpl(
TestRetryPolicy(), std::move(mock), Idempotency::kIdempotent,
*retry_policy, *mock, Idempotency::kIdempotent,
[&counter](RestContext&, int request) {
EXPECT_EQ(internal::CurrentOptions().get<StringOption>(),
"UsesBackoffPolicy");
Expand Down Expand Up @@ -201,6 +203,24 @@ TEST(RestRetryLoopTest, TooManyTransientFailuresIdempotent) {
EXPECT_THAT(actual.status().message(), HasSubstr("Retry policy exhausted"));
}

TEST(RestRetryLoopTest, ExhaustedOnStart) {
internal::OptionsSpan span(Options{}.set<StringOption>("ExhaustedOnStart"));
auto retry_policy = internal::LimitedTimeRetryPolicy<TestRetryablePolicy>(
std::chrono::seconds(0));
ASSERT_TRUE(retry_policy.IsExhausted());
StatusOr<int> actual = RestRetryLoop(
retry_policy.clone(), TestBackoffPolicy(), Idempotency::kIdempotent,
[](RestContext&, int) {
EXPECT_EQ(internal::CurrentOptions().get<StringOption>(),
"ExhaustedOnStart");
return StatusOr<int>(Status(StatusCode::kUnavailable, "try again"));
},
42, "the answer to everything");
internal::OptionsSpan overlay(Options{}.set<StringOption>("uh-oh"));
EXPECT_THAT(actual, StatusIs(StatusCode::kDeadlineExceeded,
HasSubstr("Retry policy exhausted before")));
}

#ifdef GOOGLE_CLOUD_CPP_HAVE_OPENTELEMETRY
using ::google::cloud::testing_util::DisableTracing;
using ::google::cloud::testing_util::EnableTracing;
Expand Down
26 changes: 13 additions & 13 deletions google/cloud/internal/retry_loop.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "google/cloud/grpc_options.h"
#include "google/cloud/idempotency.h"
#include "google/cloud/internal/invoke_result.h"
#include "google/cloud/internal/make_status.h"
#include "google/cloud/internal/opentelemetry.h"
#include "google/cloud/internal/retry_loop_helpers.h"
#include "google/cloud/retry_policy.h"
Expand Down Expand Up @@ -68,7 +69,8 @@ auto RetryLoopImpl(std::unique_ptr<RetryPolicy> retry_policy,
Sleeper sleeper)
-> google::cloud::internal::invoke_result_t<Functor, grpc::ClientContext&,
Request const&> {
Status last_status;
auto last_status = internal::DeadlineExceededError(
"Retry policy exhausted before first request attempt", GCP_ERROR_INFO());
while (!retry_policy->IsExhausted()) {
// Need to create a new context for each retry.
grpc::ClientContext context;
Expand All @@ -81,20 +83,18 @@ auto RetryLoopImpl(std::unique_ptr<RetryPolicy> retry_policy,
return RetryLoopError("Error in non-idempotent operation", location,
last_status);
}
if (!retry_policy->OnFailure(last_status)) {
// The retry policy is exhausted or the error is not retryable, either
// way, exit the loop.
break;
}
// The retry policy is exhausted or the error is not retryable. Either
// way, exit the loop.
if (!retry_policy->OnFailure(last_status)) break;
sleeper(backoff_policy->OnCompletion());
}
if (!retry_policy->IsExhausted()) {
// The last error cannot be retried, but it is not because the retry
// policy is exhausted, we call these "permanent errors", and they
// get a special message.
return RetryLoopError("Permanent error in", location, last_status);
}
return RetryLoopError("Retry policy exhausted in", location, last_status);
// The last error cannot be retried, but it is not because the retry
// policy is exhausted. We call these "permanent errors", and they
// get a special message.
char const* prefix = !retry_policy->IsExhausted()
? "Permanent error in"
: "Retry policy exhausted in";
return internal::RetryLoopError(prefix, location, last_status);
}

/// @copydoc RetryLoopImpl
Expand Down
19 changes: 19 additions & 0 deletions google/cloud/internal/retry_loop_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ namespace internal {
namespace {

using ::google::cloud::testing_util::MockBackoffPolicy;
using ::google::cloud::testing_util::StatusIs;
using ::testing::ElementsAre;
using ::testing::HasSubstr;
using ::testing::Return;
Expand Down Expand Up @@ -193,6 +194,24 @@ TEST(RetryLoopTest, TooManyTransientFailuresIdempotent) {
EXPECT_THAT(actual.status().message(), HasSubstr("Retry policy exhausted"));
}

TEST(RetryLoopTest, ExhaustedOnStart) {
internal::OptionsSpan span(Options{}.set<StringOption>("ExhaustedOnStart"));
auto retry_policy = internal::LimitedTimeRetryPolicy<TestRetryablePolicy>(
std::chrono::seconds(0));
ASSERT_TRUE(retry_policy.IsExhausted());
StatusOr<int> actual = RetryLoop(
retry_policy.clone(), TestBackoffPolicy(), Idempotency::kIdempotent,
[](grpc::ClientContext&, int) {
EXPECT_EQ(internal::CurrentOptions().get<StringOption>(),
"ExhaustedOnStart");
return StatusOr<int>(Status(StatusCode::kUnavailable, "try again"));
},
42, "the answer to everything");
internal::OptionsSpan overlay(Options{}.set<StringOption>("uh-oh"));
EXPECT_THAT(actual, StatusIs(StatusCode::kDeadlineExceeded,
HasSubstr("Retry policy exhausted before")));
}

TEST(RetryLoopTest, ConfigureContext) {
auto setup = [](grpc::ClientContext& context) {
context.set_compression_algorithm(GRPC_COMPRESS_DEFLATE);
Expand Down