From aa7b8e5bc6b1ce0ea74ae2ac290ebf031b2969e2 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Tue, 16 Oct 2018 17:50:04 -0700 Subject: [PATCH 1/6] Start of channelz resolution support --- .../filters/client_channel/client_channel.cc | 43 ++++++++++++++++++- .../ext/filters/client_channel/lb_policy.h | 4 +- .../client_channel/lb_policy/grpclb/grpclb.cc | 5 ++- .../lb_policy/pick_first/pick_first.cc | 13 +++--- .../lb_policy/round_robin/round_robin.cc | 7 +-- .../client_channel/lb_policy/xds/xds.cc | 5 ++- 6 files changed, 63 insertions(+), 14 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 23da11363b7..0324706170a 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -518,6 +518,14 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { } // Data used to set the channel's connectivity state. bool set_connectivity_state = true; + // We only want to trace the address resolution in the follow cases: + // (a) Address resolution resulted in service config change. + // (b) Address resolution that causes number of backends to go from + // zero to non-zero. + // (c) Address resolution that causes number of backends to go from + // non-zero to zero. + // (d) Address resolution that causes a new LB policy to be created. + bool trace_this_address_resolution = false; grpc_connectivity_state connectivity_state = GRPC_CHANNEL_TRANSIENT_FAILURE; grpc_error* connectivity_error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("No load balancing policy"); @@ -545,23 +553,56 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { gpr_log(GPR_INFO, "chand=%p: updating existing LB policy \"%s\" (%p)", chand, lb_policy_name.get(), chand->lb_policy.get()); } - chand->lb_policy->UpdateLocked(*chand->resolver_result); + // case (b) or (c) + trace_this_address_resolution = + chand->lb_policy->UpdateLocked(*chand->resolver_result); // No need to set the channel's connectivity state; the existing // watch on the LB policy will take care of that. set_connectivity_state = false; } else { + trace_this_address_resolution = true; // case (d) // Instantiate new LB policy. create_new_lb_policy_locked(chand, lb_policy_name.get(), &connectivity_state, &connectivity_error); + // we also log the name of the new LB policy in addition to logging this + // resolution event. + if (chand->channelz_channel != nullptr) { + char* str; + gpr_asprintf(&str, "Switched LB policy to %s", lb_policy_name.get()); + chand->channelz_channel->AddTraceEvent( + grpc_core::channelz::ChannelTrace::Severity::Info, + grpc_slice_from_copied_string(str)); + gpr_free(str); + } } // Find service config. grpc_core::UniquePtr service_config_json = get_service_config_from_resolver_result_locked(chand); + if ((service_config_json == nullptr && + chand->info_service_config_json != nullptr) || + (service_config_json != nullptr && + chand->info_service_config_json == nullptr) || + (service_config_json != nullptr && + chand->info_service_config_json != nullptr && + gpr_stricmp(service_config_json.get(), + chand->info_service_config_json.get()) != 0)) { + trace_this_address_resolution = true; // case (a) + if (chand->channelz_channel != nullptr) { + chand->channelz_channel->AddTraceEvent( + grpc_core::channelz::ChannelTrace::Severity::Info, + grpc_slice_from_static_string("Service config reloaded")); + } + } // Swap out the data used by cc_get_channel_info(). gpr_mu_lock(&chand->info_mu); chand->info_lb_policy_name = std::move(lb_policy_name); chand->info_service_config_json = std::move(service_config_json); gpr_mu_unlock(&chand->info_mu); + if (trace_this_address_resolution && chand->channelz_channel != nullptr) { + chand->channelz_channel->AddTraceEvent( + grpc_core::channelz::ChannelTrace::Severity::Info, + grpc_slice_from_static_string("New addresses resolved")); + } // Clean up. grpc_channel_args_destroy(chand->resolver_result); chand->resolver_result = nullptr; diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h index 21f80b7b947..3940f138e66 100644 --- a/src/core/ext/filters/client_channel/lb_policy.h +++ b/src/core/ext/filters/client_channel/lb_policy.h @@ -95,7 +95,9 @@ class LoadBalancingPolicy /// Updates the policy with a new set of \a args from the resolver. /// Note that the LB policy gets the set of addresses from the /// GRPC_ARG_LB_ADDRESSES channel arg. - virtual void UpdateLocked(const grpc_channel_args& args) GRPC_ABSTRACT; + /// Returns true if the update caused the number of backends to go from + /// zero to non-zero, or non-zero to zero. + virtual bool UpdateLocked(const grpc_channel_args& args) GRPC_ABSTRACT; /// Finds an appropriate subchannel for a call, based on data in \a pick. /// \a pick must remain alive until the pick is complete. 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 5511df7a275..4b49d10d9e0 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 @@ -123,7 +123,7 @@ class GrpcLb : public LoadBalancingPolicy { public: GrpcLb(const grpc_lb_addresses* addresses, const Args& args); - void UpdateLocked(const grpc_channel_args& args) override; + bool UpdateLocked(const grpc_channel_args& args) override; bool PickLocked(PickState* pick, grpc_error** error) override; void CancelPickLocked(PickState* pick, grpc_error* error) override; void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask, @@ -1331,7 +1331,7 @@ void GrpcLb::ProcessChannelArgsLocked(const grpc_channel_args& args) { grpc_channel_args_destroy(lb_channel_args); } -void GrpcLb::UpdateLocked(const grpc_channel_args& args) { +bool GrpcLb::UpdateLocked(const grpc_channel_args& args) { ProcessChannelArgsLocked(args); // If fallback is configured and the RR policy already exists, update // it with the new fallback addresses. @@ -1358,6 +1358,7 @@ void GrpcLb::UpdateLocked(const grpc_channel_args& args) { &lb_channel_connectivity_, &lb_channel_on_connectivity_changed_, nullptr); } + return false; // TODO } // diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc index f4dca146f71..e6f237c768d 100644 --- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc +++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc @@ -46,7 +46,7 @@ class PickFirst : public LoadBalancingPolicy { public: explicit PickFirst(const Args& args); - void UpdateLocked(const grpc_channel_args& args) override; + bool UpdateLocked(const grpc_channel_args& args) override; bool PickLocked(PickState* pick, grpc_error** error) override; void CancelPickLocked(PickState* pick, grpc_error* error) override; void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask, @@ -333,7 +333,7 @@ void PickFirst::UpdateChildRefsLocked() { child_subchannels_ = std::move(cs); } -void PickFirst::UpdateLocked(const grpc_channel_args& args) { +bool PickFirst::UpdateLocked(const grpc_channel_args& args) { AutoChildRefsUpdater guard(this); const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES); if (arg == nullptr || arg->type != GRPC_ARG_POINTER) { @@ -350,10 +350,12 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) { "ignoring.", this); } - return; + return false; } const grpc_lb_addresses* addresses = static_cast(arg->value.pointer.p); + size_t old_size = + subchannel_list_ == nullptr ? 0 : subchannel_list_->num_subchannels(); if (grpc_lb_pick_first_trace.enabled()) { gpr_log(GPR_INFO, "Pick First %p received update with %" PRIuPTR " addresses", this, @@ -371,7 +373,7 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) { "pf_update_empty"); subchannel_list_ = std::move(subchannel_list); // Empty list. selected_ = nullptr; - return; + return old_size != 0; } if (selected_ == nullptr) { // We don't yet have a selected subchannel, so replace the current @@ -411,7 +413,7 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) { // not have contained the currently selected subchannel), drop // it, so that it doesn't override what we've done here. latest_pending_subchannel_list_.reset(); - return; + return false; } GRPC_ERROR_UNREF(error); } @@ -437,6 +439,7 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args) { ->CheckConnectivityStateAndStartWatchingLocked(); } } + return old_size == 0; } void PickFirst::PickFirstSubchannelData::ProcessConnectivityChangeLocked( diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc index e9ed85cf665..76d1d8b1233 100644 --- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc +++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc @@ -57,7 +57,7 @@ class RoundRobin : public LoadBalancingPolicy { public: explicit RoundRobin(const Args& args); - void UpdateLocked(const grpc_channel_args& args) override; + bool UpdateLocked(const grpc_channel_args& args) override; bool PickLocked(PickState* pick, grpc_error** error) override; void CancelPickLocked(PickState* pick, grpc_error* error) override; void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask, @@ -664,7 +664,7 @@ void RoundRobin::NotifyOnStateChangeLocked(grpc_connectivity_state* current, notify); } -void RoundRobin::UpdateLocked(const grpc_channel_args& args) { +bool RoundRobin::UpdateLocked(const grpc_channel_args& args) { const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES); AutoChildRefsUpdater guard(this); if (GPR_UNLIKELY(arg == nullptr || arg->type != GRPC_ARG_POINTER)) { @@ -677,7 +677,7 @@ void RoundRobin::UpdateLocked(const grpc_channel_args& args) { GRPC_ERROR_CREATE_FROM_STATIC_STRING("Missing update in args"), "rr_update_missing"); } - return; + return false; // TODO } grpc_lb_addresses* addresses = static_cast(arg->value.pointer.p); @@ -711,6 +711,7 @@ void RoundRobin::UpdateLocked(const grpc_channel_args& args) { // If we've started picking, start watching the new list. latest_pending_subchannel_list_->StartWatchingLocked(); } + return false; // todo } // diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc index e6c94167f65..aee33b085c3 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc @@ -123,7 +123,7 @@ class XdsLb : public LoadBalancingPolicy { public: XdsLb(const grpc_lb_addresses* addresses, const Args& args); - void UpdateLocked(const grpc_channel_args& args) override; + bool UpdateLocked(const grpc_channel_args& args) override; bool PickLocked(PickState* pick, grpc_error** error) override; void CancelPickLocked(PickState* pick, grpc_error* error) override; void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask, @@ -1325,7 +1325,7 @@ void XdsLb::ProcessChannelArgsLocked(const grpc_channel_args& args) { grpc_channel_args_destroy(lb_channel_args); } -void XdsLb::UpdateLocked(const grpc_channel_args& args) { +bool XdsLb::UpdateLocked(const grpc_channel_args& args) { ProcessChannelArgsLocked(args); // If fallback is configured and the RR policy already exists, update // it with the new fallback addresses. @@ -1352,6 +1352,7 @@ void XdsLb::UpdateLocked(const grpc_channel_args& args) { &lb_channel_connectivity_, &lb_channel_on_connectivity_changed_, nullptr); } + return false; // TODO } // From 265eace8e6375bfd0ec88562e7988ea6de859349 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 17 Oct 2018 11:59:43 -0700 Subject: [PATCH 2/6] reviewer feedback --- .../filters/client_channel/client_channel.cc | 58 ++++++++++--------- .../ext/filters/client_channel/lb_policy.h | 4 +- .../client_channel/lb_policy/grpclb/grpclb.cc | 5 +- .../lb_policy/pick_first/pick_first.cc | 13 ++--- .../lb_policy/round_robin/round_robin.cc | 7 +-- .../client_channel/lb_policy/xds/xds.cc | 5 +- 6 files changed, 44 insertions(+), 48 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 0324706170a..b3ff8653d4f 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -525,7 +525,9 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { // (c) Address resolution that causes number of backends to go from // non-zero to zero. // (d) Address resolution that causes a new LB policy to be created. - bool trace_this_address_resolution = false; + // + // we track a list of strings to eventually be concatenated and traced. + grpc_core::InlinedVector trace_strings; grpc_connectivity_state connectivity_state = GRPC_CHANNEL_TRANSIENT_FAILURE; grpc_error* connectivity_error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("No load balancing policy"); @@ -553,44 +555,51 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { gpr_log(GPR_INFO, "chand=%p: updating existing LB policy \"%s\" (%p)", chand, lb_policy_name.get(), chand->lb_policy.get()); } - // case (b) or (c) - trace_this_address_resolution = - chand->lb_policy->UpdateLocked(*chand->resolver_result); + chand->lb_policy->UpdateLocked(*chand->resolver_result); // No need to set the channel's connectivity state; the existing // watch on the LB policy will take care of that. set_connectivity_state = false; } else { - trace_this_address_resolution = true; // case (d) // Instantiate new LB policy. create_new_lb_policy_locked(chand, lb_policy_name.get(), &connectivity_state, &connectivity_error); - // we also log the name of the new LB policy in addition to logging this - // resolution event. if (chand->channelz_channel != nullptr) { char* str; gpr_asprintf(&str, "Switched LB policy to %s", lb_policy_name.get()); - chand->channelz_channel->AddTraceEvent( - grpc_core::channelz::ChannelTrace::Severity::Info, - grpc_slice_from_copied_string(str)); - gpr_free(str); + trace_strings.push_back(str); } } // Find service config. grpc_core::UniquePtr service_config_json = get_service_config_from_resolver_result_locked(chand); - if ((service_config_json == nullptr && - chand->info_service_config_json != nullptr) || - (service_config_json != nullptr && - chand->info_service_config_json == nullptr) || - (service_config_json != nullptr && - chand->info_service_config_json != nullptr && - gpr_stricmp(service_config_json.get(), - chand->info_service_config_json.get()) != 0)) { - trace_this_address_resolution = true; // case (a) - if (chand->channelz_channel != nullptr) { + // Note: It's safe to use chand->info_lb_policy_name here without + // taking a lock on chand->info_mu, because this function is the + // only thing that modifies its value, and it can only be invoked + // once at any given time. + if (chand->channelz_channel != nullptr) { + if ((service_config_json == nullptr && + chand->info_service_config_json != nullptr) || + (service_config_json != nullptr && + chand->info_service_config_json == nullptr) || + (service_config_json != nullptr && + chand->info_service_config_json != nullptr && + strcmp(service_config_json.get(), + chand->info_service_config_json.get()) != 0)) { + trace_strings.push_back(gpr_strdup("Service config reloaded")); + } + //TODO + // const grpc_arg* channel_arg = + // grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES); + // if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) { + // grpc_lb_addresses* addresses = + // static_cast(channel_arg->value.pointer.p); + // } else { + // } + if (!trace_strings.empty()) { + // TODO, concatenate the strings from trace_strings. chand->channelz_channel->AddTraceEvent( grpc_core::channelz::ChannelTrace::Severity::Info, - grpc_slice_from_static_string("Service config reloaded")); + grpc_slice_from_static_string("New addresses resolved")); } } // Swap out the data used by cc_get_channel_info(). @@ -598,11 +607,6 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { chand->info_lb_policy_name = std::move(lb_policy_name); chand->info_service_config_json = std::move(service_config_json); gpr_mu_unlock(&chand->info_mu); - if (trace_this_address_resolution && chand->channelz_channel != nullptr) { - chand->channelz_channel->AddTraceEvent( - grpc_core::channelz::ChannelTrace::Severity::Info, - grpc_slice_from_static_string("New addresses resolved")); - } // Clean up. grpc_channel_args_destroy(chand->resolver_result); chand->resolver_result = nullptr; diff --git a/src/core/ext/filters/client_channel/lb_policy.h b/src/core/ext/filters/client_channel/lb_policy.h index 3940f138e66..21f80b7b947 100644 --- a/src/core/ext/filters/client_channel/lb_policy.h +++ b/src/core/ext/filters/client_channel/lb_policy.h @@ -95,9 +95,7 @@ class LoadBalancingPolicy /// Updates the policy with a new set of \a args from the resolver. /// Note that the LB policy gets the set of addresses from the /// GRPC_ARG_LB_ADDRESSES channel arg. - /// Returns true if the update caused the number of backends to go from - /// zero to non-zero, or non-zero to zero. - virtual bool UpdateLocked(const grpc_channel_args& args) GRPC_ABSTRACT; + virtual void UpdateLocked(const grpc_channel_args& args) GRPC_ABSTRACT; /// Finds an appropriate subchannel for a call, based on data in \a pick. /// \a pick must remain alive until the pick is complete. 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 4b49d10d9e0..5511df7a275 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 @@ -123,7 +123,7 @@ class GrpcLb : public LoadBalancingPolicy { public: GrpcLb(const grpc_lb_addresses* addresses, const Args& args); - bool UpdateLocked(const grpc_channel_args& args) override; + void UpdateLocked(const grpc_channel_args& args) override; bool PickLocked(PickState* pick, grpc_error** error) override; void CancelPickLocked(PickState* pick, grpc_error* error) override; void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask, @@ -1331,7 +1331,7 @@ void GrpcLb::ProcessChannelArgsLocked(const grpc_channel_args& args) { grpc_channel_args_destroy(lb_channel_args); } -bool GrpcLb::UpdateLocked(const grpc_channel_args& args) { +void GrpcLb::UpdateLocked(const grpc_channel_args& args) { ProcessChannelArgsLocked(args); // If fallback is configured and the RR policy already exists, update // it with the new fallback addresses. @@ -1358,7 +1358,6 @@ bool GrpcLb::UpdateLocked(const grpc_channel_args& args) { &lb_channel_connectivity_, &lb_channel_on_connectivity_changed_, nullptr); } - return false; // TODO } // diff --git a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc index e6f237c768d..f4dca146f71 100644 --- a/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc +++ b/src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc @@ -46,7 +46,7 @@ class PickFirst : public LoadBalancingPolicy { public: explicit PickFirst(const Args& args); - bool UpdateLocked(const grpc_channel_args& args) override; + void UpdateLocked(const grpc_channel_args& args) override; bool PickLocked(PickState* pick, grpc_error** error) override; void CancelPickLocked(PickState* pick, grpc_error* error) override; void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask, @@ -333,7 +333,7 @@ void PickFirst::UpdateChildRefsLocked() { child_subchannels_ = std::move(cs); } -bool PickFirst::UpdateLocked(const grpc_channel_args& args) { +void PickFirst::UpdateLocked(const grpc_channel_args& args) { AutoChildRefsUpdater guard(this); const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES); if (arg == nullptr || arg->type != GRPC_ARG_POINTER) { @@ -350,12 +350,10 @@ bool PickFirst::UpdateLocked(const grpc_channel_args& args) { "ignoring.", this); } - return false; + return; } const grpc_lb_addresses* addresses = static_cast(arg->value.pointer.p); - size_t old_size = - subchannel_list_ == nullptr ? 0 : subchannel_list_->num_subchannels(); if (grpc_lb_pick_first_trace.enabled()) { gpr_log(GPR_INFO, "Pick First %p received update with %" PRIuPTR " addresses", this, @@ -373,7 +371,7 @@ bool PickFirst::UpdateLocked(const grpc_channel_args& args) { "pf_update_empty"); subchannel_list_ = std::move(subchannel_list); // Empty list. selected_ = nullptr; - return old_size != 0; + return; } if (selected_ == nullptr) { // We don't yet have a selected subchannel, so replace the current @@ -413,7 +411,7 @@ bool PickFirst::UpdateLocked(const grpc_channel_args& args) { // not have contained the currently selected subchannel), drop // it, so that it doesn't override what we've done here. latest_pending_subchannel_list_.reset(); - return false; + return; } GRPC_ERROR_UNREF(error); } @@ -439,7 +437,6 @@ bool PickFirst::UpdateLocked(const grpc_channel_args& args) { ->CheckConnectivityStateAndStartWatchingLocked(); } } - return old_size == 0; } void PickFirst::PickFirstSubchannelData::ProcessConnectivityChangeLocked( diff --git a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc index 76d1d8b1233..e9ed85cf665 100644 --- a/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc +++ b/src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc @@ -57,7 +57,7 @@ class RoundRobin : public LoadBalancingPolicy { public: explicit RoundRobin(const Args& args); - bool UpdateLocked(const grpc_channel_args& args) override; + void UpdateLocked(const grpc_channel_args& args) override; bool PickLocked(PickState* pick, grpc_error** error) override; void CancelPickLocked(PickState* pick, grpc_error* error) override; void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask, @@ -664,7 +664,7 @@ void RoundRobin::NotifyOnStateChangeLocked(grpc_connectivity_state* current, notify); } -bool RoundRobin::UpdateLocked(const grpc_channel_args& args) { +void RoundRobin::UpdateLocked(const grpc_channel_args& args) { const grpc_arg* arg = grpc_channel_args_find(&args, GRPC_ARG_LB_ADDRESSES); AutoChildRefsUpdater guard(this); if (GPR_UNLIKELY(arg == nullptr || arg->type != GRPC_ARG_POINTER)) { @@ -677,7 +677,7 @@ bool RoundRobin::UpdateLocked(const grpc_channel_args& args) { GRPC_ERROR_CREATE_FROM_STATIC_STRING("Missing update in args"), "rr_update_missing"); } - return false; // TODO + return; } grpc_lb_addresses* addresses = static_cast(arg->value.pointer.p); @@ -711,7 +711,6 @@ bool RoundRobin::UpdateLocked(const grpc_channel_args& args) { // If we've started picking, start watching the new list. latest_pending_subchannel_list_->StartWatchingLocked(); } - return false; // todo } // diff --git a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc index aee33b085c3..e6c94167f65 100644 --- a/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc +++ b/src/core/ext/filters/client_channel/lb_policy/xds/xds.cc @@ -123,7 +123,7 @@ class XdsLb : public LoadBalancingPolicy { public: XdsLb(const grpc_lb_addresses* addresses, const Args& args); - bool UpdateLocked(const grpc_channel_args& args) override; + void UpdateLocked(const grpc_channel_args& args) override; bool PickLocked(PickState* pick, grpc_error** error) override; void CancelPickLocked(PickState* pick, grpc_error* error) override; void CancelMatchingPicksLocked(uint32_t initial_metadata_flags_mask, @@ -1325,7 +1325,7 @@ void XdsLb::ProcessChannelArgsLocked(const grpc_channel_args& args) { grpc_channel_args_destroy(lb_channel_args); } -bool XdsLb::UpdateLocked(const grpc_channel_args& args) { +void XdsLb::UpdateLocked(const grpc_channel_args& args) { ProcessChannelArgsLocked(args); // If fallback is configured and the RR policy already exists, update // it with the new fallback addresses. @@ -1352,7 +1352,6 @@ bool XdsLb::UpdateLocked(const grpc_channel_args& args) { &lb_channel_connectivity_, &lb_channel_on_connectivity_changed_, nullptr); } - return false; // TODO } // From 03dbb8c1e2ee6261e1b8802bb7dd0be9a0b8bd46 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Wed, 17 Oct 2018 14:16:49 -0700 Subject: [PATCH 3/6] reviewer feedback --- .../filters/client_channel/client_channel.cc | 49 ++++++++++++++----- 1 file changed, 38 insertions(+), 11 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index b3ff8653d4f..95d1f410945 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -131,6 +131,8 @@ typedef struct client_channel_channel_data { grpc_core::UniquePtr info_service_config_json; /* backpointer to grpc_channel's channelz node */ grpc_core::channelz::ClientChannelNode* channelz_channel; + /* caches if the last resolution event led to zero addresses */ + bool previous_resolution_zero_num_addresses; } channel_data; typedef struct { @@ -572,7 +574,7 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { // Find service config. grpc_core::UniquePtr service_config_json = get_service_config_from_resolver_result_locked(chand); - // Note: It's safe to use chand->info_lb_policy_name here without + // Note: It's safe to use chand->info_service_config_json here without // taking a lock on chand->info_mu, because this function is the // only thing that modifies its value, and it can only be invoked // once at any given time. @@ -585,21 +587,45 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { chand->info_service_config_json != nullptr && strcmp(service_config_json.get(), chand->info_service_config_json.get()) != 0)) { + // 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(gpr_strdup("Service config reloaded")); } - //TODO - // const grpc_arg* channel_arg = - // grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES); - // if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) { - // grpc_lb_addresses* addresses = - // static_cast(channel_arg->value.pointer.p); - // } else { - // } + int zero_num_addresses = true; + const grpc_arg* channel_arg = + grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES); + if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) { + grpc_lb_addresses* addresses = + static_cast(channel_arg->value.pointer.p); + if (addresses->num_addresses > 0) { + zero_num_addresses = false; + } + } + if (zero_num_addresses && + !chand->previous_resolution_zero_num_addresses) { + trace_strings.push_back(gpr_strdup("Address list became empty")); + } else if (!zero_num_addresses && + chand->previous_resolution_zero_num_addresses) { + trace_strings.push_back(gpr_strdup("Address list became non-empty")); + } + chand->previous_resolution_zero_num_addresses = zero_num_addresses; if (!trace_strings.empty()) { - // TODO, concatenate the strings from trace_strings. + gpr_strvec v; + gpr_strvec_init(&v); + gpr_strvec_add(&v, gpr_strdup("Resolution event: ")); + bool is_first = 1; + for (size_t i = 0; i < trace_strings.size(); ++i) { + if (!is_first) gpr_strvec_add(&v, gpr_strdup(", ")); + is_first = false; + gpr_strvec_add(&v, trace_strings[i]); + } + char* flat; + size_t flat_len = 0; + flat = gpr_strvec_flatten(&v, &flat_len); chand->channelz_channel->AddTraceEvent( grpc_core::channelz::ChannelTrace::Severity::Info, - grpc_slice_from_static_string("New addresses resolved")); + grpc_slice_new(flat, flat_len, gpr_free)); + gpr_strvec_destroy(&v); } } // Swap out the data used by cc_get_channel_info(). @@ -770,6 +796,7 @@ static grpc_error* cc_init_channel_elem(grpc_channel_element* elem, arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_ENABLE_RETRIES); chand->enable_retries = grpc_channel_arg_get_bool(arg, true); chand->channelz_channel = nullptr; + chand->previous_resolution_zero_num_addresses = true; // Record client channel factory. arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_CLIENT_CHANNEL_FACTORY); From a60226726ae6076916db0e1d616338a1a211770d Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 18 Oct 2018 10:49:25 -0700 Subject: [PATCH 4/6] reviewer feedback --- .../filters/client_channel/client_channel.cc | 122 ++++++++++-------- test/core/end2end/tests/channelz.cc | 2 + test/core/end2end/tests/retry_streaming.cc | 55 +++++--- 3 files changed, 107 insertions(+), 72 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 95d1f410945..64e206ec631 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -131,8 +131,8 @@ typedef struct client_channel_channel_data { grpc_core::UniquePtr info_service_config_json; /* backpointer to grpc_channel's channelz node */ grpc_core::channelz::ClientChannelNode* channelz_channel; - /* caches if the last resolution event led to zero addresses */ - bool previous_resolution_zero_num_addresses; + /* caches if the last resolution event contained addresses */ + bool previous_resolution_contained_addresses; } channel_data; typedef struct { @@ -403,6 +403,8 @@ static void request_reresolution_locked(void* arg, grpc_error* error) { chand->lb_policy->SetReresolutionClosureLocked(&args->closure); } +using TraceStringVector = grpc_core::InlinedVector; + // Creates a new LB policy, replacing any previous one. // If the new policy is created successfully, sets *connectivity_state and // *connectivity_error to its initial connectivity state; otherwise, @@ -410,7 +412,7 @@ static void request_reresolution_locked(void* arg, grpc_error* error) { static void create_new_lb_policy_locked( channel_data* chand, char* lb_policy_name, grpc_connectivity_state* connectivity_state, - grpc_error** connectivity_error) { + grpc_error** connectivity_error, TraceStringVector* trace_strings) { grpc_core::LoadBalancingPolicy::Args lb_policy_args; lb_policy_args.combiner = chand->combiner; lb_policy_args.client_channel_factory = chand->client_channel_factory; @@ -420,11 +422,21 @@ static void create_new_lb_policy_locked( lb_policy_name, lb_policy_args); if (GPR_UNLIKELY(new_lb_policy == nullptr)) { gpr_log(GPR_ERROR, "could not create LB policy \"%s\"", lb_policy_name); + if (chand->channelz_channel != nullptr) { + char* str; + gpr_asprintf(&str, "Could not create LB policy \'%s\'", lb_policy_name); + trace_strings->push_back(str); + } } else { if (grpc_client_channel_trace.enabled()) { gpr_log(GPR_INFO, "chand=%p: created new LB policy \"%s\" (%p)", chand, lb_policy_name, new_lb_policy.get()); } + if (chand->channelz_channel != nullptr) { + char* str; + gpr_asprintf(&str, "Created new LB policy \'%s\'", lb_policy_name); + trace_strings->push_back(str); + } // Swap out the LB policy and update the fds in // chand->interested_parties. if (chand->lb_policy != nullptr) { @@ -499,6 +511,51 @@ get_service_config_from_resolver_result_locked(channel_data* chand) { return grpc_core::UniquePtr(gpr_strdup(service_config_json)); } +static void check_for_important_resolution_change( + channel_data* chand, TraceStringVector* trace_strings) { + int resolution_contains_addresses = false; + const grpc_arg* channel_arg = + grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES); + if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) { + grpc_lb_addresses* addresses = + static_cast(channel_arg->value.pointer.p); + if (addresses->num_addresses > 0) { + resolution_contains_addresses = true; + } + } + if (!resolution_contains_addresses && + chand->previous_resolution_contained_addresses) { + trace_strings->push_back(gpr_strdup("Address list became empty")); + } else if (resolution_contains_addresses && + !chand->previous_resolution_contained_addresses) { + trace_strings->push_back(gpr_strdup("Address list became non-empty")); + } + chand->previous_resolution_contained_addresses = + resolution_contains_addresses; +} + +static void concatenate_and_add_channel_trace( + channel_data* chand, TraceStringVector* trace_strings) { + if (!trace_strings->empty()) { + gpr_strvec v; + gpr_strvec_init(&v); + gpr_strvec_add(&v, gpr_strdup("Resolution event: ")); + bool is_first = 1; + for (size_t i = 0; i < trace_strings->size(); ++i) { + if (!is_first) gpr_strvec_add(&v, gpr_strdup(", ")); + is_first = false; + gpr_strvec_add(&v, (*trace_strings)[i]); + } + char* flat; + size_t flat_len = 0; + flat = gpr_strvec_flatten(&v, &flat_len); + chand->channelz_channel->AddTraceEvent( + grpc_core::channelz::ChannelTrace::Severity::Info, + grpc_slice_new(flat, flat_len, gpr_free)); + gpr_strvec_destroy(&v); + } +} + // Callback invoked when a resolver result is available. static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { channel_data* chand = static_cast(arg); @@ -529,7 +586,7 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { // (d) Address resolution that causes a new LB policy to be created. // // we track a list of strings to eventually be concatenated and traced. - grpc_core::InlinedVector trace_strings; + TraceStringVector trace_strings; grpc_connectivity_state connectivity_state = GRPC_CHANNEL_TRANSIENT_FAILURE; grpc_error* connectivity_error = GRPC_ERROR_CREATE_FROM_STATIC_STRING("No load balancing policy"); @@ -564,12 +621,8 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { } else { // Instantiate new LB policy. create_new_lb_policy_locked(chand, lb_policy_name.get(), - &connectivity_state, &connectivity_error); - if (chand->channelz_channel != nullptr) { - char* str; - gpr_asprintf(&str, "Switched LB policy to %s", lb_policy_name.get()); - trace_strings.push_back(str); - } + &connectivity_state, &connectivity_error, + &trace_strings); } // Find service config. grpc_core::UniquePtr service_config_json = @@ -579,54 +632,17 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { // only thing that modifies its value, and it can only be invoked // once at any given time. if (chand->channelz_channel != nullptr) { - if ((service_config_json == nullptr && - chand->info_service_config_json != nullptr) || - (service_config_json != nullptr && - chand->info_service_config_json == nullptr) || + if (((service_config_json == nullptr) != + (chand->info_service_config_json == nullptr)) || (service_config_json != nullptr && - chand->info_service_config_json != nullptr && strcmp(service_config_json.get(), chand->info_service_config_json.get()) != 0)) { // 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(gpr_strdup("Service config reloaded")); - } - int zero_num_addresses = true; - const grpc_arg* channel_arg = - grpc_channel_args_find(chand->resolver_result, GRPC_ARG_LB_ADDRESSES); - if (channel_arg != nullptr && channel_arg->type == GRPC_ARG_POINTER) { - grpc_lb_addresses* addresses = - static_cast(channel_arg->value.pointer.p); - if (addresses->num_addresses > 0) { - zero_num_addresses = false; - } - } - if (zero_num_addresses && - !chand->previous_resolution_zero_num_addresses) { - trace_strings.push_back(gpr_strdup("Address list became empty")); - } else if (!zero_num_addresses && - chand->previous_resolution_zero_num_addresses) { - trace_strings.push_back(gpr_strdup("Address list became non-empty")); - } - chand->previous_resolution_zero_num_addresses = zero_num_addresses; - if (!trace_strings.empty()) { - gpr_strvec v; - gpr_strvec_init(&v); - gpr_strvec_add(&v, gpr_strdup("Resolution event: ")); - bool is_first = 1; - for (size_t i = 0; i < trace_strings.size(); ++i) { - if (!is_first) gpr_strvec_add(&v, gpr_strdup(", ")); - is_first = false; - gpr_strvec_add(&v, trace_strings[i]); - } - char* flat; - size_t flat_len = 0; - flat = gpr_strvec_flatten(&v, &flat_len); - chand->channelz_channel->AddTraceEvent( - grpc_core::channelz::ChannelTrace::Severity::Info, - grpc_slice_new(flat, flat_len, gpr_free)); - gpr_strvec_destroy(&v); + trace_strings.push_back(gpr_strdup("Service config changed")); } + check_for_important_resolution_change(chand, &trace_strings); + concatenate_and_add_channel_trace(chand, &trace_strings); } // Swap out the data used by cc_get_channel_info(). gpr_mu_lock(&chand->info_mu); @@ -796,7 +812,7 @@ static grpc_error* cc_init_channel_elem(grpc_channel_element* elem, arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_ENABLE_RETRIES); chand->enable_retries = grpc_channel_arg_get_bool(arg, true); chand->channelz_channel = nullptr; - chand->previous_resolution_zero_num_addresses = true; + chand->previous_resolution_contained_addresses = false; // Record client channel factory. arg = grpc_channel_args_find(args->channel_args, GRPC_ARG_CLIENT_CHANNEL_FACTORY); diff --git a/test/core/end2end/tests/channelz.cc b/test/core/end2end/tests/channelz.cc index a812994435e..7c61b7910be 100644 --- a/test/core/end2end/tests/channelz.cc +++ b/test/core/end2end/tests/channelz.cc @@ -288,6 +288,8 @@ static void test_channelz_with_channel_trace(grpc_end2end_test_config config) { grpc_server_get_channelz_node(f.server); GPR_ASSERT(channelz_server != nullptr); + run_one_request(config, f, true); + char* json = channelz_channel->RenderJsonString(); GPR_ASSERT(json != nullptr); gpr_log(GPR_INFO, "%s", json); diff --git a/test/core/end2end/tests/retry_streaming.cc b/test/core/end2end/tests/retry_streaming.cc index d06d124ca42..d52574fc589 100644 --- a/test/core/end2end/tests/retry_streaming.cc +++ b/test/core/end2end/tests/retry_streaming.cc @@ -28,6 +28,9 @@ #include #include +#include "src/core/lib/surface/channel.h" +#include "src/core/lib/surface/server.h" + #include "src/core/lib/channel/channel_args.h" #include "src/core/lib/gpr/string.h" #include "src/core/lib/gpr/useful.h" @@ -133,25 +136,30 @@ static void test_retry_streaming(grpc_end2end_test_config config) { int was_cancelled = 2; char* peer; - grpc_arg arg; - arg.type = GRPC_ARG_STRING; - arg.key = const_cast(GRPC_ARG_SERVICE_CONFIG); - arg.value.string = const_cast( - "{\n" - " \"methodConfig\": [ {\n" - " \"name\": [\n" - " { \"service\": \"service\", \"method\": \"method\" }\n" - " ],\n" - " \"retryPolicy\": {\n" - " \"maxAttempts\": 3,\n" - " \"initialBackoff\": \"1s\",\n" - " \"maxBackoff\": \"120s\",\n" - " \"backoffMultiplier\": 1.6,\n" - " \"retryableStatusCodes\": [ \"ABORTED\" ]\n" - " }\n" - " } ]\n" - "}"); - grpc_channel_args client_args = {1, &arg}; + grpc_arg arg[] = { + grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE), + 1024 * 8), + grpc_channel_arg_integer_create( + const_cast(GRPC_ARG_ENABLE_CHANNELZ), true), + grpc_channel_arg_string_create( + const_cast(GRPC_ARG_SERVICE_CONFIG), + const_cast( + "{\n" + " \"methodConfig\": [ {\n" + " \"name\": [\n" + " { \"service\": \"service\", \"method\": \"method\" }\n" + " ],\n" + " \"retryPolicy\": {\n" + " \"maxAttempts\": 3,\n" + " \"initialBackoff\": \"1s\",\n" + " \"maxBackoff\": \"120s\",\n" + " \"backoffMultiplier\": 1.6,\n" + " \"retryableStatusCodes\": [ \"ABORTED\" ]\n" + " }\n" + " } ]\n" + "}"))}; + grpc_channel_args client_args = {GPR_ARRAY_SIZE(arg), arg}; grpc_end2end_test_fixture f = begin_test(config, "retry_streaming", &client_args, nullptr); @@ -161,6 +169,9 @@ static void test_retry_streaming(grpc_end2end_test_config config) { c = grpc_channel_create_call(f.client, nullptr, GRPC_PROPAGATE_DEFAULTS, f.cq, grpc_slice_from_static_string("/service/method"), nullptr, deadline, nullptr); + grpc_core::channelz::ChannelNode* channelz_channel = + grpc_channel_get_channelz_node(f.client); + GPR_ASSERT(c); peer = grpc_call_get_peer(c); @@ -384,6 +395,11 @@ static void test_retry_streaming(grpc_end2end_test_config config) { GPR_ASSERT(0 == call_details.flags); GPR_ASSERT(was_cancelled == 1); + GPR_ASSERT(channelz_channel != nullptr); + char* json = channelz_channel->RenderJsonString(); + GPR_ASSERT(json != nullptr); + gpr_log(GPR_INFO, "%s", json); + grpc_slice_unref(details); grpc_metadata_array_destroy(&initial_metadata_recv); grpc_metadata_array_destroy(&trailing_metadata_recv); @@ -414,6 +430,7 @@ static void test_retry_streaming(grpc_end2end_test_config config) { void retry_streaming(grpc_end2end_test_config config) { GPR_ASSERT(config.feature_mask & FEATURE_MASK_SUPPORTS_CLIENT_CHANNEL); + test_retry_streaming(config); } From 0582a003d5385ce16ec13d5e5d1128913f9b59c6 Mon Sep 17 00:00:00 2001 From: ncteisen Date: Thu, 18 Oct 2018 11:42:32 -0700 Subject: [PATCH 5/6] reviewer comments --- .../ext/filters/client_channel/client_channel.cc | 8 ++++---- test/core/end2end/tests/retry_streaming.cc | 13 +++++++++++-- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 64e206ec631..daf1b89b094 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -511,7 +511,7 @@ get_service_config_from_resolver_result_locked(channel_data* chand) { return grpc_core::UniquePtr(gpr_strdup(service_config_json)); } -static void check_for_important_resolution_change( +static void maybe_add_trace_message_for_address_changes_locked( channel_data* chand, TraceStringVector* trace_strings) { int resolution_contains_addresses = false; const grpc_arg* channel_arg = @@ -534,7 +534,7 @@ static void check_for_important_resolution_change( resolution_contains_addresses; } -static void concatenate_and_add_channel_trace( +static void concatenate_and_add_channel_trace_locked( channel_data* chand, TraceStringVector* trace_strings) { if (!trace_strings->empty()) { gpr_strvec v; @@ -641,8 +641,8 @@ static void on_resolver_result_changed_locked(void* arg, grpc_error* error) { // config in the trace, at the risk of bloating the trace logs. trace_strings.push_back(gpr_strdup("Service config changed")); } - check_for_important_resolution_change(chand, &trace_strings); - concatenate_and_add_channel_trace(chand, &trace_strings); + maybe_add_trace_message_for_address_changes_locked(chand, &trace_strings); + concatenate_and_add_channel_trace_locked(chand, &trace_strings); } // Swap out the data used by cc_get_channel_info(). gpr_mu_lock(&chand->info_mu); diff --git a/test/core/end2end/tests/retry_streaming.cc b/test/core/end2end/tests/retry_streaming.cc index d52574fc589..94a27faf7b2 100644 --- a/test/core/end2end/tests/retry_streaming.cc +++ b/test/core/end2end/tests/retry_streaming.cc @@ -136,7 +136,7 @@ static void test_retry_streaming(grpc_end2end_test_config config) { int was_cancelled = 2; char* peer; - grpc_arg arg[] = { + grpc_arg args[] = { grpc_channel_arg_integer_create( const_cast(GRPC_ARG_MAX_CHANNEL_TRACE_EVENT_MEMORY_PER_NODE), 1024 * 8), @@ -159,7 +159,7 @@ static void test_retry_streaming(grpc_end2end_test_config config) { " }\n" " } ]\n" "}"))}; - grpc_channel_args client_args = {GPR_ARRAY_SIZE(arg), arg}; + grpc_channel_args client_args = {GPR_ARRAY_SIZE(args), args}; grpc_end2end_test_fixture f = begin_test(config, "retry_streaming", &client_args, nullptr); @@ -399,6 +399,15 @@ static void test_retry_streaming(grpc_end2end_test_config config) { char* json = channelz_channel->RenderJsonString(); GPR_ASSERT(json != nullptr); gpr_log(GPR_INFO, "%s", json); + GPR_ASSERT(nullptr != strstr(json, "\"trace\"")); + GPR_ASSERT(nullptr != strstr(json, "\"description\":\"Channel created\"")); + GPR_ASSERT(nullptr != strstr(json, "\"severity\":\"CT_INFO\"")); + GPR_ASSERT(nullptr != strstr(json, "Resolution event")); + GPR_ASSERT(nullptr != strstr(json, "Created new LB policy")); + GPR_ASSERT(nullptr != strstr(json, "Service config changed")); + GPR_ASSERT(nullptr != strstr(json, "Address list became non-empty")); + GPR_ASSERT(nullptr != strstr(json, "Channel state change to CONNECTING")); + gpr_free(json); grpc_slice_unref(details); grpc_metadata_array_destroy(&initial_metadata_recv); From 8c1670d5475e50e1efc876f2d45d58574495f891 Mon Sep 17 00:00:00 2001 From: Noah Eisen Date: Fri, 19 Oct 2018 10:23:18 -0700 Subject: [PATCH 6/6] Fix asan --- src/core/lib/channel/channel_trace.cc | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/core/lib/channel/channel_trace.cc b/src/core/lib/channel/channel_trace.cc index fe81acb617f..f0d21db32a8 100644 --- a/src/core/lib/channel/channel_trace.cc +++ b/src/core/lib/channel/channel_trace.cc @@ -108,16 +108,20 @@ void ChannelTrace::AddTraceEventHelper(TraceEvent* new_trace_event) { } void ChannelTrace::AddTraceEvent(Severity severity, grpc_slice data) { - if (max_event_memory_ == 0) + if (max_event_memory_ == 0) { + grpc_slice_unref_internal(data); return; // tracing is disabled if max_event_memory_ == 0 + } AddTraceEventHelper(New(severity, data)); } void ChannelTrace::AddTraceEventWithReference( Severity severity, grpc_slice data, RefCountedPtr referenced_entity) { - if (max_event_memory_ == 0) + if (max_event_memory_ == 0) { + grpc_slice_unref_internal(data); return; // tracing is disabled if max_event_memory_ == 0 + } // create and fill up the new event AddTraceEventHelper( New(severity, data, std::move(referenced_entity)));