Merge pull request #12 from freshworks/memleak_fix

More Fixes and cleanup Iteration 2
freshworks · Feb 27, 2024 · 8e04976 · 8e04976
2 parents c41c356 + 82c2080
commit 8e04976
Show file tree

Hide file tree

Showing 17 changed files with 98 additions and 102 deletions.
diff --git a/envoy/upstream/load_balancer.h b/envoy/upstream/load_balancer.h
@@ -134,7 +134,10 @@ class LoadBalancer {
  * context information. Load balancers should be written to assume that context information
  * is missing and use sensible defaults.
  */
- virtual HostConstVectorSharedPtr getallHosts(LoadBalancerContext* context) PURE;
+ virtual HostConstVectorSharedPtr getAllHosts(LoadBalancerContext* context){
+ (void)context; // Mark the context parameter as unused to avoid compiler warnings
+ return nullptr; // Return nullptr so that only required class can override this interface implementation
+ };
 
  /**
  * Returns a best effort prediction of the next host to be picked, or nullptr if not predictable.

diff --git a/source/common/upstream/cluster_manager_impl.cc b/source/common/upstream/cluster_manager_impl.cc
@@ -2131,13 +2131,6 @@ HostConstSharedPtr ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEnt
  return lb_->peekAnotherHost(context);
 }
 
-// Make the ThreadLocalClusterManagerImpl class accessible by changing its access specifier or adding a friend declaration.
-HostConstVectorSharedPtr ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::getallHosts(
- LoadBalancerContext* context) {
-
- return lb_->getallHosts(context);
-}
-
 Tcp::ConnectionPool::Instance*
 ClusterManagerImpl::ThreadLocalClusterManagerImpl::ClusterEntry::tcpConnPoolImpl(
  ResourcePriority priority, LoadBalancerContext* context, bool peek) {

diff --git a/source/common/upstream/cluster_manager_impl.h b/source/common/upstream/cluster_manager_impl.h
@@ -628,7 +628,6 @@ class ClusterManagerImpl : public ClusterManager,
 
  HostConstSharedPtr chooseHost(LoadBalancerContext* context);
  HostConstSharedPtr peekAnotherHost(LoadBalancerContext* context);
- HostConstVectorSharedPtr getallHosts(LoadBalancerContext* context);
 
  ThreadLocalClusterManagerImpl& parent_;
  PrioritySetImpl priority_set_;

diff --git a/source/common/upstream/load_balancer_impl.h b/source/common/upstream/load_balancer_impl.h
@@ -518,7 +518,6 @@ class EdfLoadBalancerBase : public ZoneAwareLoadBalancerBase {
 
  // Upstream::ZoneAwareLoadBalancerBase
  HostConstSharedPtr peekAnotherHost(LoadBalancerContext* context) override;
- HostConstVectorSharedPtr getallHosts(LoadBalancerContext* ) override {return nullptr;}
  HostConstSharedPtr chooseHostOnce(LoadBalancerContext* context) override;
 
 protected:
@@ -775,7 +774,6 @@ class RandomLoadBalancer : public ZoneAwareLoadBalancerBase {
  // Upstream::ZoneAwareLoadBalancerBase
  HostConstSharedPtr chooseHostOnce(LoadBalancerContext* context) override;
  HostConstSharedPtr peekAnotherHost(LoadBalancerContext* context) override;
- HostConstVectorSharedPtr getallHosts(LoadBalancerContext* ) override {return nullptr;}
 
 protected:
  HostConstSharedPtr peekOrChoose(LoadBalancerContext* context, bool peek);

diff --git a/source/common/upstream/thread_aware_lb_impl.h b/source/common/upstream/thread_aware_lb_impl.h
@@ -94,7 +94,6 @@ class ThreadAwareLoadBalancerBase : public LoadBalancerBase, public ThreadAwareL
  // Preconnect not implemented for hash based load balancing
  HostConstSharedPtr peekAnotherHost(LoadBalancerContext*) override { return nullptr; }
 
- HostConstVectorSharedPtr getallHosts(LoadBalancerContext*) override { return nullptr; }
  // Pool selection not implemented.
  absl::optional<Upstream::SelectedPoolAndConnection>
  selectExistingConnection(Upstream::LoadBalancerContext* /*context*/,
@@ -130,7 +129,6 @@ class ThreadAwareLoadBalancerBase : public LoadBalancerBase, public ThreadAwareL
  HostConstSharedPtr chooseHost(LoadBalancerContext* context) override;
  // Preconnect not implemented for hash based load balancing
  HostConstSharedPtr peekAnotherHost(LoadBalancerContext*) override { return nullptr; }
- HostConstVectorSharedPtr getallHosts(LoadBalancerContext*) override { return nullptr; }
  absl::optional<Upstream::SelectedPoolAndConnection>
  selectExistingConnection(Upstream::LoadBalancerContext* /*context*/,
  const Upstream::Host& /*host*/,

diff --git a/source/extensions/clusters/aggregate/cluster.cc b/source/extensions/clusters/aggregate/cluster.cc
@@ -188,13 +188,6 @@ AggregateClusterLoadBalancer::peekAnotherHost(Upstream::LoadBalancerContext* con
  return nullptr;
 }
 
-Upstream::HostConstVectorSharedPtr
-AggregateClusterLoadBalancer::getallHosts(Upstream::LoadBalancerContext* context) {
- if (load_balancer_) {
- return load_balancer_->getallHosts(context);
- }
- return nullptr;
-}
 absl::optional<Upstream::SelectedPoolAndConnection>
 AggregateClusterLoadBalancer::selectExistingConnection(Upstream::LoadBalancerContext* context,
  const Upstream::Host& host,

diff --git a/source/extensions/clusters/aggregate/cluster.h b/source/extensions/clusters/aggregate/cluster.h
@@ -77,7 +77,6 @@ class AggregateClusterLoadBalancer : public Upstream::LoadBalancer,
  // Upstream::LoadBalancer
  Upstream::HostConstSharedPtr chooseHost(Upstream::LoadBalancerContext* context) override;
  Upstream::HostConstSharedPtr peekAnotherHost(Upstream::LoadBalancerContext*) override;
- Upstream::HostConstVectorSharedPtr getallHosts(Upstream::LoadBalancerContext*) override;
 
  absl::optional<Upstream::SelectedPoolAndConnection>
  selectExistingConnection(Upstream::LoadBalancerContext* /*context*/,
@@ -104,9 +103,6 @@ class AggregateClusterLoadBalancer : public Upstream::LoadBalancer,
  Upstream::HostConstSharedPtr peekAnotherHost(Upstream::LoadBalancerContext*) override {
  return nullptr;
  }
- Upstream::HostConstVectorSharedPtr getallHosts(Upstream::LoadBalancerContext*) override {
- return nullptr;
- }
 
  absl::optional<Upstream::SelectedPoolAndConnection>
  selectExistingConnection(Upstream::LoadBalancerContext* /*context*/,

diff --git a/source/extensions/clusters/dynamic_forward_proxy/cluster.h b/source/extensions/clusters/dynamic_forward_proxy/cluster.h
@@ -95,7 +95,6 @@ class Cluster : public Upstream::BaseDynamicClusterImpl,
  Upstream::HostConstSharedPtr peekAnotherHost(Upstream::LoadBalancerContext*) override {
  return nullptr;
  }
- Upstream::HostConstVectorSharedPtr getallHosts(Upstream::LoadBalancerContext*) override { return nullptr; }
 
  absl::optional<Upstream::SelectedPoolAndConnection>
  selectExistingConnection(Upstream::LoadBalancerContext* context, const Upstream::Host& host,

diff --git a/source/extensions/clusters/original_dst/original_dst_cluster.h b/source/extensions/clusters/original_dst/original_dst_cluster.h
@@ -98,7 +98,6 @@ class OriginalDstCluster : public ClusterImplBase {
  HostConstSharedPtr chooseHost(LoadBalancerContext* context) override;
  // Preconnecting is not implemented for OriginalDstCluster
  HostConstSharedPtr peekAnotherHost(LoadBalancerContext*) override { return nullptr; }
- HostConstVectorSharedPtr getallHosts(LoadBalancerContext*) override { return nullptr; }
 
  // Pool selection not implemented for OriginalDstCluster
  absl::optional<Upstream::SelectedPoolAndConnection>

diff --git a/source/extensions/clusters/redis/redis_cluster_lb.h b/source/extensions/clusters/redis/redis_cluster_lb.h
@@ -203,27 +203,27 @@ class RedisClusterLoadBalancerFactory : public ClusterSlotUpdateCallBack,
  Upstream::HostConstSharedPtr peekAnotherHost(Upstream::LoadBalancerContext*) override {
  return nullptr;
  }
- Upstream::HostConstVectorSharedPtr getallHosts(Upstream::LoadBalancerContext*) override {
- size_t count = 0;
- if (!shard_vector_) {
- return nullptr;
- }else{
- count = shard_vector_->size();
- }
-
- Envoy::Upstream::HostConstVectorSharedPtr hosts = std::make_shared<std::vector<Upstream::HostConstSharedPtr>>();
- for (auto const& shard : *shard_vector_) {
- auto host = shard->primary();
- if (host) {
- hosts->emplace_back(std::move(host));
- }
- }
- if (count == hosts->size()) {
- return hosts;
- } else {
- return nullptr;
+ Upstream::HostConstVectorSharedPtr getAllHosts(Upstream::LoadBalancerContext*) override {
+
+ if (!shard_vector_) {
+ return nullptr;
+ }
+
+ size_t count = shard_vector_->size();
+
+ Envoy::Upstream::HostConstVectorSharedPtr hosts = std::make_shared<std::vector<Upstream::HostConstSharedPtr>>();
+ for (auto const& shard : *shard_vector_) {
+ auto host = shard->primary();
+ if (host) {
+ hosts->emplace_back(std::move(host));
  }
  }
+ if (count == hosts->size()) {
+ return hosts;
+ } else {
+ return nullptr;
+ }
+ }
  // Pool selection not implemented.
  absl::optional<Upstream::SelectedPoolAndConnection>
  selectExistingConnection(Upstream::LoadBalancerContext* /*context*/,

diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc
@@ -196,17 +196,22 @@ void ClientImpl::onEvent(Network::ConnectionEvent event) {
  if (event == Network::ConnectionEvent::RemoteClose ||
  event == Network::ConnectionEvent::LocalClose) {
 
+ std::string eventTypeStr = (event == Network::ConnectionEvent::RemoteClose ? "RemoteClose" : "LocalClose");
  Upstream::reportUpstreamCxDestroy(host_, event);
  if (!pending_requests_.empty()) {
  Upstream::reportUpstreamCxDestroyActiveRequest(host_, event);
  if (event == Network::ConnectionEvent::RemoteClose) {
  putOutlierEvent(Upstream::Outlier::Result::LocalOriginConnectFailed);
  }
  }
+ // If client is Pubsub handle the upstream close event such that downstream must also be closed.
  if (pending_requests_.empty() && is_pubsub_client_) {
  host_->cluster().trafficStats()->upstream_cx_destroy_with_active_rq_.inc();
- if (pubsub_cb_ != nullptr){
+ ENVOY_LOG(debug,"Pubsub Client Connection close event received:'{}'",eventTypeStr);
+ if ((pubsub_cb_ != nullptr)&&(event == Network::ConnectionEvent::RemoteClose)){
+ ENVOY_LOG(debug,"Pubsub Client Remote close received on Downstream Notify Upstream and close it");
  pubsub_cb_->onFailure();
+ pubsub_cb_.reset();
  }
  }
 
@@ -221,7 +226,7 @@ void ClientImpl::onEvent(Network::ConnectionEvent event) {
  }
 
  connect_or_op_timer_->disableTimer();
- // If client is Pubsub handle the upstream close event such that downstream must also be closed.
+
  } else if (event == Network::ConnectionEvent::Connected) {
  connected_ = true;
  ASSERT(!pending_requests_.empty());

diff --git a/source/extensions/filters/network/common/redis/supported_commands.h b/source/extensions/filters/network/common/redis/supported_commands.h
@@ -87,13 +87,13 @@ struct SupportedCommands {
  * @return commands which handle Redis commands without keys.
  */
  static const absl::flat_hash_set<std::string>& adminNokeyCommands() {
- CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set<std::string>, "script", "flushall","publish","pubsub", "keys", "slowlog", "config");
+ CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set<std::string>, "script", "flushall","publish","pubsub", "keys", "slowlog", "config","client");
  }
  /**
  * @return commands which handle Redis commands without keys.
  */
  static const absl::flat_hash_set<std::string>& allShardCommands() {
- CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set<std::string>, "script", "flushall", "pubsub", "keys", "slowlog", "config");
+ CONSTRUCT_ON_FIRST_USE(absl::flat_hash_set<std::string>, "script", "flushall", "pubsub", "keys", "slowlog", "config","client");
  }
 
  /**