From 84c8f7051a4cc6d5a0b7a4855e8ffedac78b05fc Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Tue, 15 Sep 2020 16:49:52 -0700 Subject: [PATCH] Simplify service config processing and fix config selector handling. --- .../filters/client_channel/client_channel.cc | 262 +++++++----------- .../filters/client_channel/config_selector.h | 13 +- .../client_channel/resolving_lb_policy.cc | 86 +++--- .../client_channel/resolving_lb_policy.h | 14 +- .../filters/client_channel/service_config.cc | 5 +- .../end2end/service_config_end2end_test.cc | 16 +- 6 files changed, 160 insertions(+), 236 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 24237d8fb29..c4089151b26 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/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 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* lb_policy_config); @@ -267,9 +265,12 @@ class ChannelData { const char* reason, std::unique_ptr picker); - void UpdateServiceConfigInDataPlaneLocked( - bool service_config_changed, - RefCountedPtr config_selector); + void UpdateServiceConfigInControlPlaneLocked( + RefCountedPtr 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 health_check_service_name_; RefCountedPtr saved_service_config_; RefCountedPtr saved_config_selector_; - bool received_first_resolver_result_ = false; // The number of SubchannelWrapper instances referencing a given Subchannel. std::map 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 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 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( - service_config->GetGlobalParsedConfig( - internal::ClientChannelServiceConfigParser::ParserIndex())); - } + static_cast( + 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( - 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 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 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* 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 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 service_config, + const internal::ClientChannelGlobalParsedConfig* parsed_service_config, + const char* lb_policy_name, const grpc_channel_args* args) { + grpc_core::UniquePtr 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( + gpr_strdup(health_check_service_name_.get()))); + } + // Swap out the data used by GetChannelInfo(). + grpc_core::UniquePtr 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( + saved_service_config_->GetGlobalParsedConfig( + internal::ClientChannelServiceConfigParser::ParserIndex())); + absl::optional + retry_throttle_config = parsed_service_config->retry_throttling(); RefCountedPtr retry_throttle_data; - if (saved_service_config_ != nullptr) { - const internal::ClientChannelGlobalParsedConfig* parsed_service_config = - static_cast( - saved_service_config_->GetGlobalParsedConfig( - internal::ClientChannelServiceConfigParser::ParserIndex())); - if (parsed_service_config != nullptr) { - absl::optional - 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 service_config = saved_service_config_; + // Grab ref to config selector. Use default if resolver didn't supply one. + RefCountedPtr config_selector = saved_config_selector_; if (config_selector == nullptr) { config_selector = MakeRefCounted(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 service_config_to_unref = saved_service_config_; - RefCountedPtr 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; diff --git a/src/core/ext/filters/client_channel/config_selector.h b/src/core/ext/filters/client_channel/config_selector.h index e3a65ef330d..895e15f56f2 100644 --- a/src/core/ext/filters/client_channel/config_selector.h +++ b/src/core/ext/filters/client_channel/config_selector.h @@ -76,14 +76,17 @@ class ConfigSelector : public RefCounted { class DefaultConfigSelector : public ConfigSelector { public: explicit DefaultConfigSelector(RefCountedPtr 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; } diff --git a/src/core/ext/filters/client_channel/resolving_lb_policy.cc b/src/core/ext/filters/client_channel/resolving_lb_policy.cc index 6f14c1bcc65..5d1d0f0eb4d 100644 --- a/src/core/ext/filters/client_channel/resolving_lb_policy.cc +++ b/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 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 diff --git a/src/core/ext/filters/client_channel/resolving_lb_policy.h b/src/core/ext/filters/client_channel/resolving_lb_policy.h index b44dc1a94e3..2835c84d707 100644 --- a/src/core/ext/filters/client_channel/resolving_lb_policy.h +++ b/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 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 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; diff --git a/src/core/ext/filters/client_channel/service_config.cc b/src/core/ext/filters/client_channel/service_config.cc index a8c0d93fd71..b162ccef2bd 100644 --- a/src/core/ext/filters/client_channel/service_config.cc +++ b/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 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; diff --git a/test/cpp/end2end/service_config_end2end_test.cc b/test/cpp/end2end/service_config_end2end_test.cc index 5e7f9f19ebf..828a7201314 100644 --- a/test/cpp/end2end/service_config_end2end_test.cc +++ b/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,