diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index a67792fcd37..40f8cef522b 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1435,10 +1435,8 @@ ChannelData::ChannelData(grpc_channel_element_args* args, grpc_error** error) std::move(target_uri), ProcessResolverResultLocked, this, error)); grpc_channel_args_destroy(new_args); if (*error != GRPC_ERROR_NONE) { - // Orphan the resolving LB policy and flush the exec_ctx to ensure - // that it finishes shutting down. This ensures that if we are - // failing, we destroy the ClientChannelControlHelper (and thus - // unref the channel stack) before we return. + // Before we return, shut down the resolving LB policy, which destroys + // the ClientChannelControlHelper and therefore unrefs the channel stack. // TODO(roth): This is not a complete solution, because it only // catches the case where channel stack initialization fails in this // particular filter. If there is a failure in a different filter, we @@ -1446,7 +1444,6 @@ ChannelData::ChannelData(grpc_channel_element_args* args, grpc_error** error) // in practice, there are no other filters that can cause failures in // channel stack initialization, so this works for now. resolving_lb_policy_.reset(); - ExecCtx::Get()->Flush(); } else { grpc_pollset_set_add_pollset_set(resolving_lb_policy_->interested_parties(), interested_parties_); diff --git a/src/core/ext/filters/client_channel/lb_policy.cc b/src/core/ext/filters/client_channel/lb_policy.cc index 3e4d3703c82..3207f888044 100644 --- a/src/core/ext/filters/client_channel/lb_policy.cc +++ b/src/core/ext/filters/client_channel/lb_policy.cc @@ -43,23 +43,8 @@ LoadBalancingPolicy::~LoadBalancingPolicy() { } void LoadBalancingPolicy::Orphan() { - // Invoke ShutdownAndUnrefLocked() inside of the combiner. - // TODO(roth): Is this actually needed? We should already be in the - // combiner here. Note that if we directly call ShutdownLocked(), - // then we can probably remove the hack whereby the helper is - // destroyed at shutdown instead of at destruction. - GRPC_CLOSURE_SCHED( - GRPC_CLOSURE_CREATE(&LoadBalancingPolicy::ShutdownAndUnrefLocked, this, - grpc_combiner_scheduler(combiner_)), - GRPC_ERROR_NONE); -} - -void LoadBalancingPolicy::ShutdownAndUnrefLocked(void* arg, - grpc_error* ignored) { - LoadBalancingPolicy* policy = static_cast(arg); - policy->ShutdownLocked(); - policy->channel_control_helper_.reset(); - policy->Unref(); + ShutdownLocked(); + Unref(); } // diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h index c21e9d90ec4..ecc235bb71b 100644 --- a/src/core/ext/filters/client_channel/lb_policy.h +++ b/src/core/ext/filters/client_channel/lb_policy.h @@ -282,6 +282,7 @@ class LoadBalancingPolicy : public InternallyRefCounted { grpc_pollset_set* interested_parties() const { return interested_parties_; } + // Note: This must be invoked while holding the combiner. void Orphan() override; // A picker that returns PICK_QUEUE for all picks. @@ -322,7 +323,6 @@ class LoadBalancingPolicy : public InternallyRefCounted { // Note: LB policies MUST NOT call any method on the helper from their // constructor. - // Note: This will return null after ShutdownLocked() has been called. ChannelControlHelper* channel_control_helper() const { return channel_control_helper_.get(); } @@ -331,8 +331,6 @@ class LoadBalancingPolicy : public InternallyRefCounted { virtual void ShutdownLocked() GRPC_ABSTRACT; private: - static void ShutdownAndUnrefLocked(void* arg, grpc_error* ignored); - /// Combiner under which LB policy actions take place. grpc_combiner* combiner_; /// Owned pointer to interested parties in load balancing decisions. 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 74660cec92b..ca6ab216515 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 @@ -1989,6 +1989,9 @@ void XdsLb::LocalityMap::LocalityEntry::ShutdownLocked() { parent_->interested_parties()); pending_child_policy_.reset(); } + // Drop our ref to the child's picker, in case it's holding a ref to + // the child. + picker_ref_.reset(); } void XdsLb::LocalityMap::LocalityEntry::ResetBackoffLocked() { diff --git a/src/core/ext/filters/client_channel/resolver.h b/src/core/ext/filters/client_channel/resolver.h index 87a4442d75b..829a860040a 100644 --- a/src/core/ext/filters/client_channel/resolver.h +++ b/src/core/ext/filters/client_channel/resolver.h @@ -117,12 +117,10 @@ class Resolver : public InternallyRefCounted { /// implementations. At that point, this method can go away. virtual void ResetBackoffLocked() {} + // Note: This must be invoked while holding the combiner. void Orphan() override { - // Invoke ShutdownAndUnrefLocked() inside of the combiner. - GRPC_CLOSURE_SCHED( - GRPC_CLOSURE_CREATE(&Resolver::ShutdownAndUnrefLocked, this, - grpc_combiner_scheduler(combiner_)), - GRPC_ERROR_NONE); + ShutdownLocked(); + Unref(); } GRPC_ABSTRACT_BASE_CLASS @@ -147,12 +145,6 @@ class Resolver : public InternallyRefCounted { ResultHandler* result_handler() const { return result_handler_.get(); } private: - static void ShutdownAndUnrefLocked(void* arg, grpc_error* ignored) { - Resolver* resolver = static_cast(arg); - resolver->ShutdownLocked(); - resolver->Unref(); - } - UniquePtr result_handler_; grpc_combiner* combiner_; };