Copilot commented on code in PR #13480:
URL: https://github.com/apache/trafficserver/pull/13480#discussion_r3707002796
##########
include/iocore/net/ConnectionTracker.h:
##########
@@ -448,25 +448,23 @@ inline int
ConnectionTracker::TxnState::reserve()
{
_reserved_p = true;
- // If metric enabled, use metric as count
+ // @a _count is always the authoritative count; the metrics, if enabled,
only mirror it.
+ int count = ++_g->_count;
if (_g->_count_metric != nullptr) {
ts::Metrics::Gauge::increment(_g->_count_metric);
ts::Metrics::Counter::increment(_g->_count_total_metric);
- return _g->_count_metric->load();
}
- return ++_g->_count;
+ return count;
Review Comment:
`count` is declared as `int`, but `++_g->_count`’s type depends on `_count`
(commonly an atomic 64-bit value). This introduces a potential
narrowing/truncation bug and can also cause signedness issues. Prefer `auto
count = ++_g->_count;` or a fixed-width type matching `_count`’s underlying
type.
##########
src/iocore/net/ConnectionTracker.cc:
##########
@@ -553,7 +540,7 @@ ConnectionTracker::Group::release()
}
} else {
// A bit dubious, as there's no guarantee it's still negative, but even
that would be interesting to know.
- Error("Number of tracked connections should be greater than or equal to
zero: %u", _count.load());
+ Error("Number of tracked connections should be greater than or equal to
zero: %d", _count.load());
Review Comment:
`Error()` uses printf-style formatting, but `%d` may not match the type
returned by `_count.load()` (often an `int64_t`, `uint32_t`, etc.). This can
cause incorrect logging or undefined behavior. Use an appropriate format macro
(e.g., `PRId64`/`PRIu64`) and cast to the corresponding fixed-width type to
match `_count`’s actual storage.
##########
src/iocore/net/ConnectionTracker.cc:
##########
@@ -522,26 +522,13 @@ ConnectionTracker::Group::should_alert(std::time_t *lat)
void
ConnectionTracker::Group::release()
{
- // If metric enabled, use metric as count
- if (_count_metric != nullptr) {
- if (_count_metric->load() > 0) {
+ // @a _count is always the authoritative count; the metric, if enabled, only
mirrors it.
+ if (_count > 0) {
+ int count = --_count;
+ if (_count_metric != nullptr) {
ts::Metrics::Gauge::decrement(_count_metric);
- if (_count_metric->load() == 0) {
- TableSingleton &table = _direction ==
DirectionType::INBOUND ? _inbound_table : _outbound_table;
- std::lock_guard<std::mutex> lock(table._mutex); // Table lock
- if (_count_metric->load() > 0) {
- // Someone else grabbed the Group between our last check and taking
the
- // lock.
- return;
- }
- table._table.erase(_key);
- }
- } else {
- // A bit dubious, as there's no guarantee it's still negative, but even
that would be interesting to know.
- Error("Number of tracked connections should be greater than or equal to
zero: %" PRId64, _count_metric->load());
}
- } else if (_count > 0) {
- if (--_count == 0) {
+ if (count == 0) {
Review Comment:
Similar to `TxnState::reserve()`, `count` is stored as `int` while
`--_count` likely returns a wider/signedness-dependent type. This risks
narrowing and incorrect `count == 0` behavior if `_count` is not an `int`. Use
`auto` or a type that matches `_count`’s underlying value type.
--
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]
