Copilot commented on code in PR #13567:
URL: https://github.com/apache/trafficserver/pull/13567#discussion_r3808634953
##########
include/tsutil/Metrics.h:
##########
@@ -347,7 +346,6 @@ class Metrics
{
auto [blob, entry] = _splitID(id);
- ts::lock_guard lock(_mutex);
return (id >= 0 && ((blob < _cur_blob && entry < MAX_SIZE) || (blob ==
_cur_blob && entry <= _cur_off)));
Review Comment:
`Storage::valid()` now reads `_cur_blob` and `_cur_off` without holding
`_mutex`. Since those members are mutated under `_mutex` in `create()` /
`createSpan()`, this is a data race (undefined behavior) and can also yield
incorrect results under concurrent metric creation.
##########
src/tsutil/Metrics.cc:
##########
@@ -94,7 +94,6 @@ Metrics::Storage::lookup(const std::string_view name) const
Metrics::AtomicType *
Metrics::Storage::lookup(Metrics::IdType id, std::string_view *out_name,
Metrics::MetricType *out_type) const
{
- ts::lock_guard lock(_mutex);
auto [blob_ix, offset] = _splitID(id);
Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
Review Comment:
`Storage::lookup(IdType, ...)` reads `_cur_blob`, `_cur_off`, and
`_blobs[...]` without synchronization, while `create()` / `createSpan()` mutate
those under `_mutex`. This is a data race (undefined behavior) and can also
cause incorrect fallback-to-bad_id behavior under concurrent metric creation.
This issue also appears in the following locations of the same file:
- line 141
- line 198
--
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]
