Skip to content

Commit fb298a0

Browse files
authored
fix(pick_first): ignore state updates from shut down or replaced subchannels (grpc#12968)
In legacy PickFirstLoadBalancer, when handleNameResolutionError() or shutdown() is called, subchannel.shutdown() is invoked. Because ManagedChannelImpl delays subchannel shutdown by 5 seconds (SUBCHANNEL_SHUTDOWN_DELAY_SECONDS), the old subchannel may complete a connection attempt during this window and fire READY state updates. Without a check verifying if the calling subchannel is still the current active subchannel, PickFirstLoadBalancer publishes a READY picker for the obsolete subchannel right before its 5-second delayed shutdown task fires and kills the transport. This leaves the channel permanently stuck in READY with a dead subchannel picker. Partially addresses grpc#12958
1 parent dc26e02 commit fb298a0

2 files changed

Lines changed: 27 additions & 0 deletions

File tree

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,9 @@ public void handleNameResolutionError(Status error) {
107107
}
108108

109109
private void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) {
110+
if (subchannel != this.subchannel) {
111+
return;
112+
}
110113
ConnectivityState newState = stateInfo.getState();
111114
if (newState == SHUTDOWN) {
112115
return;
@@ -161,6 +164,7 @@ private void updateBalancingState(ConnectivityState state, SubchannelPicker pick
161164
public void shutdown() {
162165
if (subchannel != null) {
163166
subchannel.shutdown();
167+
subchannel = null;
164168
}
165169
}
166170

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -591,6 +591,29 @@ public void requestConnection() {
591591
verify(mockSubchannel, times(2)).requestConnection();
592592
}
593593

594+
@Test
595+
public void ignoreStaleSubchannelStateChange() throws Exception {
596+
loadBalancer.acceptResolvedAddresses(
597+
ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build());
598+
verify(mockSubchannel).start(stateListenerCaptor.capture());
599+
SubchannelStateListener oldListener = stateListenerCaptor.getValue();
600+
601+
// Name resolution error occurs, shutting down old subchannel and resetting subchannel reference
602+
loadBalancer.handleNameResolutionError(Status.UNAVAILABLE);
603+
604+
// New resolution result arrives, creating a new subchannel
605+
Subchannel newSubchannel = org.mockito.Mockito.mock(Subchannel.class);
606+
when(mockHelper.createSubchannel(any())).thenReturn(newSubchannel);
607+
loadBalancer.acceptResolvedAddresses(
608+
ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build());
609+
610+
// Old subchannel (e.g. during delayed shutdown) fires READY state update
611+
oldListener.onSubchannelState(ConnectivityStateInfo.forNonError(READY));
612+
613+
// Verify that the LB state was NOT updated to READY with the old subchannel
614+
verify(mockHelper, never()).updateBalancingState(eq(READY), any(SubchannelPicker.class));
615+
}
616+
594617
private static class FakeSocketAddress extends SocketAddress {
595618
final String name;
596619

0 commit comments

Comments
 (0)