Bail out GoogleAsyncStreamImpl::writeQueued if cq is drained #4356
Conversation
Signed-off-by: Wayne Zhang <qiwzhang@google.com>
|
@htuch Please help to review this. |
| @@ -205,7 +205,8 @@ void GoogleAsyncStreamImpl::resetStream() { | |||
| } | |||
|
|
|||
| void GoogleAsyncStreamImpl::writeQueued() { | |||
| if (!call_initialized_ || finish_pending_ || write_pending_ || write_pending_queue_.empty()) { | |||
| if (!call_initialized_ || finish_pending_ || write_pending_ || write_pending_queue_.empty() || | |||
There was a problem hiding this comment.
This seems a legitimate change; we shouldn't be queueing new writes if we're draining for sure. Does this solve your teardown race for the Envoy gRPC client as well?
Trying to recall our earlier conversation, it seems we should get a clear teardown ordering. We should ensure that all the gRPC consumers are properly removed before we begin the TLS teardown.
There was a problem hiding this comment.
Yes, it solves the test flakiness. tested with runs_per_test=1000.
There was a problem hiding this comment.
We could do that teardown ordering in a separate PR. We need to get this in to unblock CI tests.
There was a problem hiding this comment.
OK, I will merge given that you folks have ownership on the underlying issue. Thanks.
Signed-off-by: Wayne Zhang qiwzhang@google.com
Description:
This change tries to fix #4352
Problem:
When GrpcSubscriptionImpl is deleted, it tries to send one more request update. Newly added sds_api is using GrpcSubscriptionImpl which is owned by a ListenerImpl. When server is killed, GoogleAsyncClientThreadLocal is removed before ListenerManager is deleted. It will cause sds_api to send a google grpc request with a deleted GoogleAsyncClientThreadLocal. Hence the crash.
Fix:
When GoogleAsyncClientThreadLocal is removed, all its active streams will be marked as draining_cq_ = true. If GoogleAsyncClientImpl::writeQueued is called with that flag, bail out.
Risk Level: Low
Testing:
bazel test --runs_per_test=1000 //test/integration:sds_dynamic_integration_test
Docs Changes: None
Release Notes: None