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 3bffadcc32e..1182700e9c4 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 @@ -346,6 +346,9 @@ void PickFirst::PickFirstSubchannelData::ProcessConnectivityChangeLocked( // TODO(qianchengz): We may want to request re-resolution in // ExitIdleLocked(). p->channel_control_helper()->RequestReresolution(); + // TODO(roth): We chould check the connectivity states of all the + // subchannels here, just in case one of them happens to be READY, + // and we could switch to that rather than going IDLE. // Enter idle. p->idle_ = true; p->selected_ = nullptr; @@ -389,13 +392,10 @@ void PickFirst::PickFirstSubchannelData::ProcessConnectivityChangeLocked( case GRPC_CHANNEL_READY: // Already handled this case above, so this should not happen. GPR_UNREACHABLE_CODE(break); - case GRPC_CHANNEL_TRANSIENT_FAILURE: - case GRPC_CHANNEL_IDLE: { - PickFirstSubchannelData* sd = this; - size_t next_index = - (sd->Index() + 1) % subchannel_list()->num_subchannels(); + case GRPC_CHANNEL_TRANSIENT_FAILURE: { + size_t next_index = (Index() + 1) % subchannel_list()->num_subchannels(); subchannel_list()->set_attempting_index(next_index); - sd = subchannel_list()->subchannel(next_index); + PickFirstSubchannelData* sd = subchannel_list()->subchannel(next_index); // If we're tried all subchannels, set state to TRANSIENT_FAILURE. if (sd->Index() == 0) { if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_pick_first_trace)) { @@ -432,7 +432,20 @@ void PickFirst::PickFirstSubchannelData::ProcessConnectivityChangeLocked( absl::make_unique(status)); } } - sd->subchannel()->RequestConnection(); + // If the next subchannel is in IDLE, trigger a connection attempt. + // If it's in READY, we can't get here, because we would already + // have selected the subchannel above. + // If it's already in CONNECTING, we don't need to do this. + // If it's in TRANSIENT_FAILURE, then we will trigger the + // connection attempt later when it reports IDLE. + auto sd_state = sd->connectivity_state(); + if (sd_state.has_value() && *sd_state == GRPC_CHANNEL_IDLE) { + sd->subchannel()->RequestConnection(); + } + break; + } + case GRPC_CHANNEL_IDLE: { + subchannel()->RequestConnection(); break; } case GRPC_CHANNEL_CONNECTING: { diff --git a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc index bee04986353..c75a8d1a397 100644 --- a/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc +++ b/src/core/ext/filters/client_channel/lb_policy/ring_hash/ring_hash.cc @@ -711,10 +711,9 @@ void RingHash::RingHashSubchannelData::ProcessConnectivityChangeLocked( } GPR_ASSERT(subchannel() != nullptr); // If this is not the initial state notification and the new state is - // TRANSIENT_FAILURE or IDLE, re-resolve and attempt to reconnect. - // Note that we don't want to do this on the initial state - // notification, because that would result in an endless loop of - // re-resolution. + // TRANSIENT_FAILURE or IDLE, re-resolve. + // Note that we don't want to do this on the initial state notification, + // because that would result in an endless loop of re-resolution. if (old_state.has_value() && (new_state == GRPC_CHANNEL_TRANSIENT_FAILURE || new_state == GRPC_CHANNEL_IDLE)) { if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_ring_hash_trace)) { 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 de0f39a7ab1..e99997e1bc9 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 @@ -131,10 +131,6 @@ class RoundRobin : public LoadBalancingPolicy { // any references to subchannels, since the subchannels' // pollset_sets will include the LB policy's pollset_set. policy->Ref(DEBUG_LOCATION, "subchannel_list").release(); - // Start connecting to all subchannels. - for (size_t i = 0; i < num_subchannels(); i++) { - subchannel(i)->subchannel()->RequestConnection(); - } } ~RoundRobinSubchannelList() override { @@ -417,10 +413,9 @@ void RoundRobin::RoundRobinSubchannelData::ProcessConnectivityChangeLocked( RoundRobin* p = static_cast(subchannel_list()->policy()); GPR_ASSERT(subchannel() != nullptr); // If this is not the initial state notification and the new state is - // TRANSIENT_FAILURE or IDLE, re-resolve and attempt to reconnect. - // Note that we don't want to do this on the initial state - // notification, because that would result in an endless loop of - // re-resolution. + // TRANSIENT_FAILURE or IDLE, re-resolve. + // Note that we don't want to do this on the initial state notification, + // because that would result in an endless loop of re-resolution. if (old_state.has_value() && (new_state == GRPC_CHANNEL_TRANSIENT_FAILURE || new_state == GRPC_CHANNEL_IDLE)) { if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_round_robin_trace)) { @@ -429,6 +424,13 @@ void RoundRobin::RoundRobinSubchannelData::ProcessConnectivityChangeLocked( subchannel(), ConnectivityStateName(new_state)); } p->channel_control_helper()->RequestReresolution(); + } + if (new_state == GRPC_CHANNEL_IDLE) { + if (GRPC_TRACE_FLAG_ENABLED(grpc_lb_round_robin_trace)) { + gpr_log(GPR_INFO, + "[RR %p] Subchannel %p reported IDLE; requesting connection", p, + subchannel()); + } subchannel()->RequestConnection(); } // Update logical connectivity state. diff --git a/src/core/ext/filters/client_channel/subchannel.cc b/src/core/ext/filters/client_channel/subchannel.cc index e66e705fecb..1c75d6949a1 100644 --- a/src/core/ext/filters/client_channel/subchannel.cc +++ b/src/core/ext/filters/client_channel/subchannel.cc @@ -793,8 +793,6 @@ void Subchannel::RequestConnection() { MutexLock lock(&mu_); if (state_ == GRPC_CHANNEL_IDLE) { StartConnectingLocked(); - } else if (state_ == GRPC_CHANNEL_TRANSIENT_FAILURE) { - connection_requested_ = true; } } @@ -899,18 +897,9 @@ void Subchannel::OnRetryTimer() { void Subchannel::OnRetryTimerLocked() { if (shutdown_) return; - if (connection_requested_) { - gpr_log(GPR_INFO, - "subchannel %p %s: connection attempt requested while backoff " - "timer was pending, retrying now", - this, key_.ToString().c_str()); - connection_requested_ = false; - StartConnectingLocked(); - } else { - gpr_log(GPR_INFO, "subchannel %p %s: backoff delay elapsed, reporting IDLE", - this, key_.ToString().c_str()); - SetConnectivityStateLocked(GRPC_CHANNEL_IDLE, absl::OkStatus()); - } + gpr_log(GPR_INFO, "subchannel %p %s: backoff delay elapsed, reporting IDLE", + this, key_.ToString().c_str()); + SetConnectivityStateLocked(GRPC_CHANNEL_IDLE, absl::OkStatus()); } void Subchannel::StartConnectingLocked() { diff --git a/src/core/ext/filters/client_channel/subchannel.h b/src/core/ext/filters/client_channel/subchannel.h index 9a9ff817c3d..59783f59c05 100644 --- a/src/core/ext/filters/client_channel/subchannel.h +++ b/src/core/ext/filters/client_channel/subchannel.h @@ -390,9 +390,6 @@ class Subchannel : public DualRefCounted { bool shutdown_ ABSL_GUARDED_BY(mu_) = false; - // Records if RequestConnection() was called while in backoff. - bool connection_requested_ ABSL_GUARDED_BY(mu_) = false; - // Connectivity state tracking. // Note that the connectivity state implies the state of the // Subchannel object: