From 2c5f0c22cdd66687f0c36f648f3fc3a05ae90309 Mon Sep 17 00:00:00 2001 From: Kannan J Date: Thu, 28 Mar 2024 00:10:13 +0530 Subject: [PATCH] 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. --- .../io/grpc/internal/ManagedChannelImpl.java | 2 + .../grpc/internal/ManagedChannelImplTest.java | 45 +++++++++---------- .../ServiceConfigErrorHandlingTest.java | 4 +- 3 files changed, 24 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 5d600d1ca5e..a25886797e0 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/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; diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 04926cc25a5..5cd0d40fc94 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/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 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 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); } -} +} \ No newline at end of file diff --git a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java b/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java index 0d050a09a9a..697b55c9027 100644 --- a/core/src/test/java/io/grpc/internal/ServiceConfigErrorHandlingTest.java +++ b/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