feat: gracefully fail adding to the CompletionQueue after Shutdown
#138
Conversation
googlebot
added
the
cla: yes
|
@coryan I made this a draft because I'm not sure it's the right approach, or if I'm doing the proper thing in |
| @@ -54,8 +54,6 @@ class AsyncGrpcOperation : public AsyncOperation { | |||
| * Derived classes wrap the callbacks provided by the application and invoke | |||
| * the callback when this virtual member function is called. | |||
| * | |||
| * @param cq the completion queue sending the notification, this is useful in | |||
| * case the callback needs to retry the operation. | |||
There was a problem hiding this comment.
missed removing this in a prior PR
Codecov Report
@@ Coverage Diff @@
## master #138 +/- ##
==========================================
- Coverage 95.81% 95.66% -0.15%
==========================================
Files 83 83
Lines 3847 3900 +53
==========================================
+ Hits 3686 3731 +45
- Misses 161 169 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1.
Reviewable status:complete! all files reviewed, all discussions resolved
|
google/cloud/grpc_utils/internal/completion_queue_impl.h, line 105 at r1 (raw file): Quoted 7 lines of code…
@coryan I did think about this - I think this can this be called now with I'll also see if I can write some tests to exercise this |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mr-salty)
google/cloud/grpc_utils/internal/completion_queue_impl.h, line 105 at r1 (raw file):
Previously, mr-salty (Todd Derr) wrote…
if (!ok) { // This would mean a bug in gRPC. The documentation states that Finish() // always returns `true` for unary RPCs. promise_.set_value(::google::cloud::Status( google::cloud::StatusCode::kUnknown, "Finish() returned false")); return true; }@coryan I did think about this - I think this can this be called now with
ok == false, so maybe I should change the comment (and also change the status tokCancelled? WDYT?I'll also see if I can write some tests to exercise this
Removing the comment and changing the code to return kCancelled sounds like a good idea. Sorry I missed it.
For what it is worth, writing unit tests for this is a good idea, you need to mock "enough" of a gRPC-generated class to make it work though, it is sort of annoying, there is one such test in google-cloud-cpp (async_retry_unary_rpc_test.cc) that might be a good start.
The grpc `CompletionQueue` asserts if items are added to it after `Shutdown()` is called. Make our `CompletionQueue` a little friendlier by just failing those operations immediately. Part of #129
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mr-salty)
There was a problem hiding this comment.
Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @coryan)
google/cloud/grpc_utils/internal/completion_queue_impl.h, line 105 at r1 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
Removing the comment and changing the code to return
kCancelledsounds like a good idea. Sorry I missed it.For what it is worth, writing unit tests for this is a good idea, you need to mock "enough" of a gRPC-generated class to make it work though, it is sort of annoying, there is one such test in
google-cloud-cpp(async_retry_unary_rpc_test.cc) that might be a good start.
I saw you added tests (thanks!) so I took what you did and added a new test, which uncovered a couple (or 3?) actual code bugs - PTAL
There was a problem hiding this comment.
Reviewed 4 of 4 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mr-salty)
google/cloud/grpc_utils/internal/async_read_stream_impl.h, line 161 at r3 (raw file):
void* tag = cq_->RegisterOperation(std::move(callback)); if (tag != nullptr) { reader_ = async_call(context_.get(), request, &cq_->cq());
FYI: I think this is correct, but I had to read it like 3 times to convince myself there are no race conditions.
|
FWIW, the Windows failure is the flakiness I fixed in #143 . I restarted the build, but rebasing might help too. |
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @mr-salty)
google/cloud/grpc_utils/internal/completion_queue_impl.h, line 105 at r1 (raw file):
Previously, mr-salty (Todd Derr) wrote…
I saw you added tests (thanks!) so I took what you did and added a new test, which uncovered a couple (or 3?) actual code bugs - PTAL
LGTM.
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @coryan)
google/cloud/grpc_utils/internal/async_read_stream_impl.h, line 161 at r3 (raw file):
Previously, coryan (Carlos O'Ryan) wrote…
FYI: I think this is correct, but I had to read it like 3 times to convince myself there are no race conditions.
Yeah, I had to convince myself a bit too :) I added a comment about it, thanks.
googleapis/google-cloud-cpp-common#138) * feat: gracefully fail adding to the `CompletionQueue` after `Shutdown` The grpc `CompletionQueue` asserts if items are added to it after `Shutdown()` is called. Make our `CompletionQueue` a little friendlier by just failing those operations immediately. Part of googleapis/google-cloud-cpp-common#129 * Add a test for making RPCs after `Shutdown()`, which uncovered a couple bugs * Add a comment about the safety of leaving reader_ null in `Start`.
googleapis/google-cloud-cpp-common#138) * feat: gracefully fail adding to the `CompletionQueue` after `Shutdown` The grpc `CompletionQueue` asserts if items are added to it after `Shutdown()` is called. Make our `CompletionQueue` a little friendlier by just failing those operations immediately. Part of googleapis/google-cloud-cpp-common#129 * Add a test for making RPCs after `Shutdown()`, which uncovered a couple bugs * Add a comment about the safety of leaving reader_ null in `Start`.
googleapis/google-cloud-cpp-common#138) * feat: gracefully fail adding to the `CompletionQueue` after `Shutdown` The grpc `CompletionQueue` asserts if items are added to it after `Shutdown()` is called. Make our `CompletionQueue` a little friendlier by just failing those operations immediately. Part of googleapis/google-cloud-cpp-common#129 * Add a test for making RPCs after `Shutdown()`, which uncovered a couple bugs * Add a comment about the safety of leaving reader_ null in `Start`.
The grpc
CompletionQueueasserts if items are added to it afterShutdown()is called. Make ourCompletionQueuea little friendlierby just failing those operations immediately.
Part of #129
This change is