diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 40f8cef522b..a67792fcd37 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1435,8 +1435,10 @@ 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) { - // Before we return, shut down the resolving LB policy, which destroys - // the ClientChannelControlHelper and therefore unrefs the channel stack. + // 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. // 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 @@ -1444,6 +1446,7 @@ 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 3207f888044..3e4d3703c82 100644 --- a/src/core/ext/filters/client_channel/lb_policy.cc +++ b/src/core/ext/filters/client_channel/lb_policy.cc @@ -43,8 +43,23 @@ LoadBalancingPolicy::~LoadBalancingPolicy() { } void LoadBalancingPolicy::Orphan() { - ShutdownLocked(); - Unref(); + // 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(); } // diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h index ecc235bb71b..c21e9d90ec4 100644 --- a/src/core/ext/filters/client_channel/lb_policy.h +++ b/src/core/ext/filters/client_channel/lb_policy.h @@ -282,7 +282,6 @@ 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. @@ -323,6 +322,7 @@ 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,6 +331,8 @@ 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 ca6ab216515..74660cec92b 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,9 +1989,6 @@ 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 829a860040a..87a4442d75b 100644 --- a/src/core/ext/filters/client_channel/resolver.h +++ b/src/core/ext/filters/client_channel/resolver.h @@ -117,10 +117,12 @@ 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 { - ShutdownLocked(); - Unref(); + // Invoke ShutdownAndUnrefLocked() inside of the combiner. + GRPC_CLOSURE_SCHED( + GRPC_CLOSURE_CREATE(&Resolver::ShutdownAndUnrefLocked, this, + grpc_combiner_scheduler(combiner_)), + GRPC_ERROR_NONE); } GRPC_ABSTRACT_BASE_CLASS @@ -145,6 +147,12 @@ 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_; };