Copilot commented on code in PR #13367:
URL: https://github.com/apache/trafficserver/pull/13367#discussion_r3540283691
##########
src/proxy/HostStatus.cc:
##########
@@ -208,8 +204,9 @@ HostStatus::setHostStatus(const std::string_view name,
TSHostStatus status, cons
// update / insert status.
// using the hash table pointer to store the TSHostStatus value.
HostStatRec *host_stat = nullptr;
- ink_rwlock_wrlock(&host_status_rwlock);
{
+ std::scoped_lock lock(host_status_rwlock);
+
Review Comment:
For exclusive access to ts::bravo::shared_mutex, prefer
std::lock_guard<ts::bravo::shared_mutex> (as suggested by tsutil/Bravo.h)
instead of std::scoped_lock. This matches other BRAVO users in the tree and
avoids relying on std::scoped_lock to carry the right thread-safety annotations.
##########
src/proxy/HostStatus.cc:
##########
@@ -188,16 +184,16 @@ HostStatus::loadRecord(std::string_view name, HostStatRec
&h)
{
HostStatRec *host_stat = nullptr;
Dbg(dbg_ctl_host_statuses, "loading host status record for %.*s",
int(name.size()), name.data());
- ink_rwlock_wrlock(&host_status_rwlock);
{
+ std::scoped_lock lock(host_status_rwlock);
+
Review Comment:
For exclusive access to ts::bravo::shared_mutex, prefer
std::lock_guard<ts::bravo::shared_mutex> (as suggested by tsutil/Bravo.h)
instead of std::scoped_lock. This is the established pattern in the codebase
and is more likely to be understood by the Clang thread-safety annotations used
by BRAVO.
##########
src/proxy/HostStatus.cc:
##########
@@ -336,15 +332,15 @@ HostStatus::getHostStatus(const std::string_view name)
}
// the hash table value pointer has the TSHostStatus value.
- ink_rwlock_rdlock(&host_status_rwlock);
{
+ ts::bravo::shared_lock<ts::bravo::shared_mutex> lock(host_status_rwlock);
+
auto it = hosts_statuses.find(std::string(name));
lookup = it != hosts_statuses.end();
if (lookup) {
Review Comment:
The shared lock is released before dereferencing and mutating the
HostStatRec (e.g. checking _status->status and writing _status->reasons). This
can race with concurrent writers in setHostStatus(), and the final
`_status->reasons = reasons` can also clobber updates performed by
setHostStatus(). Consider: (1) under the shared lock, compute which reason bits
have expired into a local bitmask (and copy any needed timestamps), then (2)
release the lock and call setHostStatus() for those bits, without directly
reading/writing HostStatRec fields outside a lock.
--
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]
