Skip to content

Commit 2c5f0c2

Browse files
authoredMar 27, 2024··
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.
1 parent f7ee5f3 commit 2c5f0c2

File tree

3 files changed

+24
-27
lines changed

3 files changed

+24
-27
lines changed
 

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

+2
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
import static com.google.common.base.Preconditions.checkNotNull;
2121
import static com.google.common.base.Preconditions.checkState;
2222
import static io.grpc.ClientStreamTracer.NAME_RESOLUTION_DELAYED;
23+
import static io.grpc.ConnectivityState.CONNECTING;
2324
import static io.grpc.ConnectivityState.IDLE;
2425
import static io.grpc.ConnectivityState.SHUTDOWN;
2526
import static io.grpc.ConnectivityState.TRANSIENT_FAILURE;
@@ -423,6 +424,7 @@ void exitIdleMode() {
423424
// may throw. We don't want to confuse our state, even if we will enter panic mode.
424425
this.lbHelper = lbHelper;
425426

427+
channelStateManager.gotoState(CONNECTING);
426428
NameResolverListener listener = new NameResolverListener(lbHelper, nameResolver);
427429
nameResolver.start(listener);
428430
nameResolverStarted = true;

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

+20-25
Original file line numberDiff line numberDiff line change
@@ -2338,7 +2338,7 @@ public void getState_loadBalancerSupportsChannelState() {
23382338
channelBuilder.nameResolverFactory(
23392339
new FakeNameResolverFactory.Builder(expectedUri).setResolvedAtStart(false).build());
23402340
createChannel();
2341-
assertEquals(IDLE, channel.getState(false));
2341+
assertEquals(CONNECTING, channel.getState(false));
23422342

23432343
updateBalancingStateSafely(helper, TRANSIENT_FAILURE, mockPicker);
23442344
assertEquals(TRANSIENT_FAILURE, channel.getState(false));
@@ -2395,21 +2395,21 @@ public void run() {
23952395
channelBuilder.nameResolverFactory(
23962396
new FakeNameResolverFactory.Builder(expectedUri).setResolvedAtStart(false).build());
23972397
createChannel();
2398-
assertEquals(IDLE, channel.getState(false));
2398+
assertEquals(CONNECTING, channel.getState(false));
23992399

2400-
channel.notifyWhenStateChanged(IDLE, onStateChanged);
2400+
channel.notifyWhenStateChanged(CONNECTING, onStateChanged);
24012401
executor.runDueTasks();
24022402
assertFalse(stateChanged.get());
24032403

2404-
// state change from IDLE to CONNECTING
2405-
updateBalancingStateSafely(helper, CONNECTING, mockPicker);
2404+
// state change from CONNECTING to IDLE
2405+
updateBalancingStateSafely(helper, IDLE, mockPicker);
24062406
// onStateChanged callback should run
24072407
executor.runDueTasks();
24082408
assertTrue(stateChanged.get());
24092409

2410-
// clear and test form CONNECTING
2410+
// clear and test form IDLE
24112411
stateChanged.set(false);
2412-
channel.notifyWhenStateChanged(IDLE, onStateChanged);
2412+
channel.notifyWhenStateChanged(CONNECTING, onStateChanged);
24132413
// onStateChanged callback should run immediately
24142414
executor.runDueTasks();
24152415
assertTrue(stateChanged.get());
@@ -2428,8 +2428,8 @@ public void run() {
24282428
channelBuilder.nameResolverFactory(
24292429
new FakeNameResolverFactory.Builder(expectedUri).setResolvedAtStart(false).build());
24302430
createChannel();
2431-
assertEquals(IDLE, channel.getState(false));
2432-
channel.notifyWhenStateChanged(IDLE, onStateChanged);
2431+
assertEquals(CONNECTING, channel.getState(false));
2432+
channel.notifyWhenStateChanged(CONNECTING, onStateChanged);
24332433
executor.runDueTasks();
24342434
assertFalse(stateChanged.get());
24352435

@@ -2452,9 +2452,6 @@ public void stateIsIdleOnIdleTimeout() {
24522452
long idleTimeoutMillis = 2000L;
24532453
channelBuilder.idleTimeout(idleTimeoutMillis, TimeUnit.MILLISECONDS);
24542454
createChannel();
2455-
assertEquals(IDLE, channel.getState(false));
2456-
2457-
updateBalancingStateSafely(helper, CONNECTING, mockPicker);
24582455
assertEquals(CONNECTING, channel.getState(false));
24592456

24602457
timer.forwardNanos(TimeUnit.MILLISECONDS.toNanos(idleTimeoutMillis));
@@ -2677,11 +2674,11 @@ public void idleTimeoutAndReconnect() {
26772674

26782675
// Updating on the old helper (whose balancer has been shutdown) does not change the channel
26792676
// state.
2680-
updateBalancingStateSafely(helper, CONNECTING, mockPicker);
2681-
assertEquals(IDLE, channel.getState(false));
2682-
2683-
updateBalancingStateSafely(helper2, CONNECTING, mockPicker);
2677+
updateBalancingStateSafely(helper, IDLE, mockPicker);
26842678
assertEquals(CONNECTING, channel.getState(false));
2679+
2680+
updateBalancingStateSafely(helper2, IDLE, mockPicker);
2681+
assertEquals(IDLE, channel.getState(false));
26852682
}
26862683

26872684
@Test
@@ -2695,7 +2692,7 @@ public void idleMode_resetsDelayedTransportPicker() {
26952692
.setServers(Collections.singletonList(new EquivalentAddressGroup(socketAddress)))
26962693
.build());
26972694
createChannel();
2698-
assertEquals(IDLE, channel.getState(false));
2695+
assertEquals(CONNECTING, channel.getState(false));
26992696

27002697
// This call will be buffered in delayedTransport
27012698
ClientCall<String, Integer> call = channel.newCall(method, CallOptions.DEFAULT);
@@ -2790,7 +2787,7 @@ public void enterIdle_exitsIdleIfDelayedStreamPending() {
27902787
// enterIdle() will shut down the name resolver and lb policy used to get a pick for the delayed
27912788
// call
27922789
channel.enterIdle();
2793-
assertEquals(IDLE, channel.getState(false));
2790+
assertEquals(CONNECTING, channel.getState(false));
27942791

27952792
// enterIdle() will restart the delayed call by exiting idle. This creates a new helper.
27962793
ArgumentCaptor<Helper> helperCaptor = ArgumentCaptor.forClass(Helper.class);
@@ -2912,14 +2909,14 @@ public void updateBalancingStateWithShutdownShouldBeIgnored() {
29122909
channelBuilder.nameResolverFactory(
29132910
new FakeNameResolverFactory.Builder(expectedUri).setResolvedAtStart(false).build());
29142911
createChannel();
2915-
assertEquals(IDLE, channel.getState(false));
2912+
assertEquals(CONNECTING, channel.getState(false));
29162913

29172914
Runnable onStateChanged = mock(Runnable.class);
2918-
channel.notifyWhenStateChanged(IDLE, onStateChanged);
2915+
channel.notifyWhenStateChanged(CONNECTING, onStateChanged);
29192916

29202917
updateBalancingStateSafely(helper, SHUTDOWN, mockPicker);
29212918

2922-
assertEquals(IDLE, channel.getState(false));
2919+
assertEquals(CONNECTING, channel.getState(false));
29232920
executor.runDueTasks();
29242921
verify(onStateChanged, never()).run();
29252922
}
@@ -3222,8 +3219,6 @@ public void channelsAndSubchannels_instrumented_state() throws Exception {
32223219
verify(mockLoadBalancerProvider).newLoadBalancer(helperCaptor.capture());
32233220
helper = helperCaptor.getValue();
32243221

3225-
assertEquals(IDLE, getStats(channel).state);
3226-
updateBalancingStateSafely(helper, CONNECTING, mockPicker);
32273222
assertEquals(CONNECTING, getStats(channel).state);
32283223

32293224
AbstractSubchannel subchannel =
@@ -3434,7 +3429,7 @@ public void channelsAndSubchannels_oob_instrumented_state() throws Exception {
34343429
assertEquals(READY, getStats(oobChannel).state);
34353430

34363431
// oobchannel state is separate from the ManagedChannel
3437-
assertEquals(IDLE, getStats(channel).state);
3432+
assertEquals(CONNECTING, getStats(channel).state);
34383433
channel.shutdownNow();
34393434
assertEquals(SHUTDOWN, getStats(channel).state);
34403435
assertEquals(SHUTDOWN, getStats(oobChannel).state);
@@ -4536,4 +4531,4 @@ private static ManagedChannelServiceConfig createManagedChannelServiceConfig(
45364531
return ManagedChannelServiceConfig
45374532
.fromServiceConfig(rawServiceConfig, true, 3, 4, policySelection);
45384533
}
4539-
}
4534+
}

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

+2-2
Original file line numberDiff line numberDiff line change
@@ -277,7 +277,7 @@ public void emptyAddresses_validConfig_2ndResolution_lbNeedsAddress() throws Exc
277277
assertThat(resolvedAddresses.getLoadBalancingPolicyConfig()).isEqualTo("12");
278278
verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class));
279279

280-
assertThat(channel.getState(true)).isEqualTo(ConnectivityState.IDLE);
280+
assertThat(channel.getState(true)).isEqualTo(ConnectivityState.CONNECTING);
281281

282282
reset(mockLoadBalancer);
283283
nameResolverFactory.servers.clear();
@@ -480,7 +480,7 @@ public void invalidConfig_2ndResolution() throws Exception {
480480
assertThat(newResolvedAddress.getLoadBalancingPolicyConfig()).isEqualTo("1st raw config");
481481
assertThat(channel.getConfigSelector()).isSameInstanceAs(configSelector);
482482
verify(mockLoadBalancer, never()).handleNameResolutionError(any(Status.class));
483-
assertThat(channel.getState(false)).isEqualTo(ConnectivityState.IDLE);
483+
assertThat(channel.getState(false)).isEqualTo(ConnectivityState.CONNECTING);
484484
}
485485

486486
@Test

0 commit comments

Comments
 (0)
Please sign in to comment.