From 683177082ec95a7f6d1467844a62c473ecb7169b Mon Sep 17 00:00:00 2001
From: Yash Tibrewal <yashkt@google.com>
Date: Wed, 12 Feb 2020 17:25:14 -0800
Subject: [PATCH] Destroy channel args early to remove xds client references

---
 src/core/ext/filters/client_channel/lb_policy/xds/cds.cc | 3 ++-
 src/core/ext/filters/client_channel/lb_policy/xds/xds.cc | 3 ++-
 .../filters/client_channel/resolver/xds/xds_resolver.cc  | 8 +++++---
 src/core/ext/filters/client_channel/xds/xds_client.cc    | 9 ++++++---
 4 files changed, 15 insertions(+), 8 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 5344cb3ddb1..e5a84f8adf1 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
@@ -230,7 +230,6 @@ CdsLb::~CdsLb() {
   if (GRPC_TRACE_FLAG_ENABLED(grpc_cds_lb_trace)) {
     gpr_log(GPR_INFO, "[cdslb %p] destroying cds LB policy", this);
   }
-  grpc_channel_args_destroy(args_);
 }
 
 void CdsLb::ShutdownLocked() {
@@ -250,6 +249,8 @@ void CdsLb::ShutdownLocked() {
     }
     xds_client_.reset();
   }
+  // Destroy the args_ now since they might be holding a ref to the xds client.
+  grpc_channel_args_destroy(args_);
 }
 
 void CdsLb::ResetBackoffLocked() {
diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc
index 5cd24724f6e..a6c843dd3b9 100644
--- a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc
+++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc
@@ -699,7 +699,6 @@ XdsLb::~XdsLb() {
   if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_xds_trace)) {
     gpr_log(GPR_INFO, "[xdslb %p] destroying xds LB policy", this);
   }
-  grpc_channel_args_destroy(args_);
 }
 
 void XdsLb::ShutdownLocked() {
@@ -737,6 +736,8 @@ void XdsLb::ShutdownLocked() {
     xds_client_from_channel_.reset();
   }
   xds_client_.reset();
+  // Destroy the args now since they might hold a ref to the xds client.
+  grpc_channel_args_destroy(args_);
 }
 
 //
diff --git a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc
index e7eed60c446..08c2843af8f 100644
--- a/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc
+++ b/src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc
@@ -41,11 +41,13 @@ class XdsResolver : public Resolver {
     server_name_.reset(gpr_strdup(path));
   }
 
-  ~XdsResolver() override { grpc_channel_args_destroy(args_); }
-
   void StartLocked() override;
 
-  void ShutdownLocked() override { xds_client_.reset(); }
+  void ShutdownLocked() override {
+    xds_client_.reset();
+    // Destroy the args now since they might hold a ref to the xds client.
+    grpc_channel_args_destroy(args_);
+  }
 
  private:
   class ServiceConfigWatcher : public XdsClient::ServiceConfigWatcherInterface {
diff --git a/src/core/ext/filters/client_channel/xds/xds_client.cc b/src/core/ext/filters/client_channel/xds/xds_client.cc
index d5185f07fe7..1f479b028b6 100644
--- a/src/core/ext/filters/client_channel/xds/xds_client.cc
+++ b/src/core/ext/filters/client_channel/xds/xds_client.cc
@@ -1746,7 +1746,10 @@ XdsClient::XdsClient(Combiner* combiner, grpc_pollset_set* interested_parties,
   }
 }
 
-XdsClient::~XdsClient() { GRPC_COMBINER_UNREF(combiner_, "xds_client"); }
+XdsClient::~XdsClient() {
+  gpr_log(GPR_INFO, "[xds_client %p] Destroying xds client", this);
+  GRPC_COMBINER_UNREF(combiner_, "xds_client");
+}
 
 void XdsClient::Orphan() {
   shutting_down_ = true;
@@ -1905,13 +1908,13 @@ void XdsClient::NotifyOnError(grpc_error* error) {
 
 void* XdsClient::ChannelArgCopy(void* p) {
   XdsClient* xds_client = static_cast<XdsClient*>(p);
-  xds_client->Ref().release();
+  xds_client->Ref(DEBUG_LOCATION, "channel arg").release();
   return p;
 }
 
 void XdsClient::ChannelArgDestroy(void* p) {
   XdsClient* xds_client = static_cast<XdsClient*>(p);
-  xds_client->Unref();
+  xds_client->Unref(DEBUG_LOCATION, "channel arg");
 }
 
 int XdsClient::ChannelArgCmp(void* p, void* q) { return GPR_ICMP(p, q); }