Merge pull request #24114 from markdroth/config_selector_error_handling2

Simplify service config processing and fix config selector handling.

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

@@ -241,17 +241,15 @@ class ChannelData {
    public:
     explicit ChannelConfigHelper(ChannelData* chand) : chand_(chand) {}
 
-    ApplyServiceConfigResult ApplyServiceConfig(
+    ChooseServiceConfigResult ChooseServiceConfig(
         const Resolver::Result& result) override;
 
-    void ApplyConfigSelector(
-        bool service_config_changed,
-        RefCountedPtr<ConfigSelector> config_selector) override;
+    void StartUsingServiceConfigForCalls() override;
 
     void ResolverTransientFailure(grpc_error* error) override;
 
    private:
-    static void ProcessLbPolicy(
+    static void ChooseLbPolicy(
         const Resolver::Result& resolver_result,
         const internal::ClientChannelGlobalParsedConfig* parsed_service_config,
         RefCountedPtr<LoadBalancingPolicy::Config>* lb_policy_config);
@@ -267,9 +265,12 @@ class ChannelData {
       const char* reason,
       std::unique_ptr<LoadBalancingPolicy::SubchannelPicker> picker);
 
-  void UpdateServiceConfigInDataPlaneLocked(
-      bool service_config_changed,
-      RefCountedPtr<ConfigSelector> config_selector);
+  void UpdateServiceConfigInControlPlaneLocked(
+      RefCountedPtr<ServiceConfig> service_config,
+      const internal::ClientChannelGlobalParsedConfig* parsed_service_config,
+      const char* lb_policy_name, const grpc_channel_args* args);
+
+  void UpdateServiceConfigInDataPlaneLocked();
 
   void CreateResolvingLoadBalancingPolicyLocked();
 
@@ -320,7 +321,6 @@ class ChannelData {
   grpc_core::UniquePtr<char> health_check_service_name_;
   RefCountedPtr<ServiceConfig> saved_service_config_;
   RefCountedPtr<ConfigSelector> saved_config_selector_;
-  bool received_first_resolver_result_ = false;
   // The number of SubchannelWrapper instances referencing a given Subchannel.
   std::map<Subchannel*, int> subchannel_refcount_map_;
   // The set of SubchannelWrappers that currently exist.
@@ -1433,23 +1433,18 @@ class ChannelData::ClientChannelControlHelper
 // ChannelData::ChannelConfigHelper
 //
 
-// Synchronous callback from ResolvingLoadBalancingPolicy to process a
-// resolver result update.
-ChannelData::ChannelConfigHelper::ApplyServiceConfigResult
-ChannelData::ChannelConfigHelper::ApplyServiceConfig(
+ChannelData::ChannelConfigHelper::ChooseServiceConfigResult
+ChannelData::ChannelConfigHelper::ChooseServiceConfig(
     const Resolver::Result& result) {
-  ApplyServiceConfigResult service_config_result;
+  ChooseServiceConfigResult service_config_result;
   RefCountedPtr<ServiceConfig> service_config;
-  // If resolver did not return a service config or returned an invalid service
-  // config, we need a fallback service config.
   if (result.service_config_error != GRPC_ERROR_NONE) {
     if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {
       gpr_log(GPR_INFO, "chand=%p: resolver returned service config error: %s",
               chand_, grpc_error_string(result.service_config_error));
     }
-    // If the service config was invalid, then fallback to the saved service
-    // config. If there is no saved config either, use the default service
-    // config.
+    // If the service config was invalid, then fallback to the
+    // previously returned service config.
     if (chand_->saved_service_config_ != nullptr) {
       if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {
         gpr_log(GPR_INFO,
@@ -1458,99 +1453,51 @@ ChannelData::ChannelConfigHelper::ApplyServiceConfig(
                 chand_);
       }
       service_config = chand_->saved_service_config_;
-    } else if (chand_->default_service_config_ != nullptr) {
-      if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {
-        gpr_log(GPR_INFO,
-                "chand=%p: resolver returned invalid service config. Using "
-                "default service config provided by client API.",
-                chand_);
-      }
-      service_config = chand_->default_service_config_;
+    } else {
+      // No previously returned config, so put the channel into
+      // TRANSIENT_FAILURE.
+      service_config_result.no_valid_service_config = true;
+      return service_config_result;
     }
   } else if (result.service_config == nullptr) {
-    if (chand_->default_service_config_ != nullptr) {
-      if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {
-        gpr_log(GPR_INFO,
-                "chand=%p: resolver returned no service config. Using default "
-                "service config provided by client API.",
-                chand_);
-      }
-      service_config = chand_->default_service_config_;
+    // Resolver did not return any service config.
+    if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {
+      gpr_log(GPR_INFO,
+              "chand=%p: resolver returned no service config. Using default "
+              "service config for channel.",
+              chand_);
     }
+    service_config = chand_->default_service_config_;
   } else {
+    // Use service config returned by resolver.
     service_config = result.service_config;
   }
-  service_config_result.service_config_error =
-      GRPC_ERROR_REF(result.service_config_error);
-  if (service_config == nullptr &&
-      result.service_config_error != GRPC_ERROR_NONE) {
-    service_config_result.no_valid_service_config = true;
-    return service_config_result;
-  }
-  // Process service config.
-  grpc_core::UniquePtr<char> service_config_json;
+  GPR_ASSERT(service_config != nullptr);
+  // Extract global config for client channel.
   const internal::ClientChannelGlobalParsedConfig* parsed_service_config =
-      nullptr;
-  if (service_config != nullptr) {
-    parsed_service_config =
-        static_cast<const internal::ClientChannelGlobalParsedConfig*>(
-            service_config->GetGlobalParsedConfig(
-                internal::ClientChannelServiceConfigParser::ParserIndex()));
-  }
+      static_cast<const internal::ClientChannelGlobalParsedConfig*>(
+          service_config->GetGlobalParsedConfig(
+              internal::ClientChannelServiceConfigParser::ParserIndex()));
+  // Find LB policy config.
+  ChooseLbPolicy(result, parsed_service_config,
+                 &service_config_result.lb_policy_config);
   // Check if the config has changed.
   service_config_result.service_config_changed =
-      ((service_config == nullptr) !=
-       (chand_->saved_service_config_ == nullptr)) ||
-      (service_config != nullptr &&
-       service_config->json_string() !=
-           chand_->saved_service_config_->json_string());
+      chand_->saved_service_config_ == nullptr ||
+      service_config->json_string() !=
+          chand_->saved_service_config_->json_string();
+  // If it has, apply the global parameters now.
   if (service_config_result.service_config_changed) {
-    service_config_json.reset(gpr_strdup(
-        service_config != nullptr ? service_config->json_string().c_str()
-                                  : ""));
-    if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {
-      gpr_log(GPR_INFO,
-              "chand=%p: resolver returned updated service config: \"%s\"",
-              chand_, service_config_json.get());
-    }
-    // Save health check service name.
-    if (service_config != nullptr) {
-      chand_->health_check_service_name_.reset(
-          gpr_strdup(parsed_service_config->health_check_service_name()));
-    } else {
-      chand_->health_check_service_name_.reset();
-    }
-    // Update health check service name used by existing subchannel wrappers.
-    for (auto* subchannel_wrapper : chand_->subchannel_wrappers_) {
-      subchannel_wrapper->UpdateHealthCheckServiceName(
-          grpc_core::UniquePtr<char>(
-              gpr_strdup(chand_->health_check_service_name_.get())));
-    }
-    // Save service config.
-    chand_->saved_service_config_ = std::move(service_config);
-  }
-  // Find LB policy config.
-  ProcessLbPolicy(result, parsed_service_config,
-                  &service_config_result.lb_policy_config);
-  grpc_core::UniquePtr<char> lb_policy_name(
-      gpr_strdup((service_config_result.lb_policy_config)->name()));
-  // Swap out the data used by GetChannelInfo().
-  {
-    MutexLock lock(&chand_->info_mu_);
-    chand_->info_lb_policy_name_ = std::move(lb_policy_name);
-    if (service_config_json != nullptr) {
-      chand_->info_service_config_json_ = std::move(service_config_json);
-    }
+    chand_->UpdateServiceConfigInControlPlaneLocked(
+        std::move(service_config), parsed_service_config,
+        service_config_result.lb_policy_config->name(), result.args);
   }
   // Return results.
   return service_config_result;
 }
 
-void ChannelData::ChannelConfigHelper::ApplyConfigSelector(
-    bool service_config_changed,
-    RefCountedPtr<ConfigSelector> config_selector) {
-  chand_->UpdateServiceConfigInDataPlaneLocked(service_config_changed,
-                                               std::move(config_selector));
+void ChannelData::ChannelConfigHelper::StartUsingServiceConfigForCalls() {
+  chand_->UpdateServiceConfigInDataPlaneLocked();
 }
 
 void ChannelData::ChannelConfigHelper::ResolverTransientFailure(
@@ -1560,21 +1507,19 @@ void ChannelData::ChannelConfigHelper::ResolverTransientFailure(
   chand_->resolver_transient_failure_error_ = error;
 }
 
-void ChannelData::ChannelConfigHelper::ProcessLbPolicy(
+void ChannelData::ChannelConfigHelper::ChooseLbPolicy(
     const Resolver::Result& resolver_result,
     const internal::ClientChannelGlobalParsedConfig* parsed_service_config,
     RefCountedPtr<LoadBalancingPolicy::Config>* lb_policy_config) {
   // Prefer the LB policy config found in the service config.
-  if (parsed_service_config != nullptr &&
-      parsed_service_config->parsed_lb_config() != nullptr) {
+  if (parsed_service_config->parsed_lb_config() != nullptr) {
     *lb_policy_config = parsed_service_config->parsed_lb_config();
     return;
   }
   // Try the deprecated LB policy name from the service config.
   // If not, try the setting from channel args.
   const char* policy_name = nullptr;
-  if (parsed_service_config != nullptr &&
-      !parsed_service_config->parsed_deprecated_lb_policy().empty()) {
+  if (!parsed_service_config->parsed_deprecated_lb_policy().empty()) {
     policy_name = parsed_service_config->parsed_deprecated_lb_policy().c_str();
   } else {
     const grpc_arg* channel_arg =
@@ -1694,16 +1639,16 @@ ChannelData::ChannelData(grpc_channel_element_args* args, grpc_error** error)
         "filter");
     return;
   }
-  // Get default service config
+  // Get default service config.  If none is specified via the client API,
+  // we use an empty config.
   const char* service_config_json = grpc_channel_arg_get_string(
       grpc_channel_args_find(args->channel_args, GRPC_ARG_SERVICE_CONFIG));
-  if (service_config_json != nullptr) {
-    *error = GRPC_ERROR_NONE;
-    default_service_config_ = ServiceConfig::Create(service_config_json, error);
-    if (*error != GRPC_ERROR_NONE) {
-      default_service_config_.reset();
-      return;
-    }
+  if (service_config_json == nullptr) service_config_json = "{}";
+  *error = GRPC_ERROR_NONE;
+  default_service_config_ = ServiceConfig::Create(service_config_json, error);
+  if (*error != GRPC_ERROR_NONE) {
+    default_service_config_.reset();
+    return;
   }
   grpc_uri* uri = grpc_uri_parse(server_uri, true);
   if (uri != nullptr && uri->path[0] != '\0') {
@@ -1755,7 +1700,6 @@ void ChannelData::UpdateStateAndPickerLocked(
     health_check_service_name_.reset();
     saved_service_config_.reset();
     saved_config_selector_.reset();
-    received_first_resolver_result_ = false;
   }
   // Update connectivity state.
   state_tracker_.SetState(state, status, reason);
@@ -1823,51 +1767,56 @@ void ChannelData::UpdateStateAndPickerLocked(
   pending_subchannel_updates_.clear();
 }
 
-void ChannelData::UpdateServiceConfigInDataPlaneLocked(
-    bool service_config_changed,
-    RefCountedPtr<ConfigSelector> config_selector) {
-  // If the service config did not change and there is no new ConfigSelector,
-  // retain the old one (if any).
-  // TODO(roth): Consider whether this is really the right way to handle
-  // this.  We might instead want to decide this in ApplyServiceConfig()
-  // where we decide whether to stick with the saved service config.
-  if (!service_config_changed && config_selector == nullptr) {
-    config_selector = saved_config_selector_;
-  }
-  // Check if ConfigSelector has changed.
-  const bool config_selector_changed =
-      saved_config_selector_ != config_selector;
-  saved_config_selector_ = config_selector;
-  // We want to set the service config at least once, even if the
-  // resolver does not return a config, because that ensures that we
-  // disable retries if they are not enabled in the service config.
-  // TODO(roth): Consider removing the received_first_resolver_result_ check
-  // when we implement transparent retries.
-  if (!service_config_changed && !config_selector_changed &&
-      received_first_resolver_result_) {
-    return;
+void ChannelData::UpdateServiceConfigInControlPlaneLocked(
+    RefCountedPtr<ServiceConfig> service_config,
+    const internal::ClientChannelGlobalParsedConfig* parsed_service_config,
+    const char* lb_policy_name, const grpc_channel_args* args) {
+  grpc_core::UniquePtr<char> service_config_json(
+      gpr_strdup(service_config->json_string().c_str()));
+  if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) {
+    gpr_log(GPR_INFO,
+            "chand=%p: resolver returned updated service config: \"%s\"", this,
+            service_config_json.get());
+  }
+  // Save service config.
+  saved_service_config_ = std::move(service_config);
+  // Save health check service name.
+  health_check_service_name_.reset(
+      gpr_strdup(parsed_service_config->health_check_service_name()));
+  // Update health check service name used by existing subchannel wrappers.
+  for (auto* subchannel_wrapper : subchannel_wrappers_) {
+    subchannel_wrapper->UpdateHealthCheckServiceName(grpc_core::UniquePtr<char>(
+        gpr_strdup(health_check_service_name_.get())));
+  }
+  // Swap out the data used by GetChannelInfo().
+  grpc_core::UniquePtr<char> lb_policy_name_owned(gpr_strdup(lb_policy_name));
+  {
+    MutexLock lock(&info_mu_);
+    info_lb_policy_name_ = std::move(lb_policy_name_owned);
+    info_service_config_json_ = std::move(service_config_json);
   }
-  received_first_resolver_result_ = true;
+  // Save config selector.
+  saved_config_selector_ = ConfigSelector::GetFromChannelArgs(*args);
+}
+
+void ChannelData::UpdateServiceConfigInDataPlaneLocked() {
   // Get retry throttle data from service config.
+  const internal::ClientChannelGlobalParsedConfig* parsed_service_config =
+      static_cast<const internal::ClientChannelGlobalParsedConfig*>(
+          saved_service_config_->GetGlobalParsedConfig(
+              internal::ClientChannelServiceConfigParser::ParserIndex()));
+  absl::optional<internal::ClientChannelGlobalParsedConfig::RetryThrottling>
+      retry_throttle_config = parsed_service_config->retry_throttling();
   RefCountedPtr<ServerRetryThrottleData> retry_throttle_data;
-  if (saved_service_config_ != nullptr) {
-    const internal::ClientChannelGlobalParsedConfig* parsed_service_config =
-        static_cast<const internal::ClientChannelGlobalParsedConfig*>(
-            saved_service_config_->GetGlobalParsedConfig(
-                internal::ClientChannelServiceConfigParser::ParserIndex()));
-    if (parsed_service_config != nullptr) {
-      absl::optional<internal::ClientChannelGlobalParsedConfig::RetryThrottling>
-          retry_throttle_config = parsed_service_config->retry_throttling();
-      if (retry_throttle_config.has_value()) {
-        retry_throttle_data =
-            internal::ServerRetryThrottleMap::GetDataForServer(
-                server_name_.get(),
-                retry_throttle_config.value().max_milli_tokens,
-                retry_throttle_config.value().milli_token_ratio);
-      }
-    }
-  }
-  // Create default config selector if not provided by resolver.
+  if (retry_throttle_config.has_value()) {
+    retry_throttle_data = internal::ServerRetryThrottleMap::GetDataForServer(
+        server_name_.get(), retry_throttle_config.value().max_milli_tokens,
+        retry_throttle_config.value().milli_token_ratio);
+  }
+  // Grab ref to service config.
+  RefCountedPtr<ServiceConfig> service_config = saved_service_config_;
+  // Grab ref to config selector.  Use default if resolver didn't supply one.
+  RefCountedPtr<ConfigSelector> config_selector = saved_config_selector_;
   if (config_selector == nullptr) {
     config_selector =
         MakeRefCounted<DefaultConfigSelector>(saved_service_config_);
@@ -1876,9 +1825,6 @@ void ChannelData::UpdateServiceConfigInDataPlaneLocked(
   //
   // We defer unreffing the old values (and deallocating memory) until
   // after releasing the lock to keep the critical section small.
-  RefCountedPtr<ServiceConfig> service_config_to_unref = saved_service_config_;
-  RefCountedPtr<ConfigSelector> config_selector_to_unref =
-      std::move(config_selector);
   {
     MutexLock lock(&data_plane_mu_);
     GRPC_ERROR_UNREF(resolver_transient_failure_error_);
@@ -1887,8 +1833,8 @@ void ChannelData::UpdateServiceConfigInDataPlaneLocked(
     received_service_config_data_ = true;
     // Old values will be unreffed after lock is released.
     retry_throttle_data_.swap(retry_throttle_data);
-    service_config_.swap(service_config_to_unref);
-    config_selector_.swap(config_selector_to_unref);
+    service_config_.swap(service_config);
+    config_selector_.swap(config_selector);
     // Re-process queued picks.
     for (QueuedPick* pick = queued_picks_; pick != nullptr; pick = pick->next) {
       grpc_call_element* elem = pick->elem;

src/core/ext/filters/client_channel/config_selector.h

@@ -76,14 +76,17 @@ class ConfigSelector : public RefCounted<ConfigSelector> {
 class DefaultConfigSelector : public ConfigSelector {
  public:
   explicit DefaultConfigSelector(RefCountedPtr<ServiceConfig> service_config)
-      : service_config_(std::move(service_config)) {}
+      : service_config_(std::move(service_config)) {
+    // The client channel code ensures that this will never be null.
+    // If neither the resolver nor the client application provide a
+    // config, a default empty config will be used.
+    GPR_DEBUG_ASSERT(service_config_ != nullptr);
+  }
 
   CallConfig GetCallConfig(GetCallConfigArgs args) override {
     CallConfig call_config;
-    if (service_config_ != nullptr) {
-      call_config.method_configs =
-          service_config_->GetMethodParsedConfigVector(*args.path);
-    }
+    call_config.method_configs =
+        service_config_->GetMethodParsedConfigVector(*args.path);
     return call_config;
   }
 

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

@@ -232,9 +232,12 @@ void ResolvingLoadBalancingPolicy::CreateOrUpdateLbPolicyLocked(
   UpdateArgs update_args;
   update_args.addresses = std::move(result.addresses);
   update_args.config = std::move(lb_policy_config);
-  // TODO(roth): Once channel args is converted to C++, use std::move() here.
-  update_args.args = result.args;
-  result.args = nullptr;
+  // Remove the config selector from channel args so that we're not holding
+  // unnecessary refs that cause it to be destroyed somewhere other than in the
+  // WorkSerializer.
+  const char* arg_name = GRPC_ARG_CONFIG_SELECTOR;
+  update_args.args =
+      grpc_channel_args_copy_and_remove(result.args, &arg_name, 1);
   // Create policy if needed.
   if (lb_policy_ == nullptr) {
     lb_policy_ = CreateLbPolicyLocked(*update_args.args);
@@ -306,61 +309,46 @@ void ResolvingLoadBalancingPolicy::OnResolverResultChangedLocked(
   //
   // We track a list of strings to eventually be concatenated and traced.
   TraceStringVector trace_strings;
-  const bool resolution_contains_addresses = result.addresses.size() > 0;
-  // Process the resolver result.
-  ChannelConfigHelper::ApplyServiceConfigResult service_config_result;
+  MaybeAddTraceMessagesForAddressChangesLocked(!result.addresses.empty(),
+                                               &trace_strings);
+  // The result of grpc_error_string() is owned by the error itself.
+  // We're storing that string in trace_strings, so we need to make sure
+  // that the error lives until we're done with the string.
+  grpc_error* service_config_error =
+      GRPC_ERROR_REF(result.service_config_error);
+  if (service_config_error != GRPC_ERROR_NONE) {
+    trace_strings.push_back(grpc_error_string(service_config_error));
+  }
+  // Choose the service config.
+  ChannelConfigHelper::ChooseServiceConfigResult service_config_result;
   if (helper_ != nullptr) {
-    service_config_result = helper_->ApplyServiceConfig(result);
-    if (service_config_result.service_config_error != GRPC_ERROR_NONE) {
-      if (service_config_result.no_valid_service_config) {
-        // We received an invalid service config and we don't have a
-        // fallback service config.
-        OnResolverError(service_config_result.service_config_error);
-        service_config_result.service_config_error = GRPC_ERROR_NONE;
-      }
-    }
+    service_config_result = helper_->ChooseServiceConfig(result);
   } else {
     service_config_result.lb_policy_config = child_lb_config_;
   }
-  // Before we send the args to the LB policy, grab the ConfigSelector for
-  // later use.
-  RefCountedPtr<ConfigSelector> config_selector =
-      ConfigSelector::GetFromChannelArgs(*result.args);
-  // Remove the config selector from channel args so that we're not holding
-  // unnecessary refs that cause it to be destroyed somewhere other than in the
-  // WorkSerializer.
-  const char* arg_name = GRPC_ARG_CONFIG_SELECTOR;
-  grpc_channel_args* new_args =
-      grpc_channel_args_copy_and_remove(result.args, &arg_name, 1);
-  grpc_channel_args_destroy(result.args);
-  result.args = new_args;
-  // Create or update LB policy, as needed.
-  if (service_config_result.lb_policy_config != nullptr) {
+  if (service_config_result.no_valid_service_config) {
+    // We received an invalid service config and we don't have a
+    // previous service config to fall back to.
+    OnResolverError(GRPC_ERROR_REF(service_config_error));
+    trace_strings.push_back("no valid service config");
+  } else {
+    // Create or update LB policy, as needed.
     CreateOrUpdateLbPolicyLocked(
         std::move(service_config_result.lb_policy_config), std::move(result));
-  }
-  // Apply ConfigSelector to channel.
-  // This needs to happen after the LB policy has been updated, since
-  // the ConfigSelector may need the LB policy to know about new
-  // destinations before it can send RPCs to those destinations.
-  if (helper_ != nullptr) {
-    helper_->ApplyConfigSelector(service_config_result.service_config_changed,
-                                 std::move(config_selector));
+    if (service_config_result.service_config_changed) {
+      // Tell channel to start using new service config for calls.
+      // This needs to happen after the LB policy has been updated, since
+      // the ConfigSelector may need the LB policy to know about new
+      // destinations before it can send RPCs to those destinations.
+      if (helper_ != nullptr) helper_->StartUsingServiceConfigForCalls();
+      // TODO(ncteisen): might be worth somehow including a snippet of the
+      // config in the trace, at the risk of bloating the trace logs.
+      trace_strings.push_back("Service config changed");
+    }
   }
   // Add channel trace event.
-  if (service_config_result.service_config_changed) {
-    // TODO(ncteisen): might be worth somehow including a snippet of the
-    // config in the trace, at the risk of bloating the trace logs.
-    trace_strings.push_back("Service config changed");
-  }
-  if (service_config_result.service_config_error != GRPC_ERROR_NONE) {
-    trace_strings.push_back(
-        grpc_error_string(service_config_result.service_config_error));
-  }
-  MaybeAddTraceMessagesForAddressChangesLocked(resolution_contains_addresses,
-                                               &trace_strings);
   ConcatenateAndAddChannelTraceLocked(trace_strings);
-  GRPC_ERROR_UNREF(service_config_result.service_config_error);
+  GRPC_ERROR_UNREF(service_config_error);
 }
 
 }  // namespace grpc_core

src/core/ext/filters/client_channel/resolving_lb_policy.h

@@ -55,29 +55,25 @@ class ResolvingLoadBalancingPolicy : public LoadBalancingPolicy {
  public:
   class ChannelConfigHelper {
    public:
-    struct ApplyServiceConfigResult {
+    struct ChooseServiceConfigResult {
       // Set to true if the service config has changed since the last result.
       bool service_config_changed = false;
       // Set to true if we don't have a valid service config to use.
       // This tells the ResolvingLoadBalancingPolicy to put the channel
       // into TRANSIENT_FAILURE.
       bool no_valid_service_config = false;
-      // A service config parsing error occurred.
-      grpc_error* service_config_error = GRPC_ERROR_NONE;
       // The LB policy config to use.
       RefCountedPtr<LoadBalancingPolicy::Config> lb_policy_config;
     };
 
     virtual ~ChannelConfigHelper() = default;
 
-    // Applies the service config to the channel.
-    virtual ApplyServiceConfigResult ApplyServiceConfig(
+    // Chooses the service config for the channel.
+    virtual ChooseServiceConfigResult ChooseServiceConfig(
         const Resolver::Result& result) = 0;
 
-    // Applies the ConfigSelector to the channel.
-    virtual void ApplyConfigSelector(
-        bool service_config_changed,
-        RefCountedPtr<ConfigSelector> config_selector) = 0;
+    // Starts using the service config for calls.
+    virtual void StartUsingServiceConfigForCalls() = 0;
 
     // Indicates a resolver transient failure.
     virtual void ResolverTransientFailure(grpc_error* error) = 0;

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

@@ -202,15 +202,16 @@ std::string ServiceConfig::ParseJsonMethodName(const Json& json,
 
 const ServiceConfigParser::ParsedConfigVector*
 ServiceConfig::GetMethodParsedConfigVector(const grpc_slice& path) const {
+  if (parsed_method_configs_map_.empty()) return nullptr;
   // Try looking up the full path in the map.
   auto it = parsed_method_configs_map_.find(path);
   if (it != parsed_method_configs_map_.end()) return it->second;
   // If we didn't find a match for the path, try looking for a wildcard
   // entry (i.e., change "/service/method" to "/service/").
   UniquePtr<char> path_str(grpc_slice_to_c_string(path));
-  char* sep = strrchr(path_str.get(), '/') + 1;
+  char* sep = strrchr(path_str.get(), '/');
   if (sep == nullptr) return nullptr;  // Shouldn't ever happen.
-  *sep = '\0';
+  sep[1] = '\0';
   grpc_slice wildcard_path = grpc_slice_from_static_string(path_str.get());
   it = parsed_method_configs_map_.find(wildcard_path);
   if (it != parsed_method_configs_map_.end()) return it->second;

test/cpp/end2end/service_config_end2end_test.cc

@@ -437,7 +437,7 @@ TEST_F(ServiceConfigEnd2endTest, NoServiceConfigTest) {
   auto stub = BuildStub(channel);
   SetNextResolutionNoServiceConfig(GetServersPorts());
   CheckRpcSendOk(stub, DEBUG_LOCATION);
-  EXPECT_STREQ("", channel->GetServiceConfigJSON().c_str());
+  EXPECT_STREQ("{}", channel->GetServiceConfigJSON().c_str());
 }
 
 TEST_F(ServiceConfigEnd2endTest, NoServiceConfigWithDefaultConfigTest) {
@@ -458,16 +458,6 @@ TEST_F(ServiceConfigEnd2endTest, InvalidServiceConfigTest) {
   CheckRpcSendFailure(stub);
 }
 
-TEST_F(ServiceConfigEnd2endTest, InvalidServiceConfigWithDefaultConfigTest) {
-  StartServers(1);
-  auto channel = BuildChannelWithDefaultServiceConfig();
-  auto stub = BuildStub(channel);
-  SetNextResolutionInvalidServiceConfig(GetServersPorts());
-  CheckRpcSendOk(stub, DEBUG_LOCATION);
-  EXPECT_STREQ(ValidDefaultServiceConfig(),
-               channel->GetServiceConfigJSON().c_str());
-}
-
 TEST_F(ServiceConfigEnd2endTest, ValidServiceConfigUpdatesTest) {
   StartServers(1);
   auto channel = BuildChannel();
@@ -490,7 +480,7 @@ TEST_F(ServiceConfigEnd2endTest,
   EXPECT_STREQ(ValidServiceConfigV1(), channel->GetServiceConfigJSON().c_str());
   SetNextResolutionNoServiceConfig(GetServersPorts());
   CheckRpcSendOk(stub, DEBUG_LOCATION);
-  EXPECT_STREQ("", channel->GetServiceConfigJSON().c_str());
+  EXPECT_STREQ("{}", channel->GetServiceConfigJSON().c_str());
 }
 
 TEST_F(ServiceConfigEnd2endTest,
@@ -552,7 +542,7 @@ TEST_F(ServiceConfigEnd2endTest, NoServiceConfigAfterInvalidServiceConfigTest) {
   CheckRpcSendFailure(stub);
   SetNextResolutionNoServiceConfig(GetServersPorts());
   CheckRpcSendOk(stub, DEBUG_LOCATION);
-  EXPECT_STREQ("", channel->GetServiceConfigJSON().c_str());
+  EXPECT_STREQ("{}", channel->GetServiceConfigJSON().c_str());
 }
 
 TEST_F(ServiceConfigEnd2endTest,