diff --git a/source/common/common/BUILD b/source/common/common/BUILD index a758943e1c1a4..ba1564e6986f5 100644 --- a/source/common/common/BUILD +++ b/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( diff --git a/source/common/common/cleanup.h b/source/common/common/cleanup.h index 1a0deeb8ebf4b..e7039ef069ce1 100644 --- a/source/common/common/cleanup.h +++ b/source/common/common/cleanup.h @@ -1,6 +1,9 @@ #pragma once #include +#include + +#include "common/common/assert.h" namespace Envoy { @@ -14,4 +17,34 @@ class Cleanup { std::function 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 RaiiListElement { +public: + RaiiListElement(std::list& 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& container_; + typename std::list::iterator it_; + bool cancelled_; +}; + } // namespace Envoy diff --git a/source/common/config/grpc_mux_impl.cc b/source/common/config/grpc_mux_impl.cc index fc654d7e1c1d6..918a7d15112ca 100644 --- a/source/common/config/grpc_mux_impl.cc +++ b/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(); } } } diff --git a/source/common/config/grpc_mux_impl.h b/source/common/config/grpc_mux_impl.h index 8702662fc683d..5a5d612bd4877 100644 --- a/source/common/config/grpc_mux_impl.h +++ b/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(const std::set& 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(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 resources_; GrpcMuxCallbacks& callbacks_; const std::string type_url_; GrpcMuxImpl& parent_; - std::list::iterator entry_; + + private: bool inserted_; }; diff --git a/source/common/upstream/BUILD b/source/common/upstream/BUILD index 1746d4d039ccc..66d7dfce6b4bd 100644 --- a/source/common/upstream/BUILD +++ b/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", diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc index 7252be1202a1d..e2d46ab64ab7e 100644 --- a/source/common/upstream/cluster_manager_impl.cc +++ b/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& 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& local_cluster_name) diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h index 2195538576250..a7ce5cb7b5bd4 100644 --- a/source/common/upstream/cluster_manager_impl.h +++ b/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 { ClusterUpdateCallbacksHandleImpl(ClusterUpdateCallbacks& cb, - std::list& parent); - ~ClusterUpdateCallbacksHandleImpl() override; - - private: - std::list::iterator entry; - std::list& list; + std::list& parent) + : RaiiListElement(parent, &cb) {} }; typedef std::unique_ptr ClusterDataPtr; diff --git a/source/server/BUILD b/source/server/BUILD index 6848915b5c150..1844d9c4c97e9 100644 --- a/source/server/BUILD +++ b/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", diff --git a/source/server/server.cc b/source/server/server.cc index 461e2b64961bf..70a763e558a98 100644 --- a/source/server/server.cc +++ b/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>( - callbacks, callbacks.insert(callbacks.end(), callback)); + return absl::make_unique>(callbacks, callback); } ServerLifecycleNotifier::HandlePtr InstanceImpl::registerCallback(Stage stage, StageCallbackWithCompletion callback) { ASSERT(stage == Stage::ShutdownExit); auto& callbacks = stage_completable_callbacks_[stage]; - return std::make_unique>( - callbacks, callbacks.insert(callbacks.end(), callback)); + return absl::make_unique>(callbacks, + callback); } void InstanceImpl::notifyCallbacksForStage(Stage stage, Event::PostCb completion_cb) { diff --git a/source/server/server.h b/source/server/server.h index 010937f4555ec..e05bde5ee9add 100644 --- a/source/server/server.h +++ b/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, using LifecycleNotifierCallbacks = std::list; using LifecycleNotifierCompletionCallbacks = std::list; - template class LifecycleCallbackHandle : public ServerLifecycleNotifier::Handle { + template + class LifecycleCallbackHandle : public ServerLifecycleNotifier::Handle, RaiiListElement { 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& callbacks, T& callback) + : RaiiListElement(callbacks, callback) {} }; absl::flat_hash_map stage_callbacks_; diff --git a/test/common/common/cleanup_test.cc b/test/common/common/cleanup_test.cc index 742f279dc530c..7b666c163caf2 100644 --- a/test/common/common/cleanup_test.cc +++ b/test/common/common/cleanup_test.cc @@ -13,4 +13,39 @@ TEST(CleanupTest, ScopeExitCallback) { EXPECT_TRUE(callback_fired); } +TEST(RaiiListElementTest, DeleteOnDestruction) { + std::list l; + + { + EXPECT_EQ(l.size(), 0); + RaiiListElement rle(l, 1); + EXPECT_EQ(l.size(), 1); + } + EXPECT_EQ(l.size(), 0); +} + +TEST(RaiiListElementTest, CancelDelete) { + std::list l; + + { + EXPECT_EQ(l.size(), 0); + RaiiListElement rle(l, 1); + EXPECT_EQ(l.size(), 1); + rle.cancel(); + } + EXPECT_EQ(l.size(), 1); +} + +TEST(RaiiListElementTest, DeleteOnErase) { + std::list l; + + { + EXPECT_EQ(l.size(), 0); + RaiiListElement rle(l, 1); + rle.erase(); + EXPECT_EQ(l.size(), 0); + } + EXPECT_EQ(l.size(), 0); +} + } // namespace Envoy