Merge pull request #17383 from markdroth/pick_first_choose_ready_subchannel_at_startup

Change pick_first to immediately select the first subchannel in READY state.

1 files changed, 35 insertions, 39 deletions

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 d454401a66..d1a05f1255 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
@@ -380,6 +380,31 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args,
     selected_ = nullptr;
     return;
   }
+  // If one of the subchannels in the new list is already in state
+  // READY, then select it immediately.  This can happen when the
+  // currently selected subchannel is also present in the update.  It
+  // can also happen if one of the subchannels in the update is already
+  // in the subchannel index because it's in use by another channel.
+  for (size_t i = 0; i < subchannel_list->num_subchannels(); ++i) {
+    PickFirstSubchannelData* sd = subchannel_list->subchannel(i);
+    grpc_error* error = GRPC_ERROR_NONE;
+    grpc_connectivity_state state = sd->CheckConnectivityStateLocked(&error);
+    GRPC_ERROR_UNREF(error);
+    if (state == GRPC_CHANNEL_READY) {
+      subchannel_list_ = std::move(subchannel_list);
+      sd->ProcessUnselectedReadyLocked();
+      sd->StartConnectivityWatchLocked();
+      // If there was a previously pending update (which may or may
+      // 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();
+      // Make sure that subsequent calls to ExitIdleLocked() don't cause
+      // us to start watching a subchannel other than the one we've
+      // selected.
+      started_picking_ = true;
+      return;
+    }
+  }
   if (selected_ == nullptr) {
     // We don't yet have a selected subchannel, so replace the current
     // subchannel list immediately.
@@ -387,46 +412,14 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args,
     // If we've started picking, start trying to connect to the first
     // subchannel in the new list.
     if (started_picking_) {
-      subchannel_list_->subchannel(0)
-          ->CheckConnectivityStateAndStartWatchingLocked();
+      // Note: No need to use CheckConnectivityStateAndStartWatchingLocked()
+      // here, since we've already checked the initial connectivity
+      // state of all subchannels above.
+      subchannel_list_->subchannel(0)->StartConnectivityWatchLocked();
     }
   } else {
-    // We do have a selected subchannel.
-    // Check if it's present in the new list.  If so, we're done.
-    for (size_t i = 0; i < subchannel_list->num_subchannels(); ++i) {
-      PickFirstSubchannelData* sd = subchannel_list->subchannel(i);
-      if (sd->subchannel() == selected_->subchannel()) {
-        // The currently selected subchannel is in the update: we are done.
-        if (grpc_lb_pick_first_trace.enabled()) {
-          gpr_log(GPR_INFO,
-                  "Pick First %p found already selected subchannel %p "
-                  "at update index %" PRIuPTR " of %" PRIuPTR "; update done",
-                  this, selected_->subchannel(), i,
-                  subchannel_list->num_subchannels());
-        }
-        // Make sure it's in state READY.  It might not be if we grabbed
-        // the combiner while a connectivity state notification
-        // informing us otherwise is pending.
-        // Note that CheckConnectivityStateLocked() also takes a ref to
-        // the connected subchannel.
-        grpc_error* error = GRPC_ERROR_NONE;
-        if (sd->CheckConnectivityStateLocked(&error) == GRPC_CHANNEL_READY) {
-          selected_ = sd;
-          subchannel_list_ = std::move(subchannel_list);
-          sd->StartConnectivityWatchLocked();
-          // If there was a previously pending update (which may or may
-          // 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;
-        }
-        GRPC_ERROR_UNREF(error);
-      }
-    }
-    // Not keeping the previous selected subchannel, so set the latest
-    // pending subchannel list to the new subchannel list.  We will wait
-    // for it to report READY before swapping it into the current
-    // subchannel list.
+    // We do have a selected subchannel, so keep using it until one of
+    // the subchannels in the new list reports READY.
     if (latest_pending_subchannel_list_ != nullptr) {
       if (grpc_lb_pick_first_trace.enabled()) {
         gpr_log(GPR_INFO,
@@ -440,8 +433,11 @@ void PickFirst::UpdateLocked(const grpc_channel_args& args,
     // If we've started picking, start trying to connect to the first
     // subchannel in the new list.
     if (started_picking_) {
+      // Note: No need to use CheckConnectivityStateAndStartWatchingLocked()
+      // here, since we've already checked the initial connectivity
+      // state of all subchannels above.
       latest_pending_subchannel_list_->subchannel(0)
-          ->CheckConnectivityStateAndStartWatchingLocked();
+          ->StartConnectivityWatchLocked();
     }
   }
 }