Stats no vector

envoy/stats/allocator.h

@@ -58,6 +58,31 @@ class Allocator {
   virtual const SymbolTable& constSymbolTable() const PURE;
   virtual SymbolTable& symbolTable() PURE;
 
+  /**
+   * Mark rejected stats as deleted by moving them to a different vector, so they don't show up
+   * when iterating over stats, but prevent crashes when trying to access references to them.
+   * Note that allocating a stat with the same name after calling this will
+   * return a new stat. Hence callers should seek to avoid this situation, as is
+   * done in ThreadLocalStore.
+   */
+  virtual void markCounterForDeletion(const CounterSharedPtr& counter) PURE;
+  virtual void markGaugeForDeletion(const GaugeSharedPtr& gauge) PURE;
+  virtual void markTextReadoutForDeletion(const TextReadoutSharedPtr& text_readout) PURE;
+
+  /**
+   * Iterate over all stats that need to be sinked. Note, that implementations can potentially hold
+   * on to a mutex that will deadlock if the passed in functors try to create or delete a stat.
+   * @param f_size functor that is provided the number of all sinked stats. Note this is called
+   *        only once, prior to any calls to f_stat.
+   * @param f_stat functor that is provided one sinked stat at a time.
+   */
+  virtual void forEachCounter(std::function<void(std::size_t)> f_size,
+                              std::function<void(Stats::Counter&)> f_stat) const PURE;
+  virtual void forEachGauge(std::function<void(std::size_t)> f_size,
+                            std::function<void(Stats::Gauge&)> f_stat) const PURE;
+  virtual void forEachTextReadout(std::function<void(std::size_t)> f_size,
+                                  std::function<void(Stats::TextReadout&)> f_stat) const PURE;
+
   // TODO(jmarantz): create a parallel mechanism to instantiate histograms. At
   // the moment, histograms don't fit the same pattern of counters and gauges
   // as they are not actually created in the context of a stats allocator.

envoy/stats/store.h

@@ -7,6 +7,7 @@
 #include "envoy/common/pure.h"
 #include "envoy/stats/histogram.h"
 #include "envoy/stats/scope.h"
+#include "envoy/stats/stats.h"
 #include "envoy/stats/stats_matcher.h"
 #include "envoy/stats/tag_producer.h"
 
@@ -48,6 +49,21 @@ class Store : public Scope {
    * @return a list of all known histograms.
    */
   virtual std::vector<ParentHistogramSharedPtr> histograms() const PURE;
+
+  /**
+   * Iterate over all stats that need to be sinked. Note, that implementations can potentially  hold
+   * on to a mutex that will deadlock if the passed in functors try to create or delete a stat.
+   * @param f_size functor that is provided the number of all sinked stats.
+   * @param f_stat functor that is provided one sinked stat at a time.
+   */
+  virtual void forEachCounter(std::function<void(std::size_t)> f_size,
+                              std::function<void(Stats::Counter&)> f_stat) const PURE;
+
+  virtual void forEachGauge(std::function<void(std::size_t)> f_size,
+                            std::function<void(Stats::Gauge&)> f_stat) const PURE;
+
+  virtual void forEachTextReadout(std::function<void(std::size_t)> f_size,
+                                  std::function<void(Stats::TextReadout&)> f_stat) const PURE;
 };
 
 using StorePtr = std::unique_ptr<Store>;

source/common/stats/allocator_impl.cc

@@ -1,5 +1,6 @@
 #include "source/common/stats/allocator_impl.h"
 
+#include <algorithm>
 #include <cstdint>
 
 #include "envoy/stats/stats.h"
@@ -25,6 +26,25 @@ const char AllocatorImpl::DecrementToZeroSyncPoint[] = "decrement-zero";
 AllocatorImpl::~AllocatorImpl() {
   ASSERT(counters_.empty());
   ASSERT(gauges_.empty());
+
+#ifndef NDEBUG
+  // Move deleted stats into the sets for the ASSERTs in removeFromSetLockHeld to function.
+  for (auto& counter : deleted_counters_) {
+    auto insertion = counters_.insert(counter.get());
+    // Assert that there were no duplicates.
+    ASSERT(insertion.second);
+  }
+  for (auto& gauge : deleted_gauges_) {
+    auto insertion = gauges_.insert(gauge.get());
+    // Assert that there were no duplicates.
+    ASSERT(insertion.second);
+  }
+  for (auto& text_readout : deleted_text_readouts_) {
+    auto insertion = text_readouts_.insert(text_readout.get());
+    // Assert that there were no duplicates.
+    ASSERT(insertion.second);
+  }
+#endif
 }
 
 #ifndef ENVOY_CONFIG_COVERAGE
@@ -316,5 +336,77 @@ Counter* AllocatorImpl::makeCounterInternal(StatName name, StatName tag_extracte
   return new CounterImpl(name, *this, tag_extracted_name, stat_name_tags);
 }
 
+void AllocatorImpl::forEachCounter(std::function<void(std::size_t)> f_size,
+                                   std::function<void(Stats::Counter&)> f_stat) const {
+  Thread::LockGuard lock(mutex_);
+  if (f_size != nullptr) {
+    f_size(counters_.size());
+  }
+  for (auto& counter : counters_) {
+    f_stat(*counter);
+  }
+}
+
+void AllocatorImpl::forEachGauge(std::function<void(std::size_t)> f_size,
+                                 std::function<void(Stats::Gauge&)> f_stat) const {
+  Thread::LockGuard lock(mutex_);
+  if (f_size != nullptr) {
+    f_size(gauges_.size());
+  }
+  for (auto& gauge : gauges_) {
+    f_stat(*gauge);
+  }
+}
+
+void AllocatorImpl::forEachTextReadout(std::function<void(std::size_t)> f_size,
+                                       std::function<void(Stats::TextReadout&)> f_stat) const {
+  Thread::LockGuard lock(mutex_);
+  if (f_size != nullptr) {
+    f_size(text_readouts_.size());
+  }
+  for (auto& text_readout : text_readouts_) {
+    f_stat(*text_readout);
+  }
+}
+
+void AllocatorImpl::markCounterForDeletion(const CounterSharedPtr& counter) {
+  Thread::LockGuard lock(mutex_);
+  auto iter = counters_.find(counter->statName());
+  if (iter == counters_.end()) {
+    // This has already been marked for deletion.
+    return;
+  }
+  ASSERT(counter.get() == *iter);
+  // Duplicates are ASSERTed in ~AllocatorImpl.
+  deleted_counters_.emplace_back(*iter);
+  counters_.erase(iter);
+}
+
+void AllocatorImpl::markGaugeForDeletion(const GaugeSharedPtr& gauge) {
+  Thread::LockGuard lock(mutex_);
+  auto iter = gauges_.find(gauge->statName());
+  if (iter == gauges_.end()) {
+    // This has already been marked for deletion.
+    return;
+  }
+  ASSERT(gauge.get() == *iter);
+  // Duplicates are ASSERTed in ~AllocatorImpl.
+  deleted_gauges_.emplace_back(*iter);
+  gauges_.erase(iter);
+}
+
+void AllocatorImpl::markTextReadoutForDeletion(const TextReadoutSharedPtr& text_readout) {
+  Thread::LockGuard lock(mutex_);
+  auto iter = text_readouts_.find(text_readout->statName());
+  if (iter == text_readouts_.end()) {
+    // This has already been marked for deletion.
+    return;
+  }
+  ASSERT(text_readout.get() == *iter);
+  // Duplicates are ASSERTed in ~AllocatorImpl.
+  deleted_text_readouts_.emplace_back(*iter);
+  text_readouts_.erase(iter);
+}
+
 } // namespace Stats
 } // namespace Envoy

