From 4a0a93efa1727c1a9ccec6bf9fa06661a4e9d58d Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 11 Feb 2020 12:57:11 -0800 Subject: [PATCH 1/5] Fix Issue 20928 --- src/core/ext/filters/client_channel/xds/xds_client.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 1d6b86ad65e..be0f7fb903e 100644 --- a/src/core/ext/filters/client_channel/xds/xds_client.cc +++ b/src/core/ext/filters/client_channel/xds/xds_client.cc @@ -1750,9 +1750,13 @@ XdsClient::~XdsClient() { GRPC_COMBINER_UNREF(combiner_, "xds_client"); } void XdsClient::Orphan() { shutting_down_ = true; + /* Reset chand_, otherwise we will never shut down. */ chand_.reset(); - cluster_map_.clear(); - endpoint_map_.clear(); + /* We do not clear cluster_map_ and endpoint_map_ because the maps contain + * refs for watchers which in turn hold refs to the loadbalancing policies. + * At this point, it is possible for Ads calls to be in progress. Unreffing + * the loadbalancing policies before those calls are done would lead to issues + * such as https://github.com/grpc/grpc/issues/20928. */ Unref(DEBUG_LOCATION, "XdsClient::Orphan()"); } From 43155036586ba7c455805a79e6f7bd52fb533edb Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Tue, 11 Feb 2020 13:27:25 -0800 Subject: [PATCH 2/5] Reviewer comments --- src/core/ext/filters/client_channel/xds/xds_client.cc | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) 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 be0f7fb903e..d5185f07fe7 100644 --- a/src/core/ext/filters/client_channel/xds/xds_client.cc +++ b/src/core/ext/filters/client_channel/xds/xds_client.cc @@ -1750,13 +1750,12 @@ XdsClient::~XdsClient() { GRPC_COMBINER_UNREF(combiner_, "xds_client"); } void XdsClient::Orphan() { shutting_down_ = true; - /* Reset chand_, otherwise we will never shut down. */ chand_.reset(); - /* We do not clear cluster_map_ and endpoint_map_ because the maps contain - * refs for watchers which in turn hold refs to the loadbalancing policies. - * At this point, it is possible for Ads calls to be in progress. Unreffing - * the loadbalancing policies before those calls are done would lead to issues - * such as https://github.com/grpc/grpc/issues/20928. */ + // We do not clear cluster_map_ and endpoint_map_ because the maps contain + // refs for watchers which in turn hold refs to the loadbalancing policies. + // At this point, it is possible for ADS calls to be in progress. Unreffing + // the loadbalancing policies before those calls are done would lead to issues + // such as https://github.com/grpc/grpc/issues/20928. Unref(DEBUG_LOCATION, "XdsClient::Orphan()"); } From 683177082ec95a7f6d1467844a62c473ecb7169b Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Wed, 12 Feb 2020 17:25:14 -0800 Subject: [PATCH 3/5] 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(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(p); - xds_client->Unref(); + xds_client->Unref(DEBUG_LOCATION, "channel arg"); } int XdsClient::ChannelArgCmp(void* p, void* q) { return GPR_ICMP(p, q); } From edda7020a34fee429d4b1fc0fd481331e476e575 Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 13 Feb 2020 12:06:09 -0800 Subject: [PATCH 4/5] Revert channel arg changes and clear the maps only if created from the XdsResolver --- .../filters/client_channel/lb_policy/xds/cds.cc | 3 +-- .../filters/client_channel/lb_policy/xds/xds.cc | 3 +-- .../client_channel/resolver/xds/xds_resolver.cc | 8 +++----- .../ext/filters/client_channel/xds/xds_client.cc | 15 ++++++++++----- 4 files changed, 15 insertions(+), 14 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 e5a84f8adf1..5344cb3ddb1 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,6 +230,7 @@ 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() { @@ -249,8 +250,6 @@ 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 a6c843dd3b9..5cd24724f6e 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,6 +699,7 @@ 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() { @@ -736,8 +737,6 @@ 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 08c2843af8f..e7eed60c446 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,13 +41,11 @@ 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(); - // Destroy the args now since they might hold a ref to the xds client. - grpc_channel_args_destroy(args_); - } + void ShutdownLocked() override { xds_client_.reset(); } 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 1f479b028b6..5a1791f805d 100644 --- a/src/core/ext/filters/client_channel/xds/xds_client.cc +++ b/src/core/ext/filters/client_channel/xds/xds_client.cc @@ -1754,11 +1754,16 @@ XdsClient::~XdsClient() { void XdsClient::Orphan() { shutting_down_ = true; chand_.reset(); - // We do not clear cluster_map_ and endpoint_map_ because the maps contain - // refs for watchers which in turn hold refs to the loadbalancing policies. - // At this point, it is possible for ADS calls to be in progress. Unreffing - // the loadbalancing policies before those calls are done would lead to issues - // such as https://github.com/grpc/grpc/issues/20928. + // We do not clear cluster_map_ and endpoint_map_ if the xds client was + // created by the XdsResolver because the maps contain refs for watchers which + // in turn hold refs to the loadbalancing policies. At this point, it is + // possible for ADS calls to be in progress. Unreffing the loadbalancing + // policies before those calls are done would lead to issues such as + // https://github.com/grpc/grpc/issues/20928. + if (service_config_watcher_ != nullptr) { + cluster_map_.clear(); + endpoint_map_.clear(); + } Unref(DEBUG_LOCATION, "XdsClient::Orphan()"); } From 9713fc6ac55f985a6e5c1de3ab22b49443204c0d Mon Sep 17 00:00:00 2001 From: Yash Tibrewal Date: Thu, 13 Feb 2020 12:21:43 -0800 Subject: [PATCH 5/5] Revert log statement --- src/core/ext/filters/client_channel/xds/xds_client.cc | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) 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 5a1791f805d..f7f2c2c605b 100644 --- a/src/core/ext/filters/client_channel/xds/xds_client.cc +++ b/src/core/ext/filters/client_channel/xds/xds_client.cc @@ -1746,10 +1746,7 @@ XdsClient::XdsClient(Combiner* combiner, grpc_pollset_set* interested_parties, } } -XdsClient::~XdsClient() { - gpr_log(GPR_INFO, "[xds_client %p] Destroying xds client", this); - GRPC_COMBINER_UNREF(combiner_, "xds_client"); -} +XdsClient::~XdsClient() { GRPC_COMBINER_UNREF(combiner_, "xds_client"); } void XdsClient::Orphan() { shutting_down_ = true;