Copilot commented on code in PR #13583:
URL: https://github.com/apache/trafficserver/pull/13583#discussion_r3919919285
##########
src/tsutil/Metrics.cc:
##########
@@ -179,60 +191,17 @@ Metrics::Storage::type(IdType id) const
return _extractType(id);
}
-Metrics::SpanType
-Metrics::Storage::createSpan(size_t size, Metrics::MetricType type,
Metrics::IdType *id)
-{
- release_assert(size <= MAX_SIZE);
- std::lock_guard lock(_mutex);
-
- // On the final blob there is nowhere left to grow, so refuse a span that
would fill or overflow
- // it rather than letting addBlob() assert. Same intent as the guard in
create(), and the same
- // cost: some slots of the last blob go unused.
- if (_cur_blob >= MAX_BLOBS - 1 && _cur_off + size >= MAX_SIZE) {
- if (id) {
- *id = 0; // Slot 0 is the reserved bad_id.
- }
- return {};
- }
-
- // A span has to be contiguous, so one that does not fit in the current blob
starts a new one.
- if (_cur_off + size > MAX_SIZE) {
- addBlob();
- }
-
- Metrics::IdType span_start = _makeId(_cur_blob, _cur_off, type);
- Metrics::NamesAndAtomics *blob = _blobs[_cur_blob].get();
- Metrics::AtomicStorage &atomics = std::get<1>(*blob);
- Metrics::SpanType span = Metrics::SpanType(&atomics[_cur_off],
size);
-
- if (id) {
- *id = span_start;
- }
-
- _cur_off += size;
-
- // create() grows as soon as it consumes the last slot; do the same here.
Otherwise a span ending
- // exactly on the boundary leaves _cur_off at MAX_SIZE, and the next
create() writes one past the
- // end of the blob's name array. It also makes end() unreachable for
iterator::next(), which
- // wraps on ++offset == MAX_SIZE.
- if (_cur_off >= MAX_SIZE) {
- addBlob();
- }
-
- return span;
-}
-
bool
Metrics::Storage::rename(Metrics::IdType id, std::string_view name)
{
- auto [blob_ix, offset] = _splitID(id);
- Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
-
// We can only rename Metrics that are already allocated
- if (!blob || (blob_ix == _cur_blob && offset > _cur_off)) {
+ if (!_is_allocated(id)) {
return false;
}
+ auto [blob_ix, offset] = _splitID(id);
+ Metrics::NamesAndAtomics *blob = _blobs[blob_ix].get();
+
std::string &cur = std::get<0>(std::get<0>(*blob)[offset]);
std::lock_guard lock(_mutex);
Review Comment:
`Storage::rename()` reads the current name string (`cur`) before acquiring
`_mutex`. If two threads call `rename()` concurrently, one can mutate that
`std::string` while the other is reading it outside the lock, which is a data
race. Move the lock acquisition before dereferencing the name storage so all
accesses to `cur` and `_lookups` are serialized.
--
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]
