Add mutex to protect exchange receiver's async client#5008
Add mutex to protect exchange receiver's async client#5008yibin87 merged 7 commits intopingcap:masterfrom yibin87:dev_feature_branch
Conversation
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
|
[REVIEW NOTIFICATION] This pull request has been approved by:
To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. DetailsReviewer can indicate their review by submitting an approval review. |
Signed-off-by: yibin <huyibin@pingcap.com>
Signed-off-by: yibin <huyibin@pingcap.com>
| std::vector<AsyncHandler *> waiting_for_retry_requests; | ||
|
|
||
| std::vector<AsyncRequestHandler<RPCContext>> handlers; | ||
| std::vector<std::unique_ptr<AsyncHandler>> handlers; |
There was a problem hiding this comment.
I can't get what would happen without this change, could you tell me something?
There was a problem hiding this comment.
In AsyncGrpcExchangePacketReader‘s init function, reader = cluster->rpc_client->sendStreamRequestAsync(xxx), the right part can execute first, and grpc thread sees the reader.
I start from the issue's core down stack, so this suspicous one could be the problem. And I can't tell a whole story about what happend either. Just this seems a suspicous one. And fixed binary doesn't core down.
There was a problem hiding this comment.
yes that's a great job and I've got what happened about this bug.
The only remaining question is why you change the element type of handlers from a value type to a unique_ptr.
It's ok, but why?
There was a problem hiding this comment.
mutex field is not support for copy constructor...
Signed-off-by: yibin <huyibin@pingcap.com>
|
/merge |
|
@yibin87: It seems you want to merge this PR, I will help you trigger all the tests: /run-all-tests You only need to trigger If you have any questions about the PR merge process, please refer to pr process. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
This pull request has been accepted and is ready to merge. DetailsCommit hash: 606b704 |
|
@yibin87: Your PR was out of date, I have automatically updated it for you. At the same time I will also trigger all tests for you: /run-all-tests If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository. |
|
/run-unit-test |
|
/run-unit-test |
Coverage for changed filesCoverage summaryfull coverage report (for internal network access only) |
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created: #5009. |
|
In response to a cherrypick label: new pull request created: #5010. |
What problem does this PR solve?
Issue Number: close #4977
Problem Summary:
What is changed and how it works?
For ExchangeReceiver's async client mode, AsyncRequestHandler instance is used to handle async client request and rsp. And it is shared between grpc and reactor thread. However, when handler is creating, there is a little chance that grpc thread get the instance before the creation is completed. So there is chance that #4977 would happen.
Now this is noticed only when makeAsyncReader failed and retried.
Check List
Tests
Side effects
Documentation
Release note