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 5d600d1ca5..a25886797e 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.checkArgument; 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 @@ final class ManagedChannelImpl extends ManagedChannel implements // 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 04926cc25a..5cd0d40fc9 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 class ManagedChannelImplTest { 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 class ManagedChannelImplTest { 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 class ManagedChannelImplTest { 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 class ManagedChannelImplTest { 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 class ManagedChannelImplTest { // 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 class ManagedChannelImplTest { .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 class ManagedChannelImplTest { // 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 class ManagedChannelImplTest { 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 class ManagedChannelImplTest { 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 class ManagedChannelImplTest { 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 @@ public class ManagedChannelImplTest { 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 0d050a09a9..697b55c902 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 class ServiceConfigErrorHandlingTest { 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 class ServiceConfigErrorHandlingTest { 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