From 2c084ff7a5d92306710d3fcd5b81a905235bae47 Mon Sep 17 00:00:00 2001 From: "Mark D. Roth" Date: Mon, 21 Dec 2020 14:40:04 -0800 Subject: [PATCH] Use std::string for health check service name. --- .../filters/client_channel/client_channel.cc | 46 ++++++++----------- .../health/health_check_client.cc | 9 ++-- .../health/health_check_client.h | 4 +- .../client_channel/resolver_result_parsing.cc | 24 +++++----- .../client_channel/resolver_result_parsing.h | 16 +++---- .../ext/filters/client_channel/subchannel.cc | 46 +++++++++---------- .../ext/filters/client_channel/subchannel.h | 17 +++---- .../client_channel/service_config_test.cc | 4 +- 8 files changed, 79 insertions(+), 87 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 1934db07ade..e196b2250f0 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -356,7 +356,7 @@ class ChannelData { bool previous_resolution_contained_addresses_ = false; RefCountedPtr saved_service_config_; RefCountedPtr saved_config_selector_; - UniquePtr health_check_service_name_; + absl::optional health_check_service_name_; OrphanablePtr lb_policy_; RefCountedPtr subchannel_pool_; // The number of SubchannelWrapper instances referencing a given Subchannel. @@ -1052,7 +1052,7 @@ class LoadBalancedCall { class ChannelData::SubchannelWrapper : public SubchannelInterface { public: SubchannelWrapper(ChannelData* chand, Subchannel* subchannel, - UniquePtr health_check_service_name) + absl::optional health_check_service_name) : SubchannelInterface( GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace) ? "SubchannelWrapper" @@ -1102,7 +1102,7 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface { grpc_connectivity_state CheckConnectivityState() override { RefCountedPtr connected_subchannel; grpc_connectivity_state connectivity_state = - subchannel_->CheckConnectivityState(health_check_service_name_.get(), + subchannel_->CheckConnectivityState(health_check_service_name_, &connected_subchannel); MaybeUpdateConnectedSubchannel(std::move(connected_subchannel)); return connectivity_state; @@ -1117,8 +1117,7 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface { Ref(DEBUG_LOCATION, "WatcherWrapper"), initial_state); subchannel_->WatchConnectivityState( - initial_state, - UniquePtr(gpr_strdup(health_check_service_name_.get())), + initial_state, health_check_service_name_, RefCountedPtr( watcher_wrapper)); } @@ -1127,7 +1126,7 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface { ConnectivityStateWatcherInterface* watcher) override { auto it = watcher_map_.find(watcher); GPR_ASSERT(it != watcher_map_.end()); - subchannel_->CancelConnectivityStateWatch(health_check_service_name_.get(), + subchannel_->CancelConnectivityStateWatch(health_check_service_name_, it->second); watcher_map_.erase(it); } @@ -1144,13 +1143,14 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface { subchannel_->ThrottleKeepaliveTime(new_keepalive_time); } - void UpdateHealthCheckServiceName(UniquePtr health_check_service_name) { + void UpdateHealthCheckServiceName( + absl::optional health_check_service_name) { if (GRPC_TRACE_FLAG_ENABLED(grpc_client_channel_routing_trace)) { gpr_log(GPR_INFO, "chand=%p: subchannel wrapper %p: updating health check service " "name from \"%s\" to \"%s\"", - chand_, this, health_check_service_name_.get(), - health_check_service_name.get()); + chand_, this, health_check_service_name_->c_str(), + health_check_service_name->c_str()); } for (auto& p : watcher_map_) { WatcherWrapper*& watcher_wrapper = p.second; @@ -1165,12 +1165,11 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface { // problem, we may be able to handle it by waiting for the new // watcher to report READY before we use it to replace the old one. WatcherWrapper* replacement = watcher_wrapper->MakeReplacement(); - subchannel_->CancelConnectivityStateWatch( - health_check_service_name_.get(), watcher_wrapper); + subchannel_->CancelConnectivityStateWatch(health_check_service_name_, + watcher_wrapper); watcher_wrapper = replacement; subchannel_->WatchConnectivityState( - replacement->last_seen_state(), - UniquePtr(gpr_strdup(health_check_service_name.get())), + replacement->last_seen_state(), health_check_service_name, RefCountedPtr( replacement)); } @@ -1334,7 +1333,7 @@ class ChannelData::SubchannelWrapper : public SubchannelInterface { ChannelData* chand_; Subchannel* subchannel_; - UniquePtr health_check_service_name_; + absl::optional health_check_service_name_; // Maps from the address of the watcher passed to us by the LB policy // to the address of the WrapperWatcher that we passed to the underlying // subchannel. This is needed so that when the LB policy calls @@ -1527,10 +1526,9 @@ class ChannelData::ClientChannelControlHelper // Determine health check service name. bool inhibit_health_checking = grpc_channel_arg_get_bool( grpc_channel_args_find(&args, GRPC_ARG_INHIBIT_HEALTH_CHECKING), false); - UniquePtr health_check_service_name; + absl::optional health_check_service_name; if (!inhibit_health_checking) { - health_check_service_name.reset( - gpr_strdup(chand_->health_check_service_name_.get())); + health_check_service_name = chand_->health_check_service_name_; } // Remove channel args that should not affect subchannel uniqueness. static const char* args_to_remove[] = { @@ -2044,17 +2042,14 @@ void ChannelData::UpdateServiceConfigInControlPlaneLocked( // Save service config. saved_service_config_ = std::move(service_config); // Update health check service name if needed. - if (((health_check_service_name_ == nullptr) != - (parsed_service_config->health_check_service_name() == nullptr)) || - (health_check_service_name_ != nullptr && - strcmp(health_check_service_name_.get(), - parsed_service_config->health_check_service_name()) != 0)) { - health_check_service_name_.reset( - gpr_strdup(parsed_service_config->health_check_service_name())); + if (health_check_service_name_ != + parsed_service_config->health_check_service_name()) { + health_check_service_name_ = + 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( - UniquePtr(gpr_strdup(health_check_service_name_.get()))); + health_check_service_name_); } } // Swap out the data used by GetChannelInfo(). @@ -2172,7 +2167,6 @@ void ChannelData::UpdateStateAndPickerLocked( std::unique_ptr picker) { // Clean the control plane when entering IDLE. if (picker == nullptr || state == GRPC_CHANNEL_SHUTDOWN) { - health_check_service_name_.reset(); saved_service_config_.reset(); saved_config_selector_.reset(); } diff --git a/src/core/ext/filters/client_channel/health/health_check_client.cc b/src/core/ext/filters/client_channel/health/health_check_client.cc index 0104395afdc..7885fadd2c3 100644 --- a/src/core/ext/filters/client_channel/health/health_check_client.cc +++ b/src/core/ext/filters/client_channel/health/health_check_client.cc @@ -46,7 +46,7 @@ TraceFlag grpc_health_check_client_trace(false, "health_check_client"); // HealthCheckClient::HealthCheckClient( - const char* service_name, + std::string service_name, RefCountedPtr connected_subchannel, grpc_pollset_set* interested_parties, RefCountedPtr channelz_node, @@ -55,7 +55,7 @@ HealthCheckClient::HealthCheckClient( GRPC_TRACE_FLAG_ENABLED(grpc_health_check_client_trace) ? "HealthCheckClient" : nullptr), - service_name_(service_name), + service_name_(std::move(service_name)), connected_subchannel_(std::move(connected_subchannel)), interested_parties_(interested_parties), channelz_node_(std::move(channelz_node)), @@ -180,13 +180,14 @@ void HealthCheckClient::OnRetryTimer(void* arg, grpc_error* error) { namespace { -void EncodeRequest(const char* service_name, +void EncodeRequest(const std::string& service_name, ManualConstructor* send_message) { upb::Arena arena; grpc_health_v1_HealthCheckRequest* request_struct = grpc_health_v1_HealthCheckRequest_new(arena.ptr()); grpc_health_v1_HealthCheckRequest_set_service( - request_struct, upb_strview_makez(service_name)); + request_struct, + upb_strview_make(service_name.data(), service_name.size())); size_t buf_length; char* buf = grpc_health_v1_HealthCheckRequest_serialize( request_struct, arena.ptr(), &buf_length); diff --git a/src/core/ext/filters/client_channel/health/health_check_client.h b/src/core/ext/filters/client_channel/health/health_check_client.h index 24b59c7c5f4..0fc39b0410b 100644 --- a/src/core/ext/filters/client_channel/health/health_check_client.h +++ b/src/core/ext/filters/client_channel/health/health_check_client.h @@ -44,7 +44,7 @@ namespace grpc_core { class HealthCheckClient : public InternallyRefCounted { public: - HealthCheckClient(const char* service_name, + HealthCheckClient(std::string service_name, RefCountedPtr connected_subchannel, grpc_pollset_set* interested_parties, RefCountedPtr channelz_node, @@ -150,7 +150,7 @@ class HealthCheckClient : public InternallyRefCounted { void SetHealthStatusLocked(grpc_connectivity_state state, const char* reason); // Requires holding mu_. - const char* service_name_; // Do not own. + std::string service_name_; RefCountedPtr connected_subchannel_; grpc_pollset_set* interested_parties_; // Do not own. RefCountedPtr channelz_node_; diff --git a/src/core/ext/filters/client_channel/resolver_result_parsing.cc b/src/core/ext/filters/client_channel/resolver_result_parsing.cc index c716bc5720d..500c7408a15 100644 --- a/src/core/ext/filters/client_channel/resolver_result_parsing.cc +++ b/src/core/ext/filters/client_channel/resolver_result_parsing.cc @@ -248,27 +248,25 @@ grpc_error* ParseRetryThrottling( return GRPC_ERROR_CREATE_FROM_VECTOR("retryPolicy", &error_list); } -const char* ParseHealthCheckConfig(const Json& field, grpc_error** error) { +absl::optional ParseHealthCheckConfig(const Json& field, + grpc_error** error) { GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE); - const char* service_name = nullptr; if (field.type() != Json::Type::OBJECT) { *error = GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:healthCheckConfig error:should be of type object"); - return nullptr; + return absl::nullopt; } std::vector error_list; + absl::optional service_name; auto it = field.object_value().find("serviceName"); if (it != field.object_value().end()) { if (it->second.type() != Json::Type::STRING) { error_list.push_back(GRPC_ERROR_CREATE_FROM_STATIC_STRING( "field:serviceName error:should be of type string")); } else { - service_name = it->second.string_value().c_str(); + service_name = it->second.string_value(); } } - if (!error_list.empty()) { - return nullptr; - } *error = GRPC_ERROR_CREATE_FROM_VECTOR("field:healthCheckConfig", &error_list); return service_name; @@ -281,12 +279,8 @@ ClientChannelServiceConfigParser::ParseGlobalParams( const grpc_channel_args* /*args*/, const Json& json, grpc_error** error) { GPR_DEBUG_ASSERT(error != nullptr && *error == GRPC_ERROR_NONE); std::vector error_list; - RefCountedPtr parsed_lb_config; - std::string lb_policy_name; - absl::optional - retry_throttling; - const char* health_check_service_name = nullptr; // Parse LB config. + RefCountedPtr parsed_lb_config; auto it = json.object_value().find("loadBalancingConfig"); if (it != json.object_value().end()) { grpc_error* parse_error = GRPC_ERROR_NONE; @@ -300,6 +294,7 @@ ClientChannelServiceConfigParser::ParseGlobalParams( } } // Parse deprecated LB policy. + std::string lb_policy_name; it = json.object_value().find("loadBalancingPolicy"); if (it != json.object_value().end()) { if (it->second.type() != Json::Type::STRING) { @@ -325,6 +320,8 @@ ClientChannelServiceConfigParser::ParseGlobalParams( } } // Parse retry throttling. + absl::optional + retry_throttling; it = json.object_value().find("retryThrottling"); if (it != json.object_value().end()) { ClientChannelGlobalParsedConfig::RetryThrottling data; @@ -336,6 +333,7 @@ ClientChannelServiceConfigParser::ParseGlobalParams( } } // Parse health check config. + absl::optional health_check_service_name; it = json.object_value().find("healthCheckConfig"); if (it != json.object_value().end()) { grpc_error* parsing_error = GRPC_ERROR_NONE; @@ -350,7 +348,7 @@ ClientChannelServiceConfigParser::ParseGlobalParams( if (*error == GRPC_ERROR_NONE) { return absl::make_unique( std::move(parsed_lb_config), std::move(lb_policy_name), - retry_throttling, health_check_service_name); + retry_throttling, std::move(health_check_service_name)); } return nullptr; } diff --git a/src/core/ext/filters/client_channel/resolver_result_parsing.h b/src/core/ext/filters/client_channel/resolver_result_parsing.h index a54209c8e82..cdf89d35ce1 100644 --- a/src/core/ext/filters/client_channel/resolver_result_parsing.h +++ b/src/core/ext/filters/client_channel/resolver_result_parsing.h @@ -49,15 +49,11 @@ class ClientChannelGlobalParsedConfig RefCountedPtr parsed_lb_config, std::string parsed_deprecated_lb_policy, const absl::optional& retry_throttling, - const char* health_check_service_name) + absl::optional health_check_service_name) : parsed_lb_config_(std::move(parsed_lb_config)), parsed_deprecated_lb_policy_(std::move(parsed_deprecated_lb_policy)), retry_throttling_(retry_throttling), - health_check_service_name_(health_check_service_name) {} - - absl::optional retry_throttling() const { - return retry_throttling_; - } + health_check_service_name_(std::move(health_check_service_name)) {} RefCountedPtr parsed_lb_config() const { return parsed_lb_config_; @@ -67,7 +63,11 @@ class ClientChannelGlobalParsedConfig return parsed_deprecated_lb_policy_; } - const char* health_check_service_name() const { + absl::optional retry_throttling() const { + return retry_throttling_; + } + + const absl::optional& health_check_service_name() const { return health_check_service_name_; } @@ -75,7 +75,7 @@ class ClientChannelGlobalParsedConfig RefCountedPtr parsed_lb_config_; std::string parsed_deprecated_lb_policy_; absl::optional retry_throttling_; - const char* health_check_service_name_; + absl::optional health_check_service_name_; }; class ClientChannelMethodParsedConfig diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index d6f9e1fbbc4..dbac59afc7b 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -424,8 +424,7 @@ void Subchannel::ConnectivityStateWatcherList::NotifyLocked( class Subchannel::HealthWatcherMap::HealthWatcher : public AsyncConnectivityStateWatcherInterface { public: - HealthWatcher(Subchannel* c, - grpc_core::UniquePtr health_check_service_name, + HealthWatcher(Subchannel* c, std::string health_check_service_name, grpc_connectivity_state subchannel_state) : subchannel_(c), health_check_service_name_(std::move(health_check_service_name)), @@ -440,8 +439,8 @@ class Subchannel::HealthWatcherMap::HealthWatcher GRPC_SUBCHANNEL_WEAK_UNREF(subchannel_, "health_watcher"); } - const char* health_check_service_name() const { - return health_check_service_name_.get(); + const std::string& health_check_service_name() const { + return health_check_service_name_; } grpc_connectivity_state state() const { return state_; } @@ -504,12 +503,12 @@ class Subchannel::HealthWatcherMap::HealthWatcher void StartHealthCheckingLocked() { GPR_ASSERT(health_check_client_ == nullptr); health_check_client_ = MakeOrphanable( - health_check_service_name_.get(), subchannel_->connected_subchannel_, + health_check_service_name_, subchannel_->connected_subchannel_, subchannel_->pollset_set_, subchannel_->channelz_node_, Ref()); } Subchannel* subchannel_; - grpc_core::UniquePtr health_check_service_name_; + std::string health_check_service_name_; OrphanablePtr health_check_client_; grpc_connectivity_state state_; absl::Status status_; @@ -522,18 +521,17 @@ class Subchannel::HealthWatcherMap::HealthWatcher void Subchannel::HealthWatcherMap::AddWatcherLocked( Subchannel* subchannel, grpc_connectivity_state initial_state, - grpc_core::UniquePtr health_check_service_name, + const std::string& health_check_service_name, RefCountedPtr watcher) { // If the health check service name is not already present in the map, // add it. - auto it = map_.find(health_check_service_name.get()); + auto it = map_.find(health_check_service_name); HealthWatcher* health_watcher; if (it == map_.end()) { - const char* key = health_check_service_name.get(); auto w = MakeOrphanable( - subchannel, std::move(health_check_service_name), subchannel->state_); + subchannel, health_check_service_name, subchannel->state_); health_watcher = w.get(); - map_[key] = std::move(w); + map_.emplace(health_check_service_name, std::move(w)); } else { health_watcher = it->second.get(); } @@ -542,7 +540,7 @@ void Subchannel::HealthWatcherMap::AddWatcherLocked( } void Subchannel::HealthWatcherMap::RemoveWatcherLocked( - const char* health_check_service_name, + const std::string& health_check_service_name, ConnectivityStateWatcherInterface* watcher) { auto it = map_.find(health_check_service_name); GPR_ASSERT(it != map_.end()); @@ -561,7 +559,7 @@ void Subchannel::HealthWatcherMap::NotifyLocked(grpc_connectivity_state state, grpc_connectivity_state Subchannel::HealthWatcherMap::CheckConnectivityStateLocked( - Subchannel* subchannel, const char* health_check_service_name) { + Subchannel* subchannel, const std::string& health_check_service_name) { auto it = map_.find(health_check_service_name); if (it == map_.end()) { // If the health check service name is not found in the map, we're @@ -824,15 +822,15 @@ channelz::SubchannelNode* Subchannel::channelz_node() { } grpc_connectivity_state Subchannel::CheckConnectivityState( - const char* health_check_service_name, + const absl::optional& health_check_service_name, RefCountedPtr* connected_subchannel) { MutexLock lock(&mu_); grpc_connectivity_state state; - if (health_check_service_name == nullptr) { + if (!health_check_service_name.has_value()) { state = state_; } else { state = health_watcher_map_.CheckConnectivityStateLocked( - this, health_check_service_name); + this, *health_check_service_name); } if (connected_subchannel != nullptr && state == GRPC_CHANNEL_READY) { *connected_subchannel = connected_subchannel_; @@ -842,37 +840,37 @@ grpc_connectivity_state Subchannel::CheckConnectivityState( void Subchannel::WatchConnectivityState( grpc_connectivity_state initial_state, - grpc_core::UniquePtr health_check_service_name, + const absl::optional& health_check_service_name, RefCountedPtr watcher) { MutexLock lock(&mu_); grpc_pollset_set* interested_parties = watcher->interested_parties(); if (interested_parties != nullptr) { grpc_pollset_set_add_pollset_set(pollset_set_, interested_parties); } - if (health_check_service_name == nullptr) { + if (!health_check_service_name.has_value()) { if (state_ != initial_state) { new AsyncWatcherNotifierLocked(watcher, this, state_, status_); } watcher_list_.AddWatcherLocked(std::move(watcher)); } else { - health_watcher_map_.AddWatcherLocked(this, initial_state, - std::move(health_check_service_name), - std::move(watcher)); + health_watcher_map_.AddWatcherLocked( + this, initial_state, *health_check_service_name, std::move(watcher)); } } void Subchannel::CancelConnectivityStateWatch( - const char* health_check_service_name, + const absl::optional& health_check_service_name, ConnectivityStateWatcherInterface* watcher) { MutexLock lock(&mu_); grpc_pollset_set* interested_parties = watcher->interested_parties(); if (interested_parties != nullptr) { grpc_pollset_set_del_pollset_set(pollset_set_, interested_parties); } - if (health_check_service_name == nullptr) { + if (!health_check_service_name.has_value()) { watcher_list_.RemoveWatcherLocked(watcher); } else { - health_watcher_map_.RemoveWatcherLocked(health_check_service_name, watcher); + health_watcher_map_.RemoveWatcherLocked(*health_check_service_name, + watcher); } } diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h index 5db222e2613..cb346dd84ca 100644 --- a/src/core/ext/filters/client_channel/subchannel.h +++ b/src/core/ext/filters/client_channel/subchannel.h @@ -251,7 +251,7 @@ class Subchannel { // service name. // If the return value is GRPC_CHANNEL_READY, also sets *connected_subchannel. grpc_connectivity_state CheckConnectivityState( - const char* health_check_service_name, + const absl::optional& health_check_service_name, RefCountedPtr* connected_subchannel); // Starts watching the subchannel's connectivity state. @@ -264,13 +264,14 @@ class Subchannel { // destroyed or when CancelConnectivityStateWatch() is called. void WatchConnectivityState( grpc_connectivity_state initial_state, - grpc_core::UniquePtr health_check_service_name, + const absl::optional& health_check_service_name, RefCountedPtr watcher); // Cancels a connectivity state watch. // If the watcher has already been destroyed, this is a no-op. - void CancelConnectivityStateWatch(const char* health_check_service_name, - ConnectivityStateWatcherInterface* watcher); + void CancelConnectivityStateWatch( + const absl::optional& health_check_service_name, + ConnectivityStateWatcherInterface* watcher); // Attempt to connect to the backend. Has no effect if already connected. void AttemptToConnect(); @@ -334,9 +335,9 @@ class Subchannel { public: void AddWatcherLocked( Subchannel* subchannel, grpc_connectivity_state initial_state, - grpc_core::UniquePtr health_check_service_name, + const std::string& health_check_service_name, RefCountedPtr watcher); - void RemoveWatcherLocked(const char* health_check_service_name, + void RemoveWatcherLocked(const std::string& health_check_service_name, ConnectivityStateWatcherInterface* watcher); // Notifies the watcher when the subchannel's state changes. @@ -344,14 +345,14 @@ class Subchannel { const absl::Status& status); grpc_connectivity_state CheckConnectivityStateLocked( - Subchannel* subchannel, const char* health_check_service_name); + Subchannel* subchannel, const std::string& health_check_service_name); void ShutdownLocked(); private: class HealthWatcher; - std::map, StringLess> map_; + std::map> map_; }; class ConnectedSubchannelStateWatcher; diff --git a/test/core/client_channel/service_config_test.cc b/test/core/client_channel/service_config_test.cc index b8449d2bd64..cc170c008e9 100644 --- a/test/core/client_channel/service_config_test.cc +++ b/test/core/client_channel/service_config_test.cc @@ -991,8 +991,8 @@ TEST_F(ClientChannelParserTest, ValidHealthCheck) { static_cast( svc_cfg->GetGlobalParsedConfig(0)); ASSERT_NE(parsed_config, nullptr); - EXPECT_STREQ(parsed_config->health_check_service_name(), - "health_check_service_name"); + EXPECT_EQ(parsed_config->health_check_service_name(), + "health_check_service_name"); } TEST_F(ClientChannelParserTest, InvalidHealthCheckMultipleEntries) {