cmcfarlen commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r4008233631
##########
include/tsutil/Metrics.h:
##########
@@ -231,43 +299,80 @@ class Metrics
return std::make_tuple(name, type, metric->_value.load());
}
+ /** Equality.
+ *
+ * Three way rather than a plain position compare: any exhausted iterator
equals the end
+ * sentinel, and equals any other exhausted iterator, since two of them
may have skipped a
+ * different number of unlisted slots. Two live iterators still compare by
position.
+ *
+ * Two positional iterators may hold different snapshots, so exhaustion
between them is judged
+ * against the earlier bound. Otherwise a walk could pass its own bound
while a stop iterator
+ * made later was still live: they would never compare equal and @c
operator++ could not make
+ * progress. The sentinel keeps its own answer, since its bound is
meaningless.
+ *
+ * @note A snapshot is the sequence: iterators from different ones are no
more comparable than
+ * iterators into different containers, and mixing them is unspecified.
Within one snapshot
+ * equality is the equivalence relation an input iterator requires. The
rule above keeps the
+ * unspecified case terminating rather than hanging.
+ */
bool
operator==(const iterator &o) const
{
- return _it == o._it && std::addressof(_metrics) ==
std::addressof(o._metrics);
- }
+ if (std::addressof(_metrics) != std::addressof(o._metrics)) {
+ return false;
+ }
- bool
- operator!=(const iterator &o) const
- {
- return _it != o._it || std::addressof(_metrics) !=
std::addressof(o._metrics);
+ if (_end || o._end) {
+ return at_end() == o.at_end();
+ }
+
+ auto const bound = _bound < o._bound ? _bound : o._bound;
+ bool const a = _it >= bound, b = o._it >= bound;
+
+ if (a || b) {
+ return a && b;
+ }
+ return _it == o._it;
}
private:
void next();
+ void advance();
+ void skip_unlisted();
+
+ bool
+ at_end() const
+ {
+ return _end || _it >= _bound;
+ }
const Metrics &_metrics;
- Metrics::IdType _it;
+ Metrics::IdType _it{0};
+ /// One past the last slot allocated when this iterator was made.
Iteration is a snapshot.
+ Metrics::IdType _bound{0};
+ bool _end{false};
};
iterator
begin() const
{
- return iterator(*this, 0);
+ return iterator(*this);
}
iterator
end() const
{
- return iterator(*this, _storage->next_free_id());
+ return iterator(*this, iterator::end_tag{});
}
iterator
find(const std::string_view name) const
{
auto id = lookup(name);
- if (id == NOT_FOUND) {
+ // An unlisted slot is never visited by iteration, so handing out an
iterator to one would
+ // produce a bound that a skipping walk steps straight over. Reach it with
lookup() instead.
+ if (id == NOT_FOUND || !listed(id)) {
return end();
} else {
return iterator(*this, id);
Review Comment:
Confirmed the check/use gap: `listed(id)` here and `skip_unlisted()` in the
positional constructor are two separate decisions, so an unlist in between
makes the returned iterator dereference to the next listed metric instead of
the one named.
Rather than make the find path retain the requested position, the positional
constructor and `find()` are both gone as of f340f14. Enumeration is now
`Metrics::for_each(func)` and there is no public iterator at all, so there is
no position for a caller to name and nothing to race against. `lookup()` is the
way to reach a single metric by name, which is what `RecLookupRecord`,
`LogAccess` and `TSStatFindName` already used.
Worth noting `find()` predated this PR and was safe there -- the constructor
did not skip, and equality was a plain position compare. This PR is what made
it unsafe, which is why removing it here rather than in a separate change
seemed right. It has no callers in `src`, `include`, `plugins`, `example` or
`tools`; the only ones were the tests added by this PR. Do you know of
out-of-tree consumers? `Metrics.h` is in `TSUTIL_PUBLIC_HEADERS` so this is a
source-breaking removal, and I would rather hear about users now than after
11.0.0.
--
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]
