From be1b7f986e55756ac83b3807ee1d73929fbb3546 Mon Sep 17 00:00:00 2001 From: David Garcia Quintas Date: Fri, 12 Jan 2018 14:01:38 -0800 Subject: [PATCH] PR comments, round 3 --- src/core/ext/filters/client_channel/client_channel.cc | 6 +++--- .../filters/client_channel/lb_policy/grpclb/grpclb.cc | 8 +++----- .../client_channel/lb_policy/pick_first/pick_first.cc | 4 +--- .../client_channel/lb_policy/round_robin/round_robin.cc | 2 +- src/core/ext/filters/client_channel/subchannel.cc | 9 ++------- src/core/lib/support/ref_counted_ptr.h | 9 ++++++++- 6 files changed, 18 insertions(+), 20 deletions(-) diff --git a/src/core/ext/filters/client_channel/client_channel.cc b/src/core/ext/filters/client_channel/client_channel.cc index 6a10a25a325..bb29f65af97 100644 --- a/src/core/ext/filters/client_channel/client_channel.cc +++ b/src/core/ext/filters/client_channel/client_channel.cc @@ -1031,7 +1031,7 @@ static void create_subchannel_call_locked(grpc_call_element* elem, static void pick_done_locked(grpc_call_element* elem, grpc_error* error) { call_data* calld = (call_data*)elem->call_data; channel_data* chand = (channel_data*)elem->channel_data; - if (!calld->pick.connected_subchannel) { + if (calld->pick.connected_subchannel == nullptr) { // Failed to create subchannel. GRPC_ERROR_UNREF(calld->error); calld->error = error == GRPC_ERROR_NONE @@ -1283,7 +1283,7 @@ static void start_pick_locked(void* arg, grpc_error* ignored) { grpc_call_element* elem = (grpc_call_element*)arg; call_data* calld = (call_data*)elem->call_data; channel_data* chand = (channel_data*)elem->channel_data; - GPR_ASSERT(!calld->pick.connected_subchannel); + GPR_ASSERT(calld->pick.connected_subchannel == nullptr); if (chand->lb_policy != nullptr) { // We already have an LB policy, so ask it for a pick. if (pick_callback_start_locked(elem)) { @@ -1462,7 +1462,7 @@ static void cc_destroy_call_elem(grpc_call_element* elem, "client_channel_destroy_call"); } GPR_ASSERT(calld->waiting_for_pick_batches_count == 0); - if (calld->pick.connected_subchannel) { + if (calld->pick.connected_subchannel != nullptr) { calld->pick.connected_subchannel.reset(); } for (size_t i = 0; i < GRPC_CONTEXT_COUNT; ++i) { 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 39467272b36..9da21c73924 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 @@ -322,7 +322,7 @@ static void pending_pick_set_metadata_and_context(pending_pick* pp) { /* if connected_subchannel is nullptr, no pick has been made by the RR * policy (e.g., all addresses failed to connect). There won't be any * user_data/token available */ - if (pp->pick->connected_subchannel.get() != nullptr) { + if (pp->pick->connected_subchannel != nullptr) { if (!GRPC_MDISNULL(pp->lb_token)) { initial_metadata_add_lb_token(pp->pick->initial_metadata, &pp->pick->lb_token_mdelem_storage, @@ -935,8 +935,7 @@ static void glb_shutdown_locked(grpc_lb_policy* pol, } gpr_free(pp); } else { - pp->pick->connected_subchannel = - grpc_core::MakeRefCounted(nullptr); + pp->pick->connected_subchannel.reset(nullptr); GRPC_CLOSURE_SCHED(&pp->on_complete, GRPC_ERROR_REF(error)); } pp = next; @@ -973,8 +972,7 @@ static void glb_cancel_pick_locked(grpc_lb_policy* pol, while (pp != nullptr) { pending_pick* next = pp->next; if (pp->pick == pick) { - pick->connected_subchannel = - grpc_core::MakeRefCounted(nullptr); + pick->connected_subchannel.reset(nullptr); GRPC_CLOSURE_SCHED(&pp->on_complete, GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING( "Pick Cancelled", &error, 1)); 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 c91a305f336..725b78d4787 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 @@ -294,7 +294,7 @@ static void pf_update_locked(grpc_lb_policy* policy, p, p->selected->subchannel, i, subchannel_list->num_subchannels); } - if (p->selected->connected_subchannel) { + if (p->selected->connected_subchannel != nullptr) { sd->connected_subchannel = p->selected->connected_subchannel; } p->selected = sd; @@ -458,8 +458,6 @@ static void pf_connectivity_changed_locked(void* arg, grpc_error* error) { // Cases 1 and 2. grpc_connectivity_state_set(&p->state_tracker, GRPC_CHANNEL_READY, GRPC_ERROR_NONE, "connecting_ready"); - sd->connected_subchannel = - grpc_subchannel_get_connected_subchannel(sd->subchannel); p->selected = sd; if (grpc_lb_pick_first_trace.enabled()) { gpr_log(GPR_INFO, "Pick First %p selected subchannel %p", (void*)p, 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 91fee2d51aa..dca345566ad 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 @@ -423,7 +423,7 @@ static void rr_connectivity_changed_locked(void* arg, grpc_error* error) { break; } case GRPC_CHANNEL_READY: { - if (!sd->connected_subchannel) { + if (sd->connected_subchannel == nullptr) { sd->connected_subchannel = grpc_subchannel_get_connected_subchannel(sd->subchannel); } diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index fe0bb486e87..5b8a14b9e7b 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -56,10 +56,6 @@ #define GRPC_SUBCHANNEL_RECONNECT_MAX_BACKOFF_SECONDS 120 #define GRPC_SUBCHANNEL_RECONNECT_JITTER 0.2 -#define GET_CONNECTED_SUBCHANNEL(subchannel, barrier) \ - ((grpc_core::ConnectedSubchannel*)(gpr_atm_##barrier##_load( \ - &(subchannel)->connected_subchannel))) - namespace { struct state_watcher { grpc_closure closure; @@ -443,7 +439,7 @@ static void maybe_start_connecting_locked(grpc_subchannel* c) { return; } - if (c->connected_subchannel) { + if (c->connected_subchannel != nullptr) { /* Already connected: don't restart */ return; } @@ -524,7 +520,7 @@ static void on_connected_subchannel_connectivity_changed(void* p, switch (connected_subchannel_watcher->connectivity_state) { case GRPC_CHANNEL_TRANSIENT_FAILURE: case GRPC_CHANNEL_SHUTDOWN: { - if (!c->disconnected && c->connected_subchannel) { + if (!c->disconnected && c->connected_subchannel != nullptr) { if (grpc_trace_stream_refcount.enabled()) { gpr_log(GPR_INFO, "Connected subchannel %p of subchannel %p has gone into %s. " @@ -605,7 +601,6 @@ static bool publish_transport_locked(grpc_subchannel* c) { /* publish */ c->connected_subchannel.reset( grpc_core::New(stk)); - gpr_atm_full_barrier(); gpr_log(GPR_INFO, "New connected subchannel at %p for subchannel %p", c->connected_subchannel.get(), c); diff --git a/src/core/lib/support/ref_counted_ptr.h b/src/core/lib/support/ref_counted_ptr.h index dad213003d6..8c8606ca0a1 100644 --- a/src/core/lib/support/ref_counted_ptr.h +++ b/src/core/lib/support/ref_counted_ptr.h @@ -76,7 +76,14 @@ class RefCountedPtr { T& operator*() const { return *value_; } T* operator->() const { return value_; } - explicit operator bool() const { return get() != nullptr; } + bool operator==(const RefCountedPtr& other) const { + return value_ == other.value_; + } + bool operator==(T* other) const { return value_ == other; } + bool operator!=(const RefCountedPtr& other) const { + return value_ != other.value_; + } + bool operator!=(T* other) const { return value_ != other; } private: T* value_ = nullptr;