Avoid data race in grpc alarm - #10124
Conversation
|
/run-unit-tests |
|
/test pull-unit-test |
| grpc::ServerContext * getGrpcContext() { return &ctx; } | ||
|
|
||
| String getResourceGroupName() const { return resource_group_name; } | ||
| grpc::Alarm * getAlarm(); |
There was a problem hiding this comment.
Better to use a reference instead of a pointer because this pointer is always not null.
There was a problem hiding this comment.
Can't use reference here because we need to store it in a std::unordered_map in MPPTaskManager
There was a problem hiding this comment.
std::reference_wrapper can be used to store a reference in unordered_map
|
/test pull-integration-test |
|
/test pull-integration-test |
1 similar comment
|
/test pull-integration-test |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gengliqi, guo-shaoge The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
[LGTM Timeline notifier]Timeline:
|
|
/test pull-integration-test |
|
/run-check-issue-triage-complete |
|
/cherry-pick release-8.5 |
|
@solotzg: new pull request created to branch DetailsIn response to this:
Instructions 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. |
What problem does this PR solve?
Issue Number: close #10107
Problem Summary:
In current implementation,
Alarmis hold byMPPGatherTaskSet, and inMPPTaskManager::abortMPPGather, all theAlarmwill be deconstructed since it callgather_task_set->alarms.clear();But when
gather_task_set->alarms.clear();is called, theEstablishCallDatamay still inside grpc's core, and it hold a raw pointer ofAlarmImpl, although insideAlarmImpl, it use atomic to try to make it thread-safeBut
Unref/Refis not thread safe because in grpc's implementation, ifEstablishCallDatais put back to grpc's core, it does not callRefimmediately, instead, looks like grpc only callRefif some event happens:So there is a case that 2 threads try to delete the AlarmImpl concurrently.
gpr_unref(&refs_)gpr_unref(&refs_)return true(refs is 0)Ref()delete thisUnref(), and it will also try to deletethisWhat is changed and how it works?
This pr let
EstablishCallDatato hold the alarm, so it will never be constructed whenEstablishCallDatais inside grpc's core.Check List
Tests
Side effects
Documentation
Release note