bneradt commented on code in PR #13616:
URL: https://github.com/apache/trafficserver/pull/13616#discussion_r3993087211
##########
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:
[P2] Keep find() from returning a different metric after concurrent unlisting
There is a check/use gap between `listed(id)` above and the positional
constructor's `skip_unlisted()`. With adjacent listed metrics `a` and `b`,
`find("a")` can observe `listed(a) == true`, another thread can unlist `a`, and
the constructor then advances to `b`. The returned iterator is non-end but
dereferences to a metric whose name does not match the query. I reproduced that
interleaving by triggering the unlist when the constructor acquires its bound.
Please make the find path retain the requested position or return end if that
position is skipped, rather than accepting the next listed metric, and add a
regression test for this interleaving.
##########
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:
[P2] Account for a saved subrange endpoint becoming unlisted
The shared-bound rule does not cover listing changes within the same
allocation snapshot. Create `a`, `b`, and `c`, obtain `start = m.find("a")` and
`stop = m.find("b")`, then call `m.unlist("b")` without creating any metrics. A
walk `for (auto it = start; it != stop; ++it)` skips `b`, visits `c` outside
the intended range, and passes its end without ever comparing equal to `stop`:
the saved stop remains below the shared bound while the walking iterator is
exhausted. This also arises when another thread unlists the endpoint during
traversal. The current subrange test only unlists interior slots before
obtaining the iterators. Please either support this endpoint transition or
explicitly define and enforce the iterator invalidation/synchronization
requirement for unlisting, with a regression test; snapshotting only the
allocation bound does not preserve the endpoint.
--
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]
