Not to send a discovery request at GrpcSubscriptionImpl destructor #4354
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Wayne Zhang <[email protected]>
Signed-off-by: Wayne Zhang <[email protected]>
|
@htuch Please help to review this change. |
|
Replaced with #4356 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description:
This change tries to fix #4352
The root cause with detail log is described at: #4167
This PR is a re-open of closed PR: #4178
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.
Ideal solution (suggested by htuch)
When GoogleAsyncClientThreadLocal is removed, somehow notify all its clients;
GoogleAsyncClientImpl, not to send any more requests. The Google grpc code is fairly complicated. It is not easy to achieve. The change could be big and high risk.
Short-team solution:
When GrpcSubscriptionImpl is deleted, not to send last request update. The change is simple and low-risk.
Risk Level: Low
Testing: tested with
bazel test --runs_per_test=1000 //test/integration:sds_dynamic_integration_test
Docs Changes: No
Release Notes: No
[Optional Fixes #Issue]
[Optional Deprecated:]