From 92c4dffc174491d056207699fb93731d942cd7cf Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 16 Jan 2019 15:10:48 -0800 Subject: [PATCH 1/4] Remove uneeded lock --- src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc | 1 - 1 file changed, 1 deletion(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 40bf9c65644..31b454098e1 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -1208,7 +1208,6 @@ void GrpcLb::FillChildRefsForChannelz( channelz::ChildRefsList* child_channels) { // delegate to the RoundRobin to fill the children subchannels. rr_policy_->FillChildRefsForChannelz(child_subchannels, child_channels); - MutexLock lock(&lb_channel_mu_); if (lb_channel_ != nullptr) { grpc_core::channelz::ChannelNode* channel_node = grpc_channel_get_channelz_node(lb_channel_); From 6e3fee6f2aeeccca2b6e63b7be2a72c0db51bd84 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 16 Jan 2019 15:27:21 -0800 Subject: [PATCH 2/4] Add additional nullptr check --- .../ext/filters/client_channel/lb_policy/grpclb/grpclb.cc | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 31b454098e1..78de8b35659 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -1207,7 +1207,9 @@ void GrpcLb::FillChildRefsForChannelz( channelz::ChildRefsList* child_subchannels, channelz::ChildRefsList* child_channels) { // delegate to the RoundRobin to fill the children subchannels. - rr_policy_->FillChildRefsForChannelz(child_subchannels, child_channels); + if (rr_policy_ != nullptr) { + rr_policy_->FillChildRefsForChannelz(child_subchannels, child_channels); + } if (lb_channel_ != nullptr) { grpc_core::channelz::ChannelNode* channel_node = grpc_channel_get_channelz_node(lb_channel_); From e3f1f3c8568314bd777f78883fa56520e0b7aee2 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 17 Jan 2019 09:39:23 -0800 Subject: [PATCH 3/4] Atomically store uuid of lb channel --- .../client_channel/lb_policy/grpclb/grpclb.cc | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 78de8b35659..6d46baa08fe 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -296,9 +296,8 @@ class GrpcLb : public LoadBalancingPolicy { // The channel for communicating with the LB server. grpc_channel* lb_channel_ = nullptr; - // Mutex to protect the channel to the LB server. This is used when - // processing a channelz request. - gpr_mu lb_channel_mu_; + // Uuid of the lb channel. Used for channelz. + gpr_atm lb_channel_uuid_ = 0; grpc_connectivity_state lb_channel_connectivity_; grpc_closure lb_channel_on_connectivity_changed_; // Are we already watching the LB channel's connectivity? @@ -986,7 +985,6 @@ GrpcLb::GrpcLb(const LoadBalancingPolicy::Args& args) .set_max_backoff(GRPC_GRPCLB_RECONNECT_MAX_BACKOFF_SECONDS * 1000)) { // Initialization. - gpr_mu_init(&lb_channel_mu_); GRPC_CLOSURE_INIT(&lb_channel_on_connectivity_changed_, &GrpcLb::OnBalancerChannelConnectivityChangedLocked, this, grpc_combiner_scheduler(args.combiner)); @@ -1023,7 +1021,6 @@ GrpcLb::GrpcLb(const LoadBalancingPolicy::Args& args) GrpcLb::~GrpcLb() { GPR_ASSERT(pending_picks_ == nullptr); - gpr_mu_destroy(&lb_channel_mu_); gpr_free((void*)server_name_); grpc_channel_args_destroy(args_); grpc_connectivity_state_destroy(&state_tracker_); @@ -1049,10 +1046,9 @@ void GrpcLb::ShutdownLocked() { // OnBalancerChannelConnectivityChangedLocked(), and we need to be // alive when that callback is invoked. if (lb_channel_ != nullptr) { - gpr_mu_lock(&lb_channel_mu_); grpc_channel_destroy(lb_channel_); lb_channel_ = nullptr; - gpr_mu_unlock(&lb_channel_mu_); + gpr_atm_no_barrier_store(&lb_channel_uuid_, 0); } grpc_connectivity_state_set(&state_tracker_, GRPC_CHANNEL_SHUTDOWN, GRPC_ERROR_REF(error), "grpclb_shutdown"); @@ -1210,12 +1206,8 @@ void GrpcLb::FillChildRefsForChannelz( if (rr_policy_ != nullptr) { rr_policy_->FillChildRefsForChannelz(child_subchannels, child_channels); } - if (lb_channel_ != nullptr) { - grpc_core::channelz::ChannelNode* channel_node = - grpc_channel_get_channelz_node(lb_channel_); - if (channel_node != nullptr) { - child_channels->push_back(channel_node->uuid()); - } + if (lb_channel_uuid_ != 0) { + child_channels->push_back(lb_channel_uuid_); } } @@ -1275,12 +1267,15 @@ void GrpcLb::ProcessChannelArgsLocked(const grpc_channel_args& args) { if (lb_channel_ == nullptr) { char* uri_str; gpr_asprintf(&uri_str, "fake:///%s", server_name_); - gpr_mu_lock(&lb_channel_mu_); lb_channel_ = grpc_client_channel_factory_create_channel( client_channel_factory(), uri_str, GRPC_CLIENT_CHANNEL_TYPE_LOAD_BALANCING, lb_channel_args); - gpr_mu_unlock(&lb_channel_mu_); GPR_ASSERT(lb_channel_ != nullptr); + grpc_core::channelz::ChannelNode* channel_node = + grpc_channel_get_channelz_node(lb_channel_); + if (channel_node != nullptr) { + gpr_atm_no_barrier_store(&lb_channel_uuid_, channel_node->uuid()); + } gpr_free(uri_str); } // Propagate updates to the LB channel (pick_first) through the fake From 79b9707db4469aa48af694689e73157b7cd16864 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 17 Jan 2019 09:51:07 -0800 Subject: [PATCH 4/4] reviewer feedback --- .../ext/filters/client_channel/lb_policy/grpclb/grpclb.cc | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc index 6d46baa08fe..51b61ecb92c 100644 --- a/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc +++ b/src/core/ext/filters/client_channel/lb_policy/grpclb/grpclb.cc @@ -1206,8 +1206,9 @@ void GrpcLb::FillChildRefsForChannelz( if (rr_policy_ != nullptr) { rr_policy_->FillChildRefsForChannelz(child_subchannels, child_channels); } - if (lb_channel_uuid_ != 0) { - child_channels->push_back(lb_channel_uuid_); + gpr_atm uuid = gpr_atm_no_barrier_load(&lb_channel_uuid_); + if (uuid != 0) { + child_channels->push_back(uuid); } }