-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: slove the coredump for monitor #1804
Conversation
for (const auto& cli : clients) { | ||
cli->WriteResp(msg); | ||
cli->SendReply(); | ||
} | ||
lock.unlock(); // SendReply without lock | ||
} | ||
|
||
void PikaServer::AddMonitorClient(const std::shared_ptr<PikaClientConn>& client_ptr) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the provided code patch, here's a brief code review:
-
Potential Bug:
Thelock.unlock()
call after sending replies to clients can lead to a race condition. It seems like the intention is to release the lock and allow other threads to acquire it. However, since the loop continues to process clients after unlocking, another thread might gain ownership of the lock and modify shared resources concurrently. Consider moving thelock.unlock()
call to the end of the loop or using a different synchronization mechanism to ensure thread safety. -
Improvement Suggestion:
Instead of callinglock.unlock()
explicitly, consider usingstd::lock_guard<std::mutex>
to acquire and automatically release the lock. This approach ensures that the lock is always released correctly, even if an exception is thrown within the loop.
Here's an updated version of the code with the suggested improvements:
void PikaServer::AddMonitorMessage(const std::string& monitor_message) {
std::lock_guard<std::mutex> lock(pika_monitor_clients_mutex_);
for (auto it = pika_monitor_clients_.begin(); it != pika_monitor_clients_.end();) {
const auto& cli = *it;
if (!cli->IsPubSub()) {
it = pika_monitor_clients_.erase(it);
} else {
cli->WriteResp(msg);
cli->SendReply();
++it;
}
}
}
By using std::lock_guard<std::mutex>
and moving the lock outside the loop, the code ensures proper locking and unlocks automatically without the need for manual unlock()
calls.
fix #1689
把原先处于上面的解锁步骤改到了下面,防止多并发情况下对reply消息的组装导致协议返回错误
pika/src/pika_server.cc +1111