Fix grpclb incompatibility

gRPC-LB's tests failed when using FixedResultPicker because it didn't
transition to READY. This was because GrpclbState.maybeUpdatePicker()
didn't consider the ConnectivityState when checking if anything had
changed. The old PickFirstLoadBalancer.Picker didn't implement equals()
so every update was considered different, ignoring the connectivity
state. PickFirstLeafLoadBalancer wasn't impacted because it didn't pick
a subchannel when in CONNECTING, and neither did PickFirstLoadBalancer
after the first update.

This is fixed on both sides: PickFirstLoadBalancer no londer returns the
useless subchannel when picking during CONNECTING, and gRPC-LB now
checks the connectivity state.

This now causes what appears to be a bug in RLS. That is still being
investigated.

Author: ejona86

core/src/main/java/io/grpc/internal/PickFirstLoadBalancer.java

@@ -86,8 +86,7 @@ public void onSubchannelState(ConnectivityStateInfo stateInfo) {
 
       // The channel state does not get updated when doing name resolving today, so for the moment
       // let LB report CONNECTION and call subchannel.requestConnection() immediately.
-      updateBalancingState(
-          CONNECTING, new FixedResultPicker(PickResult.withSubchannel(subchannel)));
+      updateBalancingState(CONNECTING, new FixedResultPicker(PickResult.withNoResult()));
       subchannel.requestConnection();
     } else {
       subchannel.updateAddresses(servers);

core/src/test/java/io/grpc/internal/PickFirstLoadBalancerTest.java

@@ -219,7 +219,7 @@ public void refreshNameResolutionAfterSubchannelConnectionBroken() {
     inOrder.verify(mockSubchannel).start(stateListenerCaptor.capture());
     SubchannelStateListener stateListener = stateListenerCaptor.getValue();
     inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
-    assertSame(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());
+    assertThat(pickerCaptor.getValue().pickSubchannel(mockArgs).hasResult()).isFalse();
     inOrder.verify(mockSubchannel).requestConnection();
 
     stateListener.onSubchannelState(ConnectivityStateInfo.forNonError(CONNECTING));
@@ -278,7 +278,7 @@ public void pickAfterResolvedAndChanged() throws Exception {
     assertThat(args.getAddresses()).isEqualTo(servers);
     inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
     verify(mockSubchannel).requestConnection();
-    assertEquals(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());
+    assertThat(pickerCaptor.getValue().pickSubchannel(mockArgs).hasResult()).isFalse();
 
     loadBalancer.acceptResolvedAddresses(
         ResolvedAddresses.newBuilder().setAddresses(newServers).setAttributes(affinity).build());
@@ -300,7 +300,7 @@ public void pickAfterStateChangeAfterResolution() throws Exception {
     verify(mockSubchannel).start(stateListenerCaptor.capture());
     SubchannelStateListener stateListener = stateListenerCaptor.getValue();
     verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
-    Subchannel subchannel = pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel();
+    assertThat(pickerCaptor.getValue().pickSubchannel(mockArgs).hasResult()).isFalse();
     reset(mockHelper);
     when(mockHelper.getSynchronizationContext()).thenReturn(syncContext);
 
@@ -317,7 +317,7 @@ public void pickAfterStateChangeAfterResolution() throws Exception {
 
     stateListener.onSubchannelState(ConnectivityStateInfo.forNonError(READY));
     inOrder.verify(mockHelper).updateBalancingState(eq(READY), pickerCaptor.capture());
-    assertEquals(subchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());
+    assertEquals(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());
 
     verify(mockHelper, atLeast(0)).getSynchronizationContext();  // Don't care
     verifyNoMoreInteractions(mockHelper);
@@ -405,8 +405,7 @@ public void nameResolutionSuccessAfterError() throws Exception {
     inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
     verify(mockSubchannel).requestConnection();
 
-    assertEquals(mockSubchannel, pickerCaptor.getValue().pickSubchannel(mockArgs)
-        .getSubchannel());
+    assertThat(pickerCaptor.getValue().pickSubchannel(mockArgs).hasResult()).isFalse();
 
     assertEquals(pickerCaptor.getValue().pickSubchannel(mockArgs),
         pickerCaptor.getValue().pickSubchannel(mockArgs));

grpclb/src/main/java/io/grpc/grpclb/GrpclbState.java

@@ -187,6 +187,7 @@ enum Mode {
   private List<DropEntry> dropList = Collections.emptyList();
   // Contains only non-drop, i.e., backends from the round-robin list from the balancer.
   private List<BackendEntry> backendList = Collections.emptyList();
+  private ConnectivityState currentState = ConnectivityState.CONNECTING;
   private RoundRobinPicker currentPicker =
       new RoundRobinPicker(Collections.<DropEntry>emptyList(), Arrays.asList(BUFFER_ENTRY));
   private boolean requestConnectionPending;
@@ -937,10 +938,12 @@ private void maybeUpdatePicker(ConnectivityState state, RoundRobinPicker picker)
     // Discard the new picker if we are sure it won't make any difference, in order to save
     // re-processing pending streams, and avoid unnecessary resetting of the pointer in
     // RoundRobinPicker.
-    if (picker.dropList.equals(currentPicker.dropList)
+    if (state.equals(currentState)
+        && picker.dropList.equals(currentPicker.dropList)
         && picker.pickList.equals(currentPicker.pickList)) {
       return;
     }
+    currentState = state;
     currentPicker = picker;
     helper.updateBalancingState(state, picker);
   }