PR comments, round 2

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 == nullptr) {
+  if (!calld->pick.connected_subchannel) {
     // 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 == nullptr);
+  GPR_ASSERT(!calld->pick.connected_subchannel);
   if (chand->lb_policy != nullptr) {
     // We already have an LB policy, so ask it for a pick.
     if (pick_callback_start_locked(elem)) {
@@ -1462,8 +1462,8 @@ 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 != nullptr) {
-    GRPC_CONNECTED_SUBCHANNEL_UNREF(calld->pick.connected_subchannel, "picked");
+  if (calld->pick.connected_subchannel) {
+    calld->pick.connected_subchannel.reset();
   }
   for (size_t i = 0; i < GRPC_CONTEXT_COUNT; ++i) {
     if (calld->pick.subchannel_call_context[i].value != nullptr) {

src/core/ext/filters/client_channel/lb_policy.h

@@ -21,6 +21,7 @@
 
 #include "src/core/ext/filters/client_channel/subchannel.h"
 #include "src/core/lib/iomgr/polling_entity.h"
+#include "src/core/lib/support/ref_counted_ptr.h"
 #include "src/core/lib/transport/connectivity_state.h"
 
 /** A load balancing policy: specified by a vtable and a struct (which
@@ -54,9 +55,9 @@ typedef struct grpc_lb_policy_pick_state {
   grpc_linked_mdelem lb_token_mdelem_storage;
   /// Closure to run when pick is complete, if not completed synchronously.
   grpc_closure* on_complete;
-  /// Will be set to the selected subchannel, or NULL on failure or when
+  /// Will be set to the selected subchannel, or nullptr on failure or when
   /// the LB policy decides to drop the call.
-  grpc_core::ConnectedSubchannel* connected_subchannel;
+  grpc_core::RefCountedPtr<grpc_core::ConnectedSubchannel> connected_subchannel;
   /// Will be populated with context to pass to the subchannel call, if needed.
   grpc_call_context_element subchannel_call_context[GRPC_CONTEXT_COUNT];
   /// Upon success, \a *user_data will be set to whatever opaque information

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 != nullptr) {
+  if (pp->pick->connected_subchannel.get() != nullptr) {
     if (!GRPC_MDISNULL(pp->lb_token)) {
       initial_metadata_add_lb_token(pp->pick->initial_metadata,
                                     &pp->pick->lb_token_mdelem_storage,
@@ -935,7 +935,8 @@ static void glb_shutdown_locked(grpc_lb_policy* pol,
       }
       gpr_free(pp);
     } else {
-      pp->pick->connected_subchannel = nullptr;
+      pp->pick->connected_subchannel =
+          grpc_core::MakeRefCounted<grpc_core::ConnectedSubchannel>(nullptr);
       GRPC_CLOSURE_SCHED(&pp->on_complete, GRPC_ERROR_REF(error));
     }
     pp = next;
@@ -972,7 +973,8 @@ 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 = nullptr;
+      pick->connected_subchannel =
+          grpc_core::MakeRefCounted<grpc_core::ConnectedSubchannel>(nullptr);
       GRPC_CLOSURE_SCHED(&pp->on_complete,
                          GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
                              "Pick Cancelled", &error, 1));

src/core/ext/filters/client_channel/lb_policy/pick_first/pick_first.cc

@@ -81,7 +81,7 @@ static void pf_shutdown_locked(grpc_lb_policy* pol,
         GRPC_CLOSURE_SCHED(pick->on_complete, GRPC_ERROR_NONE);
       }
     } else {
-      pick->connected_subchannel = nullptr;
+      pick->connected_subchannel.reset();
       GRPC_CLOSURE_SCHED(pick->on_complete, GRPC_ERROR_REF(error));
     }
   }
@@ -111,7 +111,7 @@ static void pf_cancel_pick_locked(grpc_lb_policy* pol,
   while (pp != nullptr) {
     grpc_lb_policy_pick_state* next = pp->next;
     if (pp == pick) {
-      pick->connected_subchannel = nullptr;
+      pick->connected_subchannel.reset();
       GRPC_CLOSURE_SCHED(pick->on_complete,
                          GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
                              "Pick Cancelled", &error, 1));
@@ -176,8 +176,7 @@ static int pf_pick_locked(grpc_lb_policy* pol,
   pick_first_lb_policy* p = (pick_first_lb_policy*)pol;
   // If we have a selected subchannel already, return synchronously.
   if (p->selected != nullptr) {
-    pick->connected_subchannel = GRPC_CONNECTED_SUBCHANNEL_REF(
-        p->selected->connected_subchannel, "picked");
+    pick->connected_subchannel = p->selected->connected_subchannel;
     return 1;
   }
   // No subchannel selected yet, so handle asynchronously.
@@ -295,9 +294,8 @@ static void pf_update_locked(grpc_lb_policy* policy,
                   p, p->selected->subchannel, i,
                   subchannel_list->num_subchannels);
         }
-        if (p->selected->connected_subchannel != nullptr) {
-          sd->connected_subchannel = GRPC_CONNECTED_SUBCHANNEL_REF(
-              p->selected->connected_subchannel, "pf_update_includes_selected");
+        if (p->selected->connected_subchannel) {
+          sd->connected_subchannel = p->selected->connected_subchannel;
         }
         p->selected = sd;
         if (p->subchannel_list != nullptr) {
@@ -425,7 +423,6 @@ static void pf_connectivity_changed_locked(void* arg, grpc_error* error) {
             sd->subchannel_list, "pf_selected_shutdown");
         grpc_lb_subchannel_data_unref_subchannel(
             sd, "pf_selected_shutdown");  // Unrefs connected subchannel
-
       } else {
         grpc_connectivity_state_set(&p->state_tracker,
                                     sd->curr_connectivity_state,
@@ -449,15 +446,8 @@ static void pf_connectivity_changed_locked(void* arg, grpc_error* error) {
     case GRPC_CHANNEL_READY: {
       // Case 2.  Promote p->latest_pending_subchannel_list to
       // p->subchannel_list.
-      grpc_core::ConnectedSubchannel* con =
+      sd->connected_subchannel =
           grpc_subchannel_get_connected_subchannel(sd->subchannel);
-      if (con == nullptr) {
-        // The subchannel may have become disconnected by the time this callback
-        // is invoked. Simply ignore and resubscribe: ulterior connectivity
-        // states must be in the pipeline and will eventually be invoked.
-        grpc_lb_subchannel_data_start_connectivity_watch(sd);
-        break;
-      }
       if (sd->subchannel_list == p->latest_pending_subchannel_list) {
         GPR_ASSERT(p->subchannel_list != nullptr);
         grpc_lb_subchannel_list_shutdown_and_unref(p->subchannel_list,
@@ -469,7 +459,7 @@ static void pf_connectivity_changed_locked(void* arg, grpc_error* error) {
       grpc_connectivity_state_set(&p->state_tracker, GRPC_CHANNEL_READY,
                                   GRPC_ERROR_NONE, "connecting_ready");
       sd->connected_subchannel =
-          GRPC_CONNECTED_SUBCHANNEL_REF(con, "connected");
+          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,
@@ -481,8 +471,7 @@ static void pf_connectivity_changed_locked(void* arg, grpc_error* error) {
       grpc_lb_policy_pick_state* pick;
       while ((pick = p->pending_picks)) {
         p->pending_picks = pick->next;
-        pick->connected_subchannel = GRPC_CONNECTED_SUBCHANNEL_REF(
-            p->selected->connected_subchannel, "picked");
+        pick->connected_subchannel = p->selected->connected_subchannel;
         if (grpc_lb_pick_first_trace.enabled()) {
           gpr_log(GPR_INFO,
                   "Servicing pending pick with selected subchannel %p",

src/core/ext/filters/client_channel/lb_policy/round_robin/round_robin.cc

@@ -36,6 +36,7 @@
 #include "src/core/lib/debug/trace.h"
 #include "src/core/lib/iomgr/combiner.h"
 #include "src/core/lib/iomgr/sockaddr_utils.h"
+#include "src/core/lib/support/ref_counted_ptr.h"
 #include "src/core/lib/transport/connectivity_state.h"
 #include "src/core/lib/transport/static_metadata.h"
 
@@ -127,7 +128,7 @@ static void update_last_ready_subchannel_index_locked(round_robin_lb_policy* p,
             (void*)p, (unsigned long)last_ready_index,
             (void*)p->subchannel_list->subchannels[last_ready_index].subchannel,
             (void*)p->subchannel_list->subchannels[last_ready_index]
-                .connected_subchannel);
+                .connected_subchannel.get());
   }
 }
 
@@ -162,7 +163,7 @@ static void rr_shutdown_locked(grpc_lb_policy* pol,
         GRPC_CLOSURE_SCHED(pick->on_complete, GRPC_ERROR_NONE);
       }
     } else {
-      pick->connected_subchannel = nullptr;
+      pick->connected_subchannel.reset();
       GRPC_CLOSURE_SCHED(pick->on_complete, GRPC_ERROR_REF(error));
     }
   }
@@ -192,7 +193,7 @@ static void rr_cancel_pick_locked(grpc_lb_policy* pol,
   while (pp != nullptr) {
     grpc_lb_policy_pick_state* next = pp->next;
     if (pp == pick) {
-      pick->connected_subchannel = nullptr;
+      pick->connected_subchannel.reset();
       GRPC_CLOSURE_SCHED(pick->on_complete,
                          GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
                              "Pick cancelled", &error, 1));
@@ -216,7 +217,7 @@ static void rr_cancel_picks_locked(grpc_lb_policy* pol,
     grpc_lb_policy_pick_state* next = pick->next;
     if ((pick->initial_metadata_flags & initial_metadata_flags_mask) ==
         initial_metadata_flags_eq) {
-      pick->connected_subchannel = nullptr;
+      pick->connected_subchannel.reset();
       GRPC_CLOSURE_SCHED(pick->on_complete,
                          GRPC_ERROR_CREATE_REFERENCING_FROM_STATIC_STRING(
                              "Pick cancelled", &error, 1));
@@ -262,8 +263,7 @@ static int rr_pick_locked(grpc_lb_policy* pol,
       /* readily available, report right away */
       grpc_lb_subchannel_data* sd =
           &p->subchannel_list->subchannels[next_ready_index];
-      pick->connected_subchannel =
-          GRPC_CONNECTED_SUBCHANNEL_REF(sd->connected_subchannel, "rr_picked");
+      pick->connected_subchannel = sd->connected_subchannel;
       if (pick->user_data != nullptr) {
         *pick->user_data = sd->user_data;
       }
@@ -272,8 +272,8 @@ static int rr_pick_locked(grpc_lb_policy* pol,
             GPR_DEBUG,
             "[RR %p] Picked target <-- Subchannel %p (connected %p) (sl %p, "
             "index %" PRIuPTR ")",
-            p, sd->subchannel, pick->connected_subchannel, sd->subchannel_list,
-            next_ready_index);
+            p, sd->subchannel, pick->connected_subchannel.get(),
+            sd->subchannel_list, next_ready_index);
       }
       /* only advance the last picked pointer if the selection was used */
       update_last_ready_subchannel_index_locked(p, next_ready_index);
@@ -373,7 +373,6 @@ static void update_lb_connectivity_status_locked(grpc_lb_subchannel_data* sd,
 
 static void rr_connectivity_changed_locked(void* arg, grpc_error* error) {
   grpc_lb_subchannel_data* sd = (grpc_lb_subchannel_data*)arg;
-  GPR_ASSERT(sd->curr_connectivity_state != GRPC_CHANNEL_SHUTDOWN);
   round_robin_lb_policy* p =
       (round_robin_lb_policy*)sd->subchannel_list->policy;
   if (grpc_lb_round_robin_trace.enabled()) {
@@ -408,6 +407,7 @@ static void rr_connectivity_changed_locked(void* arg, grpc_error* error) {
   // either the current or latest pending subchannel lists.
   GPR_ASSERT(sd->subchannel_list == p->subchannel_list ||
              sd->subchannel_list == p->latest_pending_subchannel_list);
+  GPR_ASSERT(sd->pending_connectivity_state_unsafe != GRPC_CHANNEL_SHUTDOWN);
   // Now that we're inside the combiner, copy the pending connectivity
   // state (which was set by the connectivity state watcher) to
   // curr_connectivity_state, which is what we use inside of the combiner.
@@ -419,18 +419,13 @@ static void rr_connectivity_changed_locked(void* arg, grpc_error* error) {
   // subchannel, if any.
   switch (sd->curr_connectivity_state) {
     case GRPC_CHANNEL_TRANSIENT_FAILURE: {
-      if (sd->connected_subchannel != nullptr) {
-        GRPC_CONNECTED_SUBCHANNEL_UNREF(
-            sd->connected_subchannel, "connected_subchannel_transient_failure");
-        sd->connected_subchannel = nullptr;
-      }
+      sd->connected_subchannel.reset();
       break;
     }
     case GRPC_CHANNEL_READY: {
-      if (sd->connected_subchannel == nullptr) {
-        sd->connected_subchannel = GRPC_CONNECTED_SUBCHANNEL_REF(
-            grpc_subchannel_get_connected_subchannel(sd->subchannel),
-            "connected");
+      if (!sd->connected_subchannel) {
+        sd->connected_subchannel =
+            grpc_subchannel_get_connected_subchannel(sd->subchannel);
       }
       if (sd->subchannel_list != p->subchannel_list) {
         // promote sd->subchannel_list to p->subchannel_list.
@@ -473,8 +468,7 @@ static void rr_connectivity_changed_locked(void* arg, grpc_error* error) {
       grpc_lb_policy_pick_state* pick;
       while ((pick = p->pending_picks)) {
         p->pending_picks = pick->next;
-        pick->connected_subchannel = GRPC_CONNECTED_SUBCHANNEL_REF(
-            selected->connected_subchannel, "rr_picked");
+        pick->connected_subchannel = selected->connected_subchannel;
         if (pick->user_data != nullptr) {
           *pick->user_data = selected->user_data;
         }
@@ -519,10 +513,9 @@ static void rr_ping_one_locked(grpc_lb_policy* pol, grpc_closure* on_initiate,
   if (next_ready_index < p->subchannel_list->num_subchannels) {
     grpc_lb_subchannel_data* selected =
         &p->subchannel_list->subchannels[next_ready_index];
-    grpc_core::ConnectedSubchannel* target = GRPC_CONNECTED_SUBCHANNEL_REF(
-        selected->connected_subchannel, "rr_ping");
+    grpc_core::RefCountedPtr<grpc_core::ConnectedSubchannel> target =
+        selected->connected_subchannel;
     target->Ping(on_initiate, on_ack);
-    GRPC_CONNECTED_SUBCHANNEL_UNREF(target, "rr_ping");
   } else {
     GRPC_CLOSURE_SCHED(on_initiate, GRPC_ERROR_CREATE_FROM_STATIC_STRING(
                                         "Round Robin not connected"));

src/core/ext/filters/client_channel/lb_policy/subchannel_list.cc

@@ -42,10 +42,7 @@ void grpc_lb_subchannel_data_unref_subchannel(grpc_lb_subchannel_data* sd,
     }
     GRPC_SUBCHANNEL_UNREF(sd->subchannel, reason);
     sd->subchannel = nullptr;
-    if (sd->connected_subchannel != nullptr) {
-      GRPC_CONNECTED_SUBCHANNEL_UNREF(sd->connected_subchannel, reason);
-      sd->connected_subchannel = nullptr;
-    }
+    sd->connected_subchannel.reset();
     if (sd->user_data != nullptr) {
       GPR_ASSERT(sd->user_data_vtable != nullptr);
       sd->user_data_vtable->destroy(sd->user_data);

src/core/ext/filters/client_channel/lb_policy/subchannel_list.h

@@ -22,6 +22,7 @@
 #include "src/core/ext/filters/client_channel/lb_policy_registry.h"
 #include "src/core/ext/filters/client_channel/subchannel.h"
 #include "src/core/lib/debug/trace.h"
+#include "src/core/lib/support/ref_counted_ptr.h"
 #include "src/core/lib/transport/connectivity_state.h"
 
 // TODO(roth): This code is intended to be shared between pick_first and
@@ -43,7 +44,7 @@ typedef struct {
   grpc_lb_subchannel_list* subchannel_list;
   /** subchannel itself */
   grpc_subchannel* subchannel;
-  grpc_core::ConnectedSubchannel* connected_subchannel;
+  grpc_core::RefCountedPtr<grpc_core::ConnectedSubchannel> connected_subchannel;
   /** Is a connectivity notification pending? */
   bool connectivity_notification_pending;
   /** notification that connectivity has changed on subchannel */

src/core/ext/filters/client_channel/subchannel.cc

@@ -108,13 +108,13 @@ struct grpc_subchannel {
       being setup */
   grpc_pollset_set* pollset_set;
 
-  /** active connection, or null; of type grpc_core::ConnectedSubchannel
-   */
-  gpr_atm connected_subchannel;
-
   /** mutex protecting remaining elements */
   gpr_mu mu;
 
+  /** active connection, or null; of type grpc_core::ConnectedSubchannel
+   */
+  grpc_core::RefCountedPtr<grpc_core::ConnectedSubchannel> connected_subchannel;
+
   /** have we seen a disconnection? */
   bool disconnected;
   /** are we connecting */
@@ -169,17 +169,6 @@ static void connection_destroy(void* arg, grpc_error* error) {
   gpr_free(stk);
 }
 
-grpc_core::ConnectedSubchannel* grpc_connected_subchannel_ref(
-    grpc_core::ConnectedSubchannel* c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) {
-  c->Ref(DEBUG_LOCATION, REF_REASON);
-  return c;
-}
-
-void grpc_connected_subchannel_unref(
-    grpc_core::ConnectedSubchannel* c GRPC_SUBCHANNEL_REF_EXTRA_ARGS) {
-  c->Unref(DEBUG_LOCATION, REF_REASON);
-}
-
 /*
  * grpc_subchannel implementation
  */
@@ -250,11 +239,7 @@ static void disconnect(grpc_subchannel* c) {
   c->disconnected = true;
   grpc_connector_shutdown(c->connector, GRPC_ERROR_CREATE_FROM_STATIC_STRING(
                                             "Subchannel disconnected"));
-  grpc_core::ConnectedSubchannel* con = GET_CONNECTED_SUBCHANNEL(c, no_barrier);
-  if (con != nullptr) {
-    GRPC_CONNECTED_SUBCHANNEL_UNREF(con, "disconnect");
-    gpr_atm_no_barrier_store(&c->connected_subchannel, (gpr_atm)0xdeadbeef);
-  }
+  c->connected_subchannel.reset();
   gpr_mu_unlock(&c->mu);
 }
 
@@ -458,7 +443,7 @@ static void maybe_start_connecting_locked(grpc_subchannel* c) {
     return;
   }
 
-  if (GET_CONNECTED_SUBCHANNEL(c, no_barrier) != nullptr) {
+  if (c->connected_subchannel) {
     /* Already connected: don't restart */
     return;
   }
@@ -536,38 +521,37 @@ static void on_connected_subchannel_connectivity_changed(void* p,
 
   gpr_mu_lock(mu);
 
-  auto* con = GET_CONNECTED_SUBCHANNEL(c, no_barrier);
-  /* if we failed just leave this closure */
-  if (connected_subchannel_watcher->connectivity_state >=
-      GRPC_CHANNEL_TRANSIENT_FAILURE) {
-    if (!c->disconnected && con != nullptr) {
-      GRPC_CONNECTED_SUBCHANNEL_UNREF(con, "transient_failure");
-      gpr_atm_no_barrier_store(&c->connected_subchannel, (gpr_atm) nullptr);
-      grpc_connectivity_state_set(&c->state_tracker,
-                                  GRPC_CHANNEL_TRANSIENT_FAILURE,
-                                  GRPC_ERROR_REF(error), "reflect_child");
-      c->backoff_begun = false;
-      c->backoff->Reset();
-      if (grpc_trace_stream_refcount.enabled()) {
-        gpr_log(GPR_INFO,
-                "Connected subchannel %p of subchannel %p has gone into %s. "
-                "Attempting to reconnect.",
-                con, c,
-                grpc_connectivity_state_name(
-                    connected_subchannel_watcher->connectivity_state));
+  switch (connected_subchannel_watcher->connectivity_state) {
+    case GRPC_CHANNEL_TRANSIENT_FAILURE:
+    case GRPC_CHANNEL_SHUTDOWN: {
+      if (!c->disconnected && c->connected_subchannel) {
+        if (grpc_trace_stream_refcount.enabled()) {
+          gpr_log(GPR_INFO,
+                  "Connected subchannel %p of subchannel %p has gone into %s. "
+                  "Attempting to reconnect.",
+                  c->connected_subchannel.get(), c,
+                  grpc_connectivity_state_name(
+                      connected_subchannel_watcher->connectivity_state));
+        }
+        c->connected_subchannel.reset();
+        grpc_connectivity_state_set(&c->state_tracker,
+                                    GRPC_CHANNEL_TRANSIENT_FAILURE,
+                                    GRPC_ERROR_REF(error), "reflect_child");
+        c->backoff_begun = false;
+        c->backoff->Reset();
+        maybe_start_connecting_locked(c);
+      } else {
+        connected_subchannel_watcher->connectivity_state =
+            GRPC_CHANNEL_SHUTDOWN;
       }
-      maybe_start_connecting_locked(c);
-    } else {
-      connected_subchannel_watcher->connectivity_state = GRPC_CHANNEL_SHUTDOWN;
+      break;
     }
-  } else {
-    grpc_connectivity_state_set(
-        &c->state_tracker, connected_subchannel_watcher->connectivity_state,
-        GRPC_ERROR_REF(error), "reflect_child");
-    if (connected_subchannel_watcher->connectivity_state <
-        GRPC_CHANNEL_TRANSIENT_FAILURE) {
+    default: {
+      grpc_connectivity_state_set(
+          &c->state_tracker, connected_subchannel_watcher->connectivity_state,
+          GRPC_ERROR_REF(error), "reflect_child");
       GRPC_SUBCHANNEL_WEAK_REF(c, "state_watcher");
-      con->NotifyOnStateChange(
+      c->connected_subchannel->NotifyOnStateChange(
           nullptr, &connected_subchannel_watcher->connectivity_state,
           &connected_subchannel_watcher->closure);
       connected_subchannel_watcher = nullptr;
@@ -619,22 +603,19 @@ static bool publish_transport_locked(grpc_subchannel* c) {
   }
 
   /* publish */
-  /* TODO(ctiller): this full barrier seems to clear up a TSAN failure.
-                    I'd have expected the rel_cas below to be enough, but
-                    seemingly it's not.
-                    Re-evaluate if we really need this. */
-  grpc_core::ConnectedSubchannel* con =
-      grpc_core::New<grpc_core::ConnectedSubchannel>(stk);
+  c->connected_subchannel.reset(
+      grpc_core::New<grpc_core::ConnectedSubchannel>(stk));
   gpr_atm_full_barrier();
-  GPR_ASSERT(gpr_atm_rel_cas(&c->connected_subchannel, 0, (gpr_atm)con));
+  gpr_log(GPR_INFO, "New connected subchannel at %p for subchannel %p",
+          c->connected_subchannel.get(), c);
 
   /* setup subchannel watching connected subchannel for changes; subchannel
      ref for connecting is donated to the state watcher */
   GRPC_SUBCHANNEL_WEAK_REF(c, "state_watcher");
   GRPC_SUBCHANNEL_WEAK_UNREF(c, "connecting");
-  con->NotifyOnStateChange(c->pollset_set,
-                           &connected_subchannel_watcher->connectivity_state,
-                           &connected_subchannel_watcher->closure);
+  c->connected_subchannel->NotifyOnStateChange(
+      c->pollset_set, &connected_subchannel_watcher->connectivity_state,
+      &connected_subchannel_watcher->closure);
 
   /* signal completion */
   grpc_connectivity_state_set(&c->state_tracker, GRPC_CHANNEL_READY,
@@ -684,7 +665,7 @@ static void subchannel_call_destroy(void* call, grpc_error* error) {
   grpc_core::ConnectedSubchannel* connection = c->connection;
   grpc_call_stack_destroy(SUBCHANNEL_CALL_TO_CALL_STACK(c), nullptr,
                           c->schedule_closure_after_destroy);
-  GRPC_CONNECTED_SUBCHANNEL_UNREF(connection, "subchannel_call");
+  connection->Unref(DEBUG_LOCATION, "subchannel_call");
   GPR_TIMER_END("grpc_subchannel_call_unref.destroy", 0);
 }
 
@@ -715,9 +696,12 @@ void grpc_subchannel_call_process_op(grpc_subchannel_call* call,
   GPR_TIMER_END("grpc_subchannel_call_process_op", 0);
 }
 
-grpc_core::ConnectedSubchannel* grpc_subchannel_get_connected_subchannel(
-    grpc_subchannel* c) {
-  return GET_CONNECTED_SUBCHANNEL(c, acq);
+grpc_core::RefCountedPtr<grpc_core::ConnectedSubchannel>
+grpc_subchannel_get_connected_subchannel(grpc_subchannel* c) {
+  gpr_mu_lock(&c->mu);
+  auto copy = c->connected_subchannel;
+  gpr_mu_unlock(&c->mu);
+  return copy;
 }
 
 const grpc_subchannel_key* grpc_subchannel_get_key(

src/core/ext/filters/client_channel/subchannel.h

@@ -24,6 +24,7 @@
 #include "src/core/lib/iomgr/polling_entity.h"
 #include "src/core/lib/support/arena.h"
 #include "src/core/lib/support/ref_counted.h"
+#include "src/core/lib/support/ref_counted_ptr.h"
 #include "src/core/lib/transport/connectivity_state.h"
 #include "src/core/lib/transport/metadata.h"
 
@@ -48,10 +49,6 @@ typedef struct grpc_subchannel_key grpc_subchannel_key;
   grpc_subchannel_weak_ref((p), __FILE__, __LINE__, (r))
 #define GRPC_SUBCHANNEL_WEAK_UNREF(p, r) \
   grpc_subchannel_weak_unref((p), __FILE__, __LINE__, (r))
-#define GRPC_CONNECTED_SUBCHANNEL_REF(p, r) \
-  grpc_connected_subchannel_ref((p), __FILE__, __LINE__, (r))
-#define GRPC_CONNECTED_SUBCHANNEL_UNREF(p, r) \
-  grpc_connected_subchannel_unref((p), __FILE__, __LINE__, (r))
 #define GRPC_SUBCHANNEL_CALL_REF(p, r) \
   grpc_subchannel_call_ref((p), __FILE__, __LINE__, (r))
 #define GRPC_SUBCHANNEL_CALL_UNREF(p, r) \
@@ -65,9 +62,6 @@ typedef struct grpc_subchannel_key grpc_subchannel_key;
 #define GRPC_SUBCHANNEL_UNREF(p, r) grpc_subchannel_unref((p))
 #define GRPC_SUBCHANNEL_WEAK_REF(p, r) grpc_subchannel_weak_ref((p))
 #define GRPC_SUBCHANNEL_WEAK_UNREF(p, r) grpc_subchannel_weak_unref((p))
-#define GRPC_CONNECTED_SUBCHANNEL_REF(p, r) grpc_connected_subchannel_ref((p))
-#define GRPC_CONNECTED_SUBCHANNEL_UNREF(p, r) \
-  grpc_connected_subchannel_unref((p))
 #define GRPC_SUBCHANNEL_CALL_REF(p, r) grpc_subchannel_call_ref((p))
 #define GRPC_SUBCHANNEL_CALL_UNREF(p, r) grpc_subchannel_call_unref((p))
 #define GRPC_SUBCHANNEL_REF_EXTRA_ARGS
@@ -111,10 +105,6 @@ grpc_subchannel* grpc_subchannel_weak_ref(
     grpc_subchannel* channel GRPC_SUBCHANNEL_REF_EXTRA_ARGS);
 void grpc_subchannel_weak_unref(
     grpc_subchannel* channel GRPC_SUBCHANNEL_REF_EXTRA_ARGS);
-grpc_core::ConnectedSubchannel* grpc_connected_subchannel_ref(
-    grpc_core::ConnectedSubchannel* channel GRPC_SUBCHANNEL_REF_EXTRA_ARGS);
-void grpc_connected_subchannel_unref(
-    grpc_core::ConnectedSubchannel* channel GRPC_SUBCHANNEL_REF_EXTRA_ARGS);
 void grpc_subchannel_call_ref(
     grpc_subchannel_call* call GRPC_SUBCHANNEL_REF_EXTRA_ARGS);
 void grpc_subchannel_call_unref(
@@ -130,10 +120,11 @@ void grpc_subchannel_notify_on_state_change(
     grpc_subchannel* channel, grpc_pollset_set* interested_parties,
     grpc_connectivity_state* state, grpc_closure* notify);
 
-/** retrieve the grpc_core::ConnectedSubchannel - or NULL if called before
-    the subchannel becomes connected */
-grpc_core::ConnectedSubchannel* grpc_subchannel_get_connected_subchannel(
-    grpc_subchannel* subchannel);
+/** retrieve the grpc_core::ConnectedSubchannel - or nullptr if not connected
+ * (which may happen before it initially connects or during transient failures)
+ * */
+grpc_core::RefCountedPtr<grpc_core::ConnectedSubchannel>
+grpc_subchannel_get_connected_subchannel(grpc_subchannel* c);
 
 /** return the subchannel index key for \a subchannel */
 const grpc_subchannel_key* grpc_subchannel_get_key(

src/core/lib/debug/trace.h

@@ -88,10 +88,9 @@ class TraceFlag {
 #ifndef NDEBUG
 typedef TraceFlag DebugOnlyTraceFlag;
 #else
-class DebugOnlyTraceFlag : public TraceFlag {
+class DebugOnlyTraceFlag {
  public:
-  DebugOnlyTraceFlag(bool default_enabled, const char* name)
-      : TraceFlag(default_enabled, name) {}
+  DebugOnlyTraceFlag(bool default_enabled, const char* name) {}
   bool enabled() { return false; }
 
  private:

src/core/lib/support/ref_counted.h

@@ -106,13 +106,19 @@ class RefCountedWithTracing {
   template <typename T>
   friend void Delete(T*);
 
-  RefCountedWithTracing() : RefCountedWithTracing(nullptr) {}
+  RefCountedWithTracing()
+      : RefCountedWithTracing(static_cast<TraceFlag*>(nullptr)) {}
 
   explicit RefCountedWithTracing(TraceFlag* trace_flag)
       : trace_flag_(trace_flag) {
     gpr_ref_init(&refs_, 1);
   }
 
+#ifdef NDEBUG
+  explicit RefCountedWithTracing(DebugOnlyTraceFlag* trace_flag)
+      : RefCountedWithTracing() {}
+#endif
+
   virtual ~RefCountedWithTracing() {}
 
  private:

src/core/lib/support/ref_counted_ptr.h

@@ -76,6 +76,8 @@ class RefCountedPtr {
   T& operator*() const { return *value_; }
   T* operator->() const { return value_; }
 
+  explicit operator bool() const { return get() != nullptr; }
+
  private:
   T* value_ = nullptr;
 };