From b7625c575f96384e2b6d55f4d9999650cad7a327 Mon Sep 17 00:00:00 2001
From: "Mark D. Roth" <roth@google.com>
Date: Thu, 28 May 2020 08:20:02 -0700
Subject: [PATCH] Fix CDS and EDS policies to destroy their children in
 OnResourceDoesNotExist().

---
 .../client_channel/lb_policy/xds/cds.cc       | 19 +++++++++++------
 .../client_channel/lb_policy/xds/eds.cc       | 21 +++++++++++++------
 2 files changed, 28 insertions(+), 12 deletions(-)

diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc
index 3869f81a623..500f1ac056b 100644
--- a/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/xds/cds.cc
@@ -93,6 +93,8 @@ class CdsLb : public LoadBalancingPolicy {
 
   void ShutdownLocked() override;
 
+  void MaybeDestroyChildPolicyLocked();
+
   RefCountedPtr<CdsLbConfig> config_;
 
   // Current channel args from the resolver.
@@ -226,6 +228,7 @@ void CdsLb::ClusterWatcher::OnResourceDoesNotExist() {
               absl::StrCat("CDS resource \"", parent_->config_->cluster(),
                            "\" does not exist")
                   .c_str())));
+  parent_->MaybeDestroyChildPolicyLocked();
 }
 
 //
@@ -240,7 +243,7 @@ RefCountedPtr<SubchannelInterface> CdsLb::Helper::CreateSubchannel(
 
 void CdsLb::Helper::UpdateState(grpc_connectivity_state state,
                                 std::unique_ptr<SubchannelPicker> picker) {
-  if (parent_->shutting_down_) return;
+  if (parent_->shutting_down_ || parent_->child_policy_ == nullptr) return;
   if (GRPC_TRACE_FLAG_ENABLED(grpc_cds_lb_trace)) {
     gpr_log(GPR_INFO, "[cdslb %p] state updated by child: %s", this,
             ConnectivityStateName(state));
@@ -287,11 +290,7 @@ void CdsLb::ShutdownLocked() {
     gpr_log(GPR_INFO, "[cdslb %p] shutting down", this);
   }
   shutting_down_ = true;
-  if (child_policy_ != nullptr) {
-    grpc_pollset_set_del_pollset_set(child_policy_->interested_parties(),
-                                     interested_parties());
-    child_policy_.reset();
-  }
+  MaybeDestroyChildPolicyLocked();
   if (xds_client_ != nullptr) {
     if (cluster_watcher_ != nullptr) {
       if (GRPC_TRACE_FLAG_ENABLED(grpc_cds_lb_trace)) {
@@ -304,6 +303,14 @@ void CdsLb::ShutdownLocked() {
   }
 }
 
+void CdsLb::MaybeDestroyChildPolicyLocked() {
+  if (child_policy_ != nullptr) {
+    grpc_pollset_set_del_pollset_set(child_policy_->interested_parties(),
+                                     interested_parties());
+    child_policy_.reset();
+  }
+}
+
 void CdsLb::ResetBackoffLocked() {
   if (child_policy_ != nullptr) child_policy_->ResetBackoffLocked();
 }
diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc
index 4521d842818..8e1263bdd00 100644
--- a/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/xds/eds.cc
@@ -148,6 +148,8 @@ class EdsLb : public LoadBalancingPolicy {
 
   void ShutdownLocked() override;
 
+  void MaybeDestroyChildPolicyLocked();
+
   void UpdatePriorityList(XdsApi::PriorityListUpdate priority_list_update);
   void UpdateChildPolicyLocked();
   OrphanablePtr<LoadBalancingPolicy> CreateChildPolicyLocked(
@@ -263,7 +265,9 @@ RefCountedPtr<SubchannelInterface> EdsLb::Helper::CreateSubchannel(
 
 void EdsLb::Helper::UpdateState(grpc_connectivity_state state,
                                 std::unique_ptr<SubchannelPicker> picker) {
-  if (eds_policy_->shutting_down_) return;
+  if (eds_policy_->shutting_down_ || eds_policy_->child_policy_ == nullptr) {
+    return;
+  }
   if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_eds_trace)) {
     gpr_log(GPR_INFO, "[edslb %p] child policy updated state=%s picker=%p",
             eds_policy_.get(), ConnectivityStateName(state), picker.get());
@@ -351,6 +355,7 @@ class EdsLb::EndpointWatcher : public XdsClient::EndpointWatcherInterface {
         absl::make_unique<TransientFailurePicker>(
             GRPC_ERROR_CREATE_FROM_STATIC_STRING(
                 "EDS resource does not exist")));
+    eds_policy_->MaybeDestroyChildPolicyLocked();
   }
 
  private:
@@ -397,11 +402,7 @@ void EdsLb::ShutdownLocked() {
   // Drop our ref to the child's picker, in case it's holding a ref to
   // the child.
   child_picker_.reset();
-  if (child_policy_ != nullptr) {
-    grpc_pollset_set_del_pollset_set(child_policy_->interested_parties(),
-                                     interested_parties());
-    child_policy_.reset();
-  }
+  MaybeDestroyChildPolicyLocked();
   drop_stats_.reset();
   // Cancel the endpoint watch here instead of in our dtor if we are using the
   // xds resolver, because the watcher holds a ref to us and we might not be
@@ -421,6 +422,14 @@ void EdsLb::ShutdownLocked() {
   xds_client_.reset();
 }
 
+void EdsLb::MaybeDestroyChildPolicyLocked() {
+  if (child_policy_ != nullptr) {
+    grpc_pollset_set_del_pollset_set(child_policy_->interested_parties(),
+                                     interested_parties());
+    child_policy_.reset();
+  }
+}
+
 void EdsLb::UpdateLocked(UpdateArgs args) {
   if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_eds_trace)) {
     gpr_log(GPR_INFO, "[edslb %p] Received update", this);