Add RAII helper class to add/remove an element from an std::list

source/common/common/BUILD

@@ -62,6 +62,9 @@ envoy_cc_library(
 envoy_cc_library(
     name = "cleanup_lib",
     hdrs = ["cleanup.h"],
+    deps = [
+        ":assert_lib",
+    ],
 )
 
 envoy_cc_library(

source/common/common/cleanup.h

@@ -1,6 +1,9 @@
 #pragma once
 
 #include <functional>
+#include <list>
+
+#include "common/common/assert.h"
 
 namespace Envoy {
 
@@ -14,4 +17,34 @@ class Cleanup {
   std::function<void()> f_;
 };
 
+// RAII helper class to add an element to an std::list on construction and erase
+// it on destruction, unless the cancel method has been called.
+template <class T> class RaiiListElement {
+public:
+  RaiiListElement(std::list<T>& container, T element) : container_(container), cancelled_(false) {
+    it_ = container.emplace(container.begin(), element);
+  }
+  virtual ~RaiiListElement() {
+    if (!cancelled_) {
+      erase();
+    }
+  }
+
+  // Cancel deletion of the element on destruction. This should be called if the iterator has
+  // been invalidated, eg. if the list has been cleared or the element removed some other way.
+  void cancel() { cancelled_ = true; }
+
+  // Delete the element now, instead of at destruction.
+  void erase() {
+    ASSERT(!cancelled_);
+    container_.erase(it_);
+    cancelled_ = true;
+  }
+
+private:
+  std::list<T>& container_;
+  typename std::list<T>::iterator it_;
+  bool cancelled_;
+};
+
 } // namespace Envoy

source/common/config/grpc_mux_impl.cc

@@ -23,7 +23,8 @@ GrpcMuxImpl::GrpcMuxImpl(const LocalInfo::LocalInfo& local_info, Grpc::AsyncClie
 GrpcMuxImpl::~GrpcMuxImpl() {
   for (const auto& api_state : api_state_) {
     for (auto watch : api_state.second.watches_) {
-      watch->inserted_ = false;
+      // watch->inserted_ = false;
+      watch->clear();
     }
   }
 }

source/common/config/grpc_mux_impl.h

@@ -10,6 +10,7 @@
 #include "envoy/grpc/status.h"
 #include "envoy/upstream/cluster_manager.h"
 
+#include "common/common/cleanup.h"
 #include "common/common/logger.h"
 #include "common/config/grpc_stream.h"
 #include "common/config/utility.h"
@@ -52,27 +53,32 @@ class GrpcMuxImpl : public GrpcMux,
 private:
   void setRetryTimer();
 
-  struct GrpcMuxWatchImpl : public GrpcMuxWatch {
+  struct GrpcMuxWatchImpl : public GrpcMuxWatch, RaiiListElement<GrpcMuxWatchImpl*> {
     GrpcMuxWatchImpl(const std::set<std::string>& resources, GrpcMuxCallbacks& callbacks,
                      const std::string& type_url, GrpcMuxImpl& parent)
-        : resources_(resources), callbacks_(callbacks), type_url_(type_url), parent_(parent),
-          inserted_(true) {
-      entry_ = parent.api_state_[type_url].watches_.emplace(
-          parent.api_state_[type_url].watches_.begin(), this);
-    }
+        : RaiiListElement<GrpcMuxWatchImpl*>(parent.api_state_[type_url].watches_, this),
+          resources_(resources), callbacks_(callbacks), type_url_(type_url), parent_(parent),
+          inserted_(true) {}
     ~GrpcMuxWatchImpl() override {
       if (inserted_) {
-        parent_.api_state_[type_url_].watches_.erase(entry_);
+        erase();
         if (!resources_.empty()) {
           parent_.sendDiscoveryRequest(type_url_);
         }
       }
     }
+
+    void clear() {
+      inserted_ = false;
+      cancel();
+    }
+
     std::set<std::string> resources_;
     GrpcMuxCallbacks& callbacks_;
     const std::string type_url_;
     GrpcMuxImpl& parent_;
-    std::list<GrpcMuxWatchImpl*>::iterator entry_;
+
+  private:
     bool inserted_;
   };
 

source/common/upstream/BUILD

@@ -46,6 +46,7 @@ envoy_cc_library(
         "//include/envoy/ssl:context_manager_interface",
         "//include/envoy/thread_local:thread_local_interface",
         "//include/envoy/upstream:cluster_manager_interface",
+        "//source/common/common:cleanup_lib",
         "//source/common/common:enum_to_int",
         "//source/common/common:utility_lib",
         "//source/common/config:cds_json_lib",

source/common/upstream/cluster_manager_impl.cc

@@ -777,17 +777,6 @@ ProtobufTypes::MessagePtr ClusterManagerImpl::dumpClusterConfigs() {
   return config_dump;
 }
 
-ClusterManagerImpl::ClusterUpdateCallbacksHandleImpl::ClusterUpdateCallbacksHandleImpl(
-    ClusterUpdateCallbacks& cb, std::list<ClusterUpdateCallbacks*>& ll)
-    : list(ll) {
-  entry = ll.emplace(ll.begin(), &cb);
-}
-
-ClusterManagerImpl::ClusterUpdateCallbacksHandleImpl::~ClusterUpdateCallbacksHandleImpl() {
-  ASSERT(std::find(list.begin(), list.end(), *entry) != list.end());
-  list.erase(entry);
-}
-
 ClusterManagerImpl::ThreadLocalClusterManagerImpl::ThreadLocalClusterManagerImpl(
     ClusterManagerImpl& parent, Event::Dispatcher& dispatcher,
     const absl::optional<std::string>& local_cluster_name)

source/common/upstream/cluster_manager_impl.h

@@ -20,6 +20,7 @@
 #include "envoy/thread_local/thread_local.h"
 #include "envoy/upstream/cluster_manager.h"
 
+#include "common/common/cleanup.h"
 #include "common/config/grpc_mux_impl.h"
 #include "common/http/async_client_impl.h"
 #include "common/upstream/load_stats_reporter.h"
@@ -378,14 +379,11 @@ class ClusterManagerImpl : public ClusterManager, Logger::Loggable<Logger::Id::u
     SystemTime last_updated_;
   };
 
-  struct ClusterUpdateCallbacksHandleImpl : public ClusterUpdateCallbacksHandle {
+  struct ClusterUpdateCallbacksHandleImpl : public ClusterUpdateCallbacksHandle,
+                                            RaiiListElement<ClusterUpdateCallbacks*> {
     ClusterUpdateCallbacksHandleImpl(ClusterUpdateCallbacks& cb,
-                                     std::list<ClusterUpdateCallbacks*>& parent);
-    ~ClusterUpdateCallbacksHandleImpl() override;
-
-  private:
-    std::list<ClusterUpdateCallbacks*>::iterator entry;
-    std::list<ClusterUpdateCallbacks*>& list;
+                                     std::list<ClusterUpdateCallbacks*>& parent)
+        : RaiiListElement<ClusterUpdateCallbacks*>(parent, &cb) {}
   };
 
   typedef std::unique_ptr<ClusterData> ClusterDataPtr;

source/server/BUILD

@@ -338,6 +338,7 @@ envoy_cc_library(
         "//source/common/access_log:access_log_manager_lib",
         "//source/common/api:api_lib",
         "//source/common/buffer:buffer_lib",
+        "//source/common/common:cleanup_lib",
         "//source/common/common:logger_lib",
         "//source/common/common:mutex_tracer_lib",
         "//source/common/common:utility_lib",

source/server/server.cc

@@ -594,16 +594,15 @@ void InstanceImpl::shutdownAdmin() {
 ServerLifecycleNotifier::HandlePtr InstanceImpl::registerCallback(Stage stage,
                                                                   StageCallback callback) {
   auto& callbacks = stage_callbacks_[stage];
-  return std::make_unique<LifecycleCallbackHandle<LifecycleNotifierCallbacks>>(
-      callbacks, callbacks.insert(callbacks.end(), callback));
+  return absl::make_unique<LifecycleCallbackHandle<StageCallback>>(callbacks, callback);
 }
 
 ServerLifecycleNotifier::HandlePtr
 InstanceImpl::registerCallback(Stage stage, StageCallbackWithCompletion callback) {
   ASSERT(stage == Stage::ShutdownExit);
   auto& callbacks = stage_completable_callbacks_[stage];
-  return std::make_unique<LifecycleCallbackHandle<LifecycleNotifierCompletionCallbacks>>(
-      callbacks, callbacks.insert(callbacks.end(), callback));
+  return absl::make_unique<LifecycleCallbackHandle<StageCallbackWithCompletion>>(callbacks,
+                                                                                 callback);
 }
 
 void InstanceImpl::notifyCallbacksForStage(Stage stage, Event::PostCb completion_cb) {

source/server/server.h

@@ -19,6 +19,7 @@
 
 #include "common/access_log/access_log_manager_impl.h"
 #include "common/common/assert.h"
+#include "common/common/cleanup.h"
 #include "common/common/logger_delegates.h"
 #include "common/grpc/async_client_manager_impl.h"
 #include "common/http/context_impl.h"
@@ -269,15 +270,11 @@ class InstanceImpl : Logger::Loggable<Logger::Id::main>,
   using LifecycleNotifierCallbacks = std::list<StageCallback>;
   using LifecycleNotifierCompletionCallbacks = std::list<StageCallbackWithCompletion>;
 
-  template <class T> class LifecycleCallbackHandle : public ServerLifecycleNotifier::Handle {
+  template <class T>
+  class LifecycleCallbackHandle : public ServerLifecycleNotifier::Handle, RaiiListElement<T> {
   public:
-    LifecycleCallbackHandle(T& callbacks, typename T::iterator it)
-        : callbacks_(callbacks), it_(it) {}
-    ~LifecycleCallbackHandle() override { callbacks_.erase(it_); }
-
-  private:
-    T& callbacks_;
-    typename T::iterator it_;
+    LifecycleCallbackHandle(std::list<T>& callbacks, T& callback)
+        : RaiiListElement<T>(callbacks, callback) {}
   };
 
   absl::flat_hash_map<Stage, LifecycleNotifierCallbacks> stage_callbacks_;

test/common/common/cleanup_test.cc

@@ -13,4 +13,39 @@ TEST(CleanupTest, ScopeExitCallback) {
   EXPECT_TRUE(callback_fired);
 }
 
+TEST(RaiiListElementTest, DeleteOnDestruction) {
+  std::list<int> l;
+
+  {
+    EXPECT_EQ(l.size(), 0);
+    RaiiListElement<int> rle(l, 1);
+    EXPECT_EQ(l.size(), 1);
+  }
+  EXPECT_EQ(l.size(), 0);
+}
+
+TEST(RaiiListElementTest, CancelDelete) {
+  std::list<int> l;
+
+  {
+    EXPECT_EQ(l.size(), 0);
+    RaiiListElement<int> rle(l, 1);
+    EXPECT_EQ(l.size(), 1);
+    rle.cancel();
+  }
+  EXPECT_EQ(l.size(), 1);
+}
+
+TEST(RaiiListElementTest, DeleteOnErase) {
+  std::list<int> l;
+
+  {
+    EXPECT_EQ(l.size(), 0);
+    RaiiListElement<int> rle(l, 1);
+    rle.erase();
+    EXPECT_EQ(l.size(), 0);
+  }
+  EXPECT_EQ(l.size(), 0);
+}
+
 } // namespace Envoy