Copilot commented on code in PR #3650:
URL: https://github.com/apache/brpc/pull/3650#discussion_r4366458472
##########
src/brpc/selective_channel.cpp:
##########
@@ -286,11 +287,30 @@ inline int ChannelBalancer::SelectChannel(const
LoadBalancer::SelectIn& in,
}
int ChannelBalancer::CheckHealth() {
- BAIDU_SCOPED_LOCK(_mutex);
- for (ChannelToIdMap::const_iterator it = _chan_map.begin();
- it != _chan_map.end(); ++it) {
- if (!it->second->Failed() &&
- it->first->CheckHealth() == 0) {
+ std::vector<SocketId> ids;
+ {
+ BAIDU_SCOPED_LOCK(_mutex);
+ ids.reserve(_chan_map.size());
+ for (ChannelToIdMap::const_iterator it = _chan_map.begin();
+ it != _chan_map.end(); ++it) {
+ SubChannel* sub = static_cast<SubChannel*>(it->second->user());
+ if (sub->ownership == DOESNT_OWN_CHANNEL) {
+ // The caller may delete a non-owning channel as soon as
+ // RemoveAndDestroyChannel() returns. Keep the mutex while
+ // using it, so removal cannot return during this call.
+ if (!it->second->Failed() && sub->chan->CheckHealth() == 0) {
+ return 0;
+ }
+ } else {
+ ids.push_back(it->second->id());
+ }
+ }
+ }
+ for (size_t i = 0; i < ids.size(); ++i) {
+ SocketUniquePtr ptr;
+ if (Socket::AddressFailedAsWell(ids[i], &ptr) >= 0 &&
+ !ptr->Failed() &&
+ static_cast<SubChannel*>(ptr->user())->chan->CheckHealth() == 0) {
Review Comment:
Both new regression tests exercise only `Describe()`, and the mock's
`CheckHealth()` always returns immediately, leaving this independently
implemented concurrency path untested. Please add blocking health-check tests
verifying that owning registrations allow concurrent add/removal while
remaining alive until the check completes, and that non-owning removal waits
for the check to finish. The current tests would still pass if the health-check
lock scope or lifetime protection regressed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