source/common/stats/allocator_impl.h

@@ -33,6 +33,15 @@ class AllocatorImpl : public Allocator {
   SymbolTable& symbolTable() override { return symbol_table_; }
   const SymbolTable& constSymbolTable() const override { return symbol_table_; }
 
+  void forEachCounter(std::function<void(std::size_t)>,
+                      std::function<void(Stats::Counter&)>) const override;
+
+  void forEachGauge(std::function<void(std::size_t)>,
+                    std::function<void(Stats::Gauge&)>) const override;
+
+  void forEachTextReadout(std::function<void(std::size_t)>,
+                          std::function<void(Stats::TextReadout&)>) const override;
+
 #ifndef ENVOY_CONFIG_COVERAGE
   void debugPrint();
 #endif
@@ -47,6 +56,10 @@ class AllocatorImpl : public Allocator {
    */
   bool isMutexLockedForTest();
 
+  void markCounterForDeletion(const CounterSharedPtr& counter) override;
+  void markGaugeForDeletion(const GaugeSharedPtr& gauge) override;
+  void markTextReadoutForDeletion(const TextReadoutSharedPtr& text_readout) override;
+
 protected:
   virtual Counter* makeCounterInternal(StatName name, StatName tag_extracted_name,
                                        const StatNameTagVector& stat_name_tags);
@@ -58,21 +71,29 @@ class AllocatorImpl : public Allocator {
   friend class TextReadoutImpl;
   friend class NotifyingAllocatorImpl;
 
-  void removeCounterFromSetLockHeld(Counter* counter) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
-  void removeGaugeFromSetLockHeld(Gauge* gauge) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
-  void removeTextReadoutFromSetLockHeld(Counter* counter) ABSL_EXCLUSIVE_LOCKS_REQUIRED(mutex_);
+  // A mutex is needed here to protect both the stats_ object from both
+  // alloc() and free() operations. Although alloc() operations are called under existing locking,
+  // free() operations are made from the destructors of the individual stat objects, which are not
+  // protected by locks.
+  mutable Thread::MutexBasicLockable mutex_;
 
   StatSet<Counter> counters_ ABSL_GUARDED_BY(mutex_);
   StatSet<Gauge> gauges_ ABSL_GUARDED_BY(mutex_);
   StatSet<TextReadout> text_readouts_ ABSL_GUARDED_BY(mutex_);
 
-  SymbolTable& symbol_table_;
+  // Retain storage for deleted stats; these are no longer in maps because
+  // the matcher-pattern was established after they were created. Since the
+  // stats are held by reference in code that expects them to be there, we
+  // can't actually delete the stats.
+  //
+  // It seems like it would be better to have each client that expects a stat
+  // to exist to hold it as (e.g.) a CounterSharedPtr rather than a Counter&
+  // but that would be fairly complex to change.
+  std::vector<CounterSharedPtr> deleted_counters_ ABSL_GUARDED_BY(mutex_);
+  std::vector<GaugeSharedPtr> deleted_gauges_ ABSL_GUARDED_BY(mutex_);
+  std::vector<TextReadoutSharedPtr> deleted_text_readouts_ ABSL_GUARDED_BY(mutex_);
 
-  // A mutex is needed here to protect both the stats_ object from both
-  // alloc() and free() operations. Although alloc() operations are called under existing locking,
-  // free() operations are made from the destructors of the individual stat objects, which are not
-  // protected by locks.
-  Thread::MutexBasicLockable mutex_;
+  SymbolTable& symbol_table_;
 
   Thread::ThreadSynchronizer sync_;
 };

source/common/stats/isolated_store_impl.h

@@ -2,6 +2,7 @@
 
 #include <algorithm>
 #include <cstring>
+#include <functional>
 #include <string>
 
 #include "envoy/stats/stats.h"
@@ -100,6 +101,14 @@ template <class Base> class IsolatedStatsCache {
     return true;
   }
 
+  void forEachStat(std::function<void(std::size_t)> f_size,
+                   std::function<void(Base&)> f_stat) const {
+    f_size(stats_.size());
+    for (auto const& stat : stats_) {
+      f_stat(*stat.second);
+    }
+  }
+
 private:
   friend class IsolatedStoreImpl;
 
@@ -205,6 +214,21 @@ class IsolatedStoreImpl : public StoreImpl {
     return textReadoutFromStatName(storage.statName());
   }
 
+  void forEachCounter(std::function<void(std::size_t)> f_size,
+                      std::function<void(Stats::Counter&)> f_stat) const override {
+    counters_.forEachStat(f_size, f_stat);
+  }
+
+  void forEachGauge(std::function<void(std::size_t)> f_size,
+                    std::function<void(Stats::Gauge&)> f_stat) const override {
+    gauges_.forEachStat(f_size, f_stat);
+  }
+
+  void forEachTextReadout(std::function<void(std::size_t)> f_size,
+                          std::function<void(Stats::TextReadout&)> f_stat) const override {
+    text_readouts_.forEachStat(f_size, f_stat);
+  }
+
 private:
   IsolatedStoreImpl(std::unique_ptr<SymbolTable>&& symbol_table);