Merge pull request #25259 from markdroth/client_channel_filter_fix

Fix locking bug that caused a crash during channel shutdown.

src/core/ext/filters/client_channel/client_channel.cc

@@ -2391,10 +2391,23 @@ void ChannelData::UpdateStateAndPickerLocked(
     grpc_connectivity_state state, const absl::Status& status,
     const char* reason,
     std::unique_ptr<LoadBalancingPolicy::SubchannelPicker> picker) {
-  // Clean the control plane when entering IDLE.
+  // Special case for IDLE and SHUTDOWN states.
   if (picker == nullptr || state == GRPC_CHANNEL_SHUTDOWN) {
     saved_service_config_.reset();
     saved_config_selector_.reset();
+    // Acquire resolution lock to update config selector and associated state.
+    // To minimize lock contention, we wait to unref these objects until
+    // after we release the lock.
+    RefCountedPtr<ServiceConfig> service_config_to_unref;
+    RefCountedPtr<ConfigSelector> config_selector_to_unref;
+    RefCountedPtr<DynamicFilters> dynamic_filters_to_unref;
+    {
+      MutexLock lock(&resolution_mu_);
+      received_service_config_data_ = false;
+      service_config_to_unref = std::move(service_config_);
+      config_selector_to_unref = std::move(config_selector_);
+      dynamic_filters_to_unref = std::move(dynamic_filters_);
+    }
   }
   // Update connectivity state.
   state_tracker_.SetState(state, status, reason);
@@ -2414,13 +2427,7 @@ void ChannelData::UpdateStateAndPickerLocked(
   // the refs until after we release the lock, and then unref them at
   // that point.  This includes the following:
   // - refs to subchannel wrappers in the keys of pending_subchannel_updates_
-  // - ref stored in service_config_
-  // - ref stored in config_selector_
-  // - ref stored in dynamic_filters_
   // - ownership of the existing picker in picker_
-  RefCountedPtr<ServiceConfig> service_config_to_unref;
-  RefCountedPtr<ConfigSelector> config_selector_to_unref;
-  RefCountedPtr<DynamicFilters> dynamic_filters_to_unref;
   {
     MutexLock lock(&data_plane_mu_);
     // Handle subchannel updates.
@@ -2439,14 +2446,6 @@ void ChannelData::UpdateStateAndPickerLocked(
     // Swap out the picker.
     // Note: Original value will be destroyed after the lock is released.
     picker_.swap(picker);
-    // Clean the data plane if the updated picker is nullptr.
-    if (picker_ == nullptr || state == GRPC_CHANNEL_SHUTDOWN) {
-      received_service_config_data_ = false;
-      // Note: We save the objects to unref until after the lock is released.
-      service_config_to_unref = std::move(service_config_);
-      config_selector_to_unref = std::move(config_selector_);
-      dynamic_filters_to_unref = std::move(dynamic_filters_);
-    }
     // Re-process queued picks.
     for (LbQueuedCall* call = lb_queued_calls_; call != nullptr;
          call = call->next) {
@@ -2651,7 +2650,11 @@ CallData::CallData(grpc_call_element* elem, const ChannelData& chand,
       arena_(args.arena),
       owning_call_(args.call_stack),
       call_combiner_(args.call_combiner),
-      call_context_(args.context) {}
+      call_context_(args.context) {
+  if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_call_trace)) {
+    gpr_log(GPR_INFO, "chand=%p calld=%p: created call", &chand, this);
+  }
+}
 
 CallData::~CallData() {
   grpc_slice_unref_internal(path_);
@@ -3166,6 +3169,12 @@ void CallData::CreateDynamicCall(grpc_call_element* elem) {
                                      call_combiner_};
   grpc_error* error = GRPC_ERROR_NONE;
   DynamicFilters* channel_stack = args.channel_stack.get();
+  if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {
+    gpr_log(
+        GPR_INFO,
+        "chand=%p calld=%p: creating dynamic call stack on channel_stack=%p",
+        chand, this, channel_stack);
+  }
   dynamic_call_ = channel_stack->CreateCall(std::move(args), &error);
   if (error != GRPC_ERROR_NONE) {
     if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {