cmcfarlen commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r4008235819
##########
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;
Review Comment:
Reproduced, and it goes one step further than described. With `a`, `b`, `c`
listed, `start = find("a")`, `stop = find("b")`, then `unlist("b")`: the walk
visits `a`, skips the unlisted `b` and lands on `c`, compares live-vs-live so
it visits `c` outside the range, then exhausts. At that point `at_end()` is
true for the walker and false for the saved `stop`, so `a && b` is false and
`!=` stays true while `++` cannot make progress -- it spins, dereferencing `_it
== _bound`. `Storage::lookup` guards with `offset > _cur_off`, so one-past-end
slips past the redirect and yields an empty name rather than `bad_id`. So:
out-of-range visit followed by a non-terminating loop.
Your reading of the coverage was right too -- the subrange test only
unlisted interior slots, and only before obtaining the iterators. Neither test
unlisted an endpoint after capturing it.
I went with the second of your two options, but by construction rather than
by contract: as of f340f14 there is no way to form a subrange.
`Metrics::for_each(func)` is the only enumeration, always the whole store, so
there is no cursor to save across a listing change and no invalidation rule to
define or enforce. `_bound`, the shared-bound comparison and the equality rules
are all gone with the iterator. The two subrange tests are deleted rather than
extended, since what they covered can no longer be expressed.
Net effect on the PR is 170 insertions against 344 deletions.
--
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]
