core: Transition to CONNECTING immediately when exiting idle

The name resolver takes some time before it returns addresses. While waiting the channel will be IDLE instead of the proper CONNECTING. This generally doesn't matter since RPCs behave similarly for IDLE and CONNECTING, but is confusing for users when watching channel.getState() closely.

Fixes #10517.

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

@@ -20,6 +20,7 @@
 import static com.google.common.base.Preconditions.checkNotNull;
 import static com.google.common.base.Preconditions.checkState;
 import static io.grpc.ClientStreamTracer.NAME_RESOLUTION_DELAYED;
+import static io.grpc.ConnectivityState.CONNECTING;
 import static io.grpc.ConnectivityState.IDLE;
 import static io.grpc.ConnectivityState.SHUTDOWN;
 import static io.grpc.ConnectivityState.TRANSIENT_FAILURE;
@@ -423,6 +424,7 @@ void exitIdleMode() {
     // may throw. We don't want to confuse our state, even if we will enter panic mode.
     this.lbHelper = lbHelper;
 
+    channelStateManager.gotoState(CONNECTING);
     NameResolverListener listener = new NameResolverListener(lbHelper, nameResolver);
     nameResolver.start(listener);
     nameResolverStarted = true;

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

@@ -2338,7 +2338,7 @@ public void getState_loadBalancerSupportsChannelState() {
     channelBuilder.nameResolverFactory(
         new FakeNameResolverFactory.Builder(expectedUri).setResolvedAtStart(false).build());
     createChannel();
-    assertEquals(IDLE, channel.getState(false));
+    assertEquals(CONNECTING, channel.getState(false));
 
     updateBalancingStateSafely(helper, TRANSIENT_FAILURE, mockPicker);
     assertEquals(TRANSIENT_FAILURE, channel.getState(false));
@@ -2395,21 +2395,21 @@ public void run() {
     channelBuilder.nameResolverFactory(
         new FakeNameResolverFactory.Builder(expectedUri).setResolvedAtStart(false).build());
     createChannel();
-    assertEquals(IDLE, channel.getState(false));
+    assertEquals(CONNECTING, channel.getState(false));
 
-    channel.notifyWhenStateChanged(IDLE, onStateChanged);
+    channel.notifyWhenStateChanged(CONNECTING, onStateChanged);
     executor.runDueTasks();
     assertFalse(stateChanged.get());
 
-    // state change from IDLE to CONNECTING
-    updateBalancingStateSafely(helper, CONNECTING, mockPicker);
+    // state change from CONNECTING to IDLE
+    updateBalancingStateSafely(helper, IDLE, mockPicker);
     // onStateChanged callback should run
     executor.runDueTasks();
     assertTrue(stateChanged.get());
 
-    // clear and test form CONNECTING
+    // clear and test form IDLE
     stateChanged.set(false);
-    channel.notifyWhenStateChanged(IDLE, onStateChanged);
+    channel.notifyWhenStateChanged(CONNECTING, onStateChanged);
     // onStateChanged callback should run immediately
     executor.runDueTasks();
     assertTrue(stateChanged.get());
@@ -2428,8 +2428,8 @@ public void run() {
     channelBuilder.nameResolverFactory(
         new FakeNameResolverFactory.Builder(expectedUri).setResolvedAtStart(false).build());
     createChannel();
-    assertEquals(IDLE, channel.getState(false));
-    channel.notifyWhenStateChanged(IDLE, onStateChanged);
+    assertEquals(CONNECTING, channel.getState(false));
+    channel.notifyWhenStateChanged(CONNECTING, onStateChanged);
     executor.runDueTasks();
     assertFalse(stateChanged.get());
 
@@ -2452,9 +2452,6 @@ public void stateIsIdleOnIdleTimeout() {
     long idleTimeoutMillis = 2000L;
     channelBuilder.idleTimeout(idleTimeoutMillis, TimeUnit.MILLISECONDS);
     createChannel();
-    assertEquals(IDLE, channel.getState(false));
-
-    updateBalancingStateSafely(helper, CONNECTING, mockPicker);
     assertEquals(CONNECTING, channel.getState(false));
 
     timer.forwardNanos(TimeUnit.MILLISECONDS.toNanos(idleTimeoutMillis));
@@ -2677,11 +2674,11 @@ public void idleTimeoutAndReconnect() {
 
     // Updating on the old helper (whose balancer has been shutdown) does not change the channel
     // state.
-    updateBalancingStateSafely(helper, CONNECTING, mockPicker);
-    assertEquals(IDLE, channel.getState(false));
-
-    updateBalancingStateSafely(helper2, CONNECTING, mockPicker);
+    updateBalancingStateSafely(helper, IDLE, mockPicker);
     assertEquals(CONNECTING, channel.getState(false));
+
+    updateBalancingStateSafely(helper2, IDLE, mockPicker);
+    assertEquals(IDLE, channel.getState(false));
   }
 
   @Test
@@ -2695,7 +2692,7 @@ public void idleMode_resetsDelayedTransportPicker() {
             .setServers(Collections.singletonList(new EquivalentAddressGroup(socketAddress)))
             .build());
     createChannel();
-    assertEquals(IDLE, channel.getState(false));
+    assertEquals(CONNECTING, channel.getState(false));
 
     // This call will be buffered in delayedTransport
     ClientCall<String, Integer> call = channel.newCall(method, CallOptions.DEFAULT);
@@ -2790,7 +2787,7 @@ public void enterIdle_exitsIdleIfDelayedStreamPending() {
     // enterIdle() will shut down the name resolver and lb policy used to get a pick for the delayed
     // call
     channel.enterIdle();
-    assertEquals(IDLE, channel.getState(false));
+    assertEquals(CONNECTING, channel.getState(false));
 
     // enterIdle() will restart the delayed call by exiting idle. This creates a new helper.
     ArgumentCaptor<Helper> helperCaptor = ArgumentCaptor.forClass(Helper.class);
@@ -2912,14 +2909,14 @@ public void updateBalancingStateWithShutdownShouldBeIgnored() {
     channelBuilder.nameResolverFactory(
         new FakeNameResolverFactory.Builder(expectedUri).setResolvedAtStart(false).build());
     createChannel();
-    assertEquals(IDLE, channel.getState(false));
+    assertEquals(CONNECTING, channel.getState(false));
 
     Runnable onStateChanged = mock(Runnable.class);
-    channel.notifyWhenStateChanged(IDLE, onStateChanged);
+    channel.notifyWhenStateChanged(CONNECTING, onStateChanged);
 
     updateBalancingStateSafely(helper, SHUTDOWN, mockPicker);
 
-    assertEquals(IDLE, channel.getState(false));
+    assertEquals(CONNECTING, channel.getState(false));
     executor.runDueTasks();
     verify(onStateChanged, never()).run();
   }
@@ -3222,8 +3219,6 @@ public void channelsAndSubchannels_instrumented_state() throws Exception {
     verify(mockLoadBalancerProvider).newLoadBalancer(helperCaptor.capture());
     helper = helperCaptor.getValue();
 
-    assertEquals(IDLE, getStats(channel).state);
-    updateBalancingStateSafely(helper, CONNECTING, mockPicker);
     assertEquals(CONNECTING, getStats(channel).state);
 
     AbstractSubchannel subchannel =
@@ -3434,7 +3429,7 @@ public void channelsAndSubchannels_oob_instrumented_state() throws Exception {
     assertEquals(READY, getStats(oobChannel).state);
 
     // oobchannel state is separate from the ManagedChannel
-    assertEquals(IDLE, getStats(channel).state);
+    assertEquals(CONNECTING, getStats(channel).state);
     channel.shutdownNow();
     assertEquals(SHUTDOWN, getStats(channel).state);
     assertEquals(SHUTDOWN, getStats(oobChannel).state);
@@ -4536,4 +4531,4 @@ private static ManagedChannelServiceConfig createManagedChannelServiceConfig(
     return ManagedChannelServiceConfig
         .fromServiceConfig(rawServiceConfig, true, 3, 4, policySelection);
   }
-}
+}

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

@@ -277,7 +277,7 @@ public void emptyAddresses_validConfig_2ndResolution_lbNeedsAddress() throws Exc
     assertThat(resolvedAddresses.getLoadBalancingPolicyConfig()).isEqualTo("12");
     verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class));
 
-    assertThat(channel.getState(true)).isEqualTo(ConnectivityState.IDLE);
+    assertThat(channel.getState(true)).isEqualTo(ConnectivityState.CONNECTING);
 
     reset(mockLoadBalancer);
     nameResolverFactory.servers.clear();
@@ -480,7 +480,7 @@ public void invalidConfig_2ndResolution() throws Exception {
     assertThat(newResolvedAddress.getLoadBalancingPolicyConfig()).isEqualTo("1st raw config");
     assertThat(channel.getConfigSelector()).isSameInstanceAs(configSelector);
     verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class));
-    assertThat(channel.getState(false)).isEqualTo(ConnectivityState.IDLE);
+    assertThat(channel.getState(false)).isEqualTo(ConnectivityState.CONNECTING);
   }
 
   @Test