Skip to content
This repository was archived by the owner on Dec 8, 2021. It is now read-only.

Commit a399079

Browse files
coryancoryan
authored
bug: create new grpc::ClientContext for each RPC (#617)
gRPC requires a newly minted grpc::ClientContext for each RPC, and without assignment operators (no move, no copy) the only way is to actually create a new instance in the middle of the loop. Therefore passing a `grpc::ClientContext` has no effect, and we should not even have that argument in `RetryLoop()`.
1 parent 42aef87 commit a399079

File tree

6 files changed

+50
-54
lines changed

6 files changed

+50
-54
lines changed

‎google/cloud/spanner/internal/database_admin_retry.cc‎

Lines changed: 12 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -96,36 +96,36 @@ DatabaseAdminRetry::DatabaseAdminRetry(PrivateConstructorTag,
9696
namespace gcsa = google::spanner::admin::database::v1;
9797

9898
StatusOr<google::longrunning::Operation> DatabaseAdminRetry::CreateDatabase(
99-
grpc::ClientContext& context, gcsa::CreateDatabaseRequest const& request) {
99+
grpc::ClientContext&, gcsa::CreateDatabaseRequest const& request) {
100100
return RetryLoop(
101101
retry_policy_->clone(), backoff_policy_->clone(), false,
102102
[this](grpc::ClientContext& context,
103103
gcsa::CreateDatabaseRequest const& request) {
104104
return child_->CreateDatabase(context, request);
105105
},
106-
context, request, __func__);
106+
request, __func__);
107107
}
108108

109109
StatusOr<gcsa::Database> DatabaseAdminRetry::GetDatabase(
110-
grpc::ClientContext& context, gcsa::GetDatabaseRequest const& request) {
110+
grpc::ClientContext&, gcsa::GetDatabaseRequest const& request) {
111111
return RetryLoop(
112112
retry_policy_->clone(), backoff_policy_->clone(), true,
113113
[this](grpc::ClientContext& context,
114114
gcsa::GetDatabaseRequest const& request) {
115115
return child_->GetDatabase(context, request);
116116
},
117-
context, request, __func__);
117+
request, __func__);
118118
}
119119

120120
StatusOr<gcsa::GetDatabaseDdlResponse> DatabaseAdminRetry::GetDatabaseDdl(
121-
grpc::ClientContext& context, gcsa::GetDatabaseDdlRequest const& request) {
121+
grpc::ClientContext&, gcsa::GetDatabaseDdlRequest const& request) {
122122
return RetryLoop(
123123
retry_policy_->clone(), backoff_policy_->clone(), true,
124124
[this](grpc::ClientContext& context,
125125
gcsa::GetDatabaseDdlRequest const& request) {
126126
return child_->GetDatabaseDdl(context, request);
127127
},
128-
context, request, __func__);
128+
request, __func__);
129129
}
130130

131131
future<StatusOr<gcsa::Database>> DatabaseAdminRetry::AwaitCreateDatabase(
@@ -166,7 +166,7 @@ future<StatusOr<gcsa::Database>> DatabaseAdminRetry::AwaitCreateDatabase(
166166
}
167167

168168
StatusOr<google::longrunning::Operation> DatabaseAdminRetry::UpdateDatabase(
169-
grpc::ClientContext& context,
169+
grpc::ClientContext&,
170170
google::spanner::admin::database::v1::UpdateDatabaseDdlRequest const&
171171
request) {
172172
return RetryLoop(
@@ -175,7 +175,7 @@ StatusOr<google::longrunning::Operation> DatabaseAdminRetry::UpdateDatabase(
175175
gcsa::UpdateDatabaseDdlRequest const& request) {
176176
return child_->UpdateDatabase(context, request);
177177
},
178-
context, request, __func__);
178+
request, __func__);
179179
}
180180

181181
future<StatusOr<gcsa::UpdateDatabaseDdlMetadata>>
@@ -219,26 +219,26 @@ DatabaseAdminRetry::AwaitUpdateDatabase(
219219
}
220220

221221
Status DatabaseAdminRetry::DropDatabase(
222-
grpc::ClientContext& context,
222+
grpc::ClientContext&,
223223
google::spanner::admin::database::v1::DropDatabaseRequest const& request) {
224224
return RetryLoop(
225225
retry_policy_->clone(), backoff_policy_->clone(), true,
226226
[this](grpc::ClientContext& context,
227227
gcsa::DropDatabaseRequest const& request) {
228228
return child_->DropDatabase(context, request);
229229
},
230-
context, request, __func__);
230+
request, __func__);
231231
}
232232

233233
StatusOr<gcsa::ListDatabasesResponse> DatabaseAdminRetry::ListDatabases(
234-
grpc::ClientContext& context, gcsa::ListDatabasesRequest const& request) {
234+
grpc::ClientContext&, gcsa::ListDatabasesRequest const& request) {
235235
return RetryLoop(
236236
retry_policy_->clone(), backoff_policy_->clone(), true,
237237
[this](grpc::ClientContext& context,
238238
gcsa::ListDatabasesRequest const& request) {
239239
return child_->ListDatabases(context, request);
240240
},
241-
context, request, __func__);
241+
request, __func__);
242242
}
243243

244244
StatusOr<google::longrunning::Operation> DatabaseAdminRetry::GetOperation(

‎google/cloud/spanner/internal/database_admin_retry.h‎

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ inline namespace SPANNER_CLIENT_NS {
2727
namespace internal {
2828
/**
2929
* Implements the retry Decorator for DatabaseAdminStub.
30+
*
31+
* @note All retry decorators ignore the `grpc::ClientContext` parameter, and
32+
* create a new context for each child request.
3033
*/
3134
class DatabaseAdminRetry : public DatabaseAdminStub {
3235
public:
@@ -47,25 +50,25 @@ class DatabaseAdminRetry : public DatabaseAdminStub {
4750
*/
4851
///
4952
StatusOr<google::longrunning::Operation> CreateDatabase(
50-
grpc::ClientContext& context,
53+
grpc::ClientContext& /*unused*/,
5154
google::spanner::admin::database::v1::CreateDatabaseRequest const&
5255
request) override;
5356

5457
future<StatusOr<google::spanner::admin::database::v1::Database>>
5558
AwaitCreateDatabase(google::longrunning::Operation) override;
5659

5760
StatusOr<google::spanner::admin::database::v1::Database> GetDatabase(
58-
grpc::ClientContext&,
61+
grpc::ClientContext& /*unused*/,
5962
google::spanner::admin::database::v1::GetDatabaseRequest const&) override;
6063

6164
StatusOr<google::spanner::admin::database::v1::GetDatabaseDdlResponse>
6265
GetDatabaseDdl(
63-
grpc::ClientContext&,
66+
grpc::ClientContext& /*unused*/,
6467
google::spanner::admin::database::v1::GetDatabaseDdlRequest const&)
6568
override;
6669

6770
StatusOr<google::longrunning::Operation> UpdateDatabase(
68-
grpc::ClientContext& context,
71+
grpc::ClientContext& /*unused*/,
6972
google::spanner::admin::database::v1::UpdateDatabaseDdlRequest const&
7073
request) override;
7174

@@ -74,18 +77,18 @@ class DatabaseAdminRetry : public DatabaseAdminStub {
7477
AwaitUpdateDatabase(google::longrunning::Operation operation) override;
7578

7679
Status DropDatabase(
77-
grpc::ClientContext& context,
80+
grpc::ClientContext& /*unused*/,
7881
google::spanner::admin::database::v1::DropDatabaseRequest const& request)
7982
override;
8083

8184
StatusOr<google::spanner::admin::database::v1::ListDatabasesResponse>
8285
ListDatabases(
83-
grpc::ClientContext&,
86+
grpc::ClientContext& /*unused*/,
8487
google::spanner::admin::database::v1::ListDatabasesRequest const&)
8588
override;
8689

8790
StatusOr<google::longrunning::Operation> GetOperation(
88-
grpc::ClientContext& context,
91+
grpc::ClientContext& /*unused*/,
8992
google::longrunning::GetOperationRequest const& request) override;
9093
//@}
9194

‎google/cloud/spanner/internal/instance_admin_retry.cc‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -64,50 +64,48 @@ namespace gcsa = google::spanner::admin::instance::v1;
6464
namespace giam = google::iam::v1;
6565

6666
StatusOr<gcsa::Instance> InstanceAdminRetry::GetInstance(
67-
grpc::ClientContext& context, gcsa::GetInstanceRequest const& request) {
67+
grpc::ClientContext&, gcsa::GetInstanceRequest const& request) {
6868
return RetryLoop(
6969
retry_policy_->clone(), backoff_policy_->clone(), true,
7070
[this](grpc::ClientContext& context,
7171
gcsa::GetInstanceRequest const& request) {
7272
return child_->GetInstance(context, request);
7373
},
74-
context, request, __func__);
74+
request, __func__);
7575
}
7676

7777
StatusOr<gcsa::ListInstancesResponse> InstanceAdminRetry::ListInstances(
78-
grpc::ClientContext& context, gcsa::ListInstancesRequest const& request) {
78+
grpc::ClientContext&, gcsa::ListInstancesRequest const& request) {
7979
return RetryLoop(
8080
retry_policy_->clone(), backoff_policy_->clone(), true,
8181
[this](grpc::ClientContext& context,
8282
gcsa::ListInstancesRequest const& request) {
8383
return child_->ListInstances(context, request);
8484
},
85-
context, request, __func__);
85+
request, __func__);
8686
}
8787

8888
StatusOr<google::iam::v1::Policy> InstanceAdminRetry::GetIamPolicy(
89-
grpc::ClientContext& context,
90-
google::iam::v1::GetIamPolicyRequest const& request) {
89+
grpc::ClientContext&, google::iam::v1::GetIamPolicyRequest const& request) {
9190
return RetryLoop(
9291
retry_policy_->clone(), backoff_policy_->clone(), true,
9392
[this](grpc::ClientContext& context,
9493
giam::GetIamPolicyRequest const& request) {
9594
return child_->GetIamPolicy(context, request);
9695
},
97-
context, request, __func__);
96+
request, __func__);
9897
}
9998

10099
StatusOr<giam::TestIamPermissionsResponse>
101100
InstanceAdminRetry::TestIamPermissions(
102-
grpc::ClientContext& context,
103-
giam::TestIamPermissionsRequest const& request) {
101+
grpc::ClientContext&, giam::TestIamPermissionsRequest const& request) {
104102
return RetryLoop(
105103
retry_policy_->clone(), backoff_policy_->clone(), true,
106104
[this](grpc::ClientContext& context,
107105
giam::TestIamPermissionsRequest const& request) {
108106
return child_->TestIamPermissions(context, request);
109107
},
110-
context, request, __func__);
108+
request, __func__);
111109
}
112110

113111
} // namespace internal

‎google/cloud/spanner/internal/instance_admin_retry.h‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,9 @@ inline namespace SPANNER_CLIENT_NS {
2727
namespace internal {
2828
/**
2929
* Implements the retry Decorator for InstanceAdminStub.
30+
*
31+
* @note All retry decorators ignore the `grpc::ClientContext` parameter, and
32+
* create a new context for each child request.
3033
*/
3134
class InstanceAdminRetry : public InstanceAdminStub {
3235
public:
@@ -46,19 +49,19 @@ class InstanceAdminRetry : public InstanceAdminStub {
4649
* Run the retry loop (if appropriate) for the child InstanceAdminStub.
4750
*/
4851
StatusOr<google::spanner::admin::instance::v1::Instance> GetInstance(
49-
grpc::ClientContext&,
52+
grpc::ClientContext& /*unused*/,
5053
google::spanner::admin::instance::v1::GetInstanceRequest const&) override;
5154

5255
StatusOr<google::spanner::admin::instance::v1::ListInstancesResponse>
5356
ListInstances(
54-
grpc::ClientContext&,
57+
grpc::ClientContext& /*unused*/,
5558
google::spanner::admin::instance::v1::ListInstancesRequest const&)
5659
override;
5760
StatusOr<google::iam::v1::Policy> GetIamPolicy(
58-
grpc::ClientContext&,
61+
grpc::ClientContext& /*unused*/,
5962
google::iam::v1::GetIamPolicyRequest const&) override;
6063
StatusOr<google::iam::v1::TestIamPermissionsResponse> TestIamPermissions(
61-
grpc::ClientContext&,
64+
grpc::ClientContext& /*unused*/,
6265
google::iam::v1::TestIamPermissionsRequest const&) override;
6366
//@}
6467

‎google/cloud/spanner/internal/retry_loop.h‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -72,13 +72,14 @@ template <typename Functor, typename Request, typename Sleeper,
7272
auto RetryLoopImpl(std::unique_ptr<RetryPolicy> retry_policy,
7373
std::unique_ptr<BackoffPolicy> backoff_policy,
7474
bool is_idempotent, Functor&& functor,
75-
grpc::ClientContext& context, Request const& request,
76-
char const* location, Sleeper sleeper)
75+
Request const& request, char const* location,
76+
Sleeper sleeper)
7777
-> google::cloud::internal::invoke_result_t<Functor, grpc::ClientContext&,
7878
Request const&> {
7979
Status last_status;
80-
8180
while (!retry_policy->IsExhausted()) {
81+
// Need to create a new context for each retry.
82+
grpc::ClientContext context;
8283
auto result = functor(context, request);
8384
if (result.ok()) {
8485
return result;
@@ -112,14 +113,13 @@ template <typename Functor, typename Request,
112113
int>::type = 0>
113114
auto RetryLoop(std::unique_ptr<RetryPolicy> retry_policy,
114115
std::unique_ptr<BackoffPolicy> backoff_policy,
115-
bool is_idempotent, Functor&& functor,
116-
grpc::ClientContext& context, Request const& request,
116+
bool is_idempotent, Functor&& functor, Request const& request,
117117
char const* location)
118118
-> google::cloud::internal::invoke_result_t<Functor, grpc::ClientContext&,
119119
Request const&> {
120120
return RetryLoopImpl(
121121
std::move(retry_policy), std::move(backoff_policy), is_idempotent,
122-
std::forward<Functor>(functor), context, request, location,
122+
std::forward<Functor>(functor), request, location,
123123
[](std::chrono::milliseconds p) { std::this_thread::sleep_for(p); });
124124
}
125125

‎google/cloud/spanner/internal/retry_loop_test.cc‎

Lines changed: 7 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -38,19 +38,17 @@ std::unique_ptr<BackoffPolicy> TestBackoffPolicy() {
3838
}
3939

4040
TEST(RetryLoopTest, Success) {
41-
grpc::ClientContext context;
4241
StatusOr<int> actual = RetryLoop(
4342
TestRetryPolicy(), TestBackoffPolicy(), true,
4443
[](grpc::ClientContext&, int request) {
4544
return StatusOr<int>(2 * request);
4645
},
47-
context, 42, "error message");
46+
42, "error message");
4847
EXPECT_STATUS_OK(actual);
4948
EXPECT_EQ(84, *actual);
5049
}
5150

5251
TEST(RetryLoopTest, TransientThenSuccess) {
53-
grpc::ClientContext context;
5452
int counter = 0;
5553
StatusOr<int> actual = RetryLoop(
5654
TestRetryPolicy(), TestBackoffPolicy(), true,
@@ -60,13 +58,12 @@ TEST(RetryLoopTest, TransientThenSuccess) {
6058
}
6159
return StatusOr<int>(2 * request);
6260
},
63-
context, 42, "error message");
61+
42, "error message");
6462
EXPECT_STATUS_OK(actual);
6563
EXPECT_EQ(84, *actual);
6664
}
6765

6866
TEST(RetryLoopTest, ReturnJustStatus) {
69-
grpc::ClientContext context;
7067
int counter = 0;
7168
Status actual = RetryLoop(
7269
TestRetryPolicy(), TestBackoffPolicy(), true,
@@ -76,7 +73,7 @@ TEST(RetryLoopTest, ReturnJustStatus) {
7673
}
7774
return Status();
7875
},
79-
context, 42, "error message");
76+
42, "error message");
8077
EXPECT_STATUS_OK(actual);
8178
}
8279

@@ -100,7 +97,6 @@ TEST(RetryLoopTest, UsesBackoffPolicy) {
10097
.WillOnce(Return(ms(20)))
10198
.WillOnce(Return(ms(30)));
10299

103-
grpc::ClientContext context;
104100
int counter = 0;
105101
std::vector<ms> sleep_for;
106102
StatusOr<int> actual = RetryLoopImpl(
@@ -111,50 +107,46 @@ TEST(RetryLoopTest, UsesBackoffPolicy) {
111107
}
112108
return StatusOr<int>(2 * request);
113109
},
114-
context, 42, "error message",
115-
[&sleep_for](ms p) { sleep_for.push_back(p); });
110+
42, "error message", [&sleep_for](ms p) { sleep_for.push_back(p); });
116111
EXPECT_STATUS_OK(actual);
117112
EXPECT_EQ(84, *actual);
118113
EXPECT_THAT(sleep_for,
119114
ElementsAre(ms(10), std::chrono::milliseconds(20), ms(30)));
120115
}
121116

122117
TEST(RetryLoopTest, TransientFailureNonIdempotent) {
123-
grpc::ClientContext context;
124118
StatusOr<int> actual = RetryLoop(
125119
TestRetryPolicy(), TestBackoffPolicy(), false,
126120
[](grpc::ClientContext&, int) {
127121
return StatusOr<int>(Status(StatusCode::kUnavailable, "try again"));
128122
},
129-
context, 42, "the answer to everything");
123+
42, "the answer to everything");
130124
EXPECT_EQ(StatusCode::kUnavailable, actual.status().code());
131125
EXPECT_THAT(actual.status().message(), HasSubstr("try again"));
132126
EXPECT_THAT(actual.status().message(), HasSubstr("the answer to everything"));
133127
EXPECT_THAT(actual.status().message(), HasSubstr("Error in non-idempotent"));
134128
}
135129

136130
TEST(RetryLoopTest, PermanentFailureFailureIdempotent) {
137-
grpc::ClientContext context;
138131
StatusOr<int> actual = RetryLoop(
139132
TestRetryPolicy(), TestBackoffPolicy(), true,
140133
[](grpc::ClientContext&, int) {
141134
return StatusOr<int>(Status(StatusCode::kPermissionDenied, "uh oh"));
142135
},
143-
context, 42, "the answer to everything");
136+
42, "the answer to everything");
144137
EXPECT_EQ(StatusCode::kPermissionDenied, actual.status().code());
145138
EXPECT_THAT(actual.status().message(), HasSubstr("uh oh"));
146139
EXPECT_THAT(actual.status().message(), HasSubstr("the answer to everything"));
147140
EXPECT_THAT(actual.status().message(), HasSubstr("Permanent error"));
148141
}
149142

150143
TEST(RetryLoopTest, TooManyTransientFailuresIdempotent) {
151-
grpc::ClientContext context;
152144
StatusOr<int> actual = RetryLoop(
153145
TestRetryPolicy(), TestBackoffPolicy(), true,
154146
[](grpc::ClientContext&, int) {
155147
return StatusOr<int>(Status(StatusCode::kUnavailable, "try again"));
156148
},
157-
context, 42, "the answer to everything");
149+
42, "the answer to everything");
158150
EXPECT_EQ(StatusCode::kUnavailable, actual.status().code());
159151
EXPECT_THAT(actual.status().message(), HasSubstr("try again"));
160152
EXPECT_THAT(actual.status().message(), HasSubstr("the answer to everything"));

0 commit comments

Comments
 (0)