Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Eliminate NPE after recovering from a temporary name resolution failure. #11298

Merged
merged 3 commits into from
Jun 21, 2024
Merged

Eliminate NPE after recovering from a temporary name resolution failure. #11298

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
401865a
Eliminate NPE after recovering from a temporary name resolution failure.
larry-safran Jun 18, 2024
8ea7ca9
Add test case for 2 failing subchannels to make sure it causes channe…
larry-safran Jun 20, 2024
c8e2bc5
Respond to reviewer comments
larry-safran Jun 20, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion core/src/main/java/io/grpc/internal/PickFirstLeafLoadBalancer.java
View file
Original file line number Diff line number Diff line change
@@ -180,22 +180,32 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) {

@Override
public void handleNameResolutionError(Status error) {
if (rawConnectivityState == SHUTDOWN) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was this introduced to avoid NPE and handle the case of acceptResolvedAddresses being called when the state is SHUTDOWN ? Can we add test for this as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eric pointed out that it shouldn't be possible to get into that state, so I don't think we can add a test. I was being overly cautious in case someone messed up the logic in the future.

return;
}

for (SubchannelData subchannelData : subchannels.values()) {
subchannelData.getSubchannel().shutdown();
}
subchannels.clear();
if (addressIndex != null) {
addressIndex.updateGroups(null);
}
rawConnectivityState = TRANSIENT_FAILURE;
updateBalancingState(TRANSIENT_FAILURE, new Picker(PickResult.withError(error)));
}

void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateInfo) {
ConnectivityState newState = stateInfo.getState();

SubchannelData subchannelData = subchannels.get(getAddress(subchannel));
// Shutdown channels/previously relevant subchannels can still callback with state updates.
// To prevent pickers from returning these obsolete subchannels, this logic
// is included to check if the current list of active subchannels includes this subchannel.
SubchannelData subchannelData = subchannels.get(getAddress(subchannel));
if (subchannelData == null || subchannelData.getSubchannel() != subchannel) {
return;
}

if (newState == SHUTDOWN) {
return;
}
58 changes: 58 additions & 0 deletions core/src/test/java/io/grpc/internal/PickFirstLeafLoadBalancerTest.java
View file
Original file line number Diff line number Diff line change
@@ -657,6 +657,37 @@ public void nameResolutionSuccessAfterError() {
assertEquals(mockSubchannel1, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());
}

@Test
public void nameResolutionTemporaryError() {
List<EquivalentAddressGroup> newServers = Lists.newArrayList(servers.get(0));
InOrder inOrder = inOrder(mockHelper, mockSubchannel1);
loadBalancer.acceptResolvedAddresses(
ResolvedAddresses.newBuilder().setAddresses(newServers).setAttributes(affinity).build());
inOrder.verify(mockSubchannel1).start(stateListenerCaptor.capture());
SubchannelStateListener stateListener1 = stateListenerCaptor.getValue();
stateListener1.onSubchannelState(ConnectivityStateInfo.forNonError(READY));
inOrder.verify(mockHelper).updateBalancingState(eq(READY), pickerCaptor.capture());
assertEquals(mockSubchannel1, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());

loadBalancer.handleNameResolutionError(
Status.UNAVAILABLE.withDescription("nameResolutionError"));
inOrder.verify(mockHelper).updateBalancingState(
eq(TRANSIENT_FAILURE), any(SubchannelPicker.class));

loadBalancer.acceptResolvedAddresses(
ResolvedAddresses.newBuilder().setAddresses(servers).setAttributes(affinity).build());
inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
inOrder.verify(mockSubchannel1).start(stateListenerCaptor.capture());
SubchannelStateListener stateListener2 = stateListenerCaptor.getValue();

assertNull(pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());

stateListener2.onSubchannelState(ConnectivityStateInfo.forNonError(READY));
inOrder.verify(mockHelper).updateBalancingState(eq(READY), pickerCaptor.capture());
assertEquals(mockSubchannel1, pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());
}


@Test
public void nameResolutionErrorWithStateChanges() {
List<EquivalentAddressGroup> newServers = Lists.newArrayList(servers.get(0));
@@ -725,6 +756,33 @@ public void requestConnection() {
}
}

@Test
public void failChannelWhenSubchannelsFail() {
List<EquivalentAddressGroup> newServers = Lists.newArrayList(servers.get(0), servers.get(1));
when(mockSubchannel1.getAllAddresses()).thenReturn(Lists.newArrayList(servers.get(0)));
when(mockSubchannel2.getAllAddresses()).thenReturn(Lists.newArrayList(servers.get(1)));

// accept resolved addresses
loadBalancer.acceptResolvedAddresses(
ResolvedAddresses.newBuilder().setAddresses(newServers).setAttributes(affinity).build());
InOrder inOrder = inOrder(mockHelper, mockSubchannel1, mockSubchannel2);
verify(mockHelper).updateBalancingState(eq(CONNECTING), pickerCaptor.capture());
verify(mockHelper).createSubchannel(createArgsCaptor.capture());
inOrder.verify(mockSubchannel1).start(stateListenerCaptor.capture());
SubchannelStateListener stateListener = stateListenerCaptor.getValue();
assertNull(pickerCaptor.getValue().pickSubchannel(mockArgs).getSubchannel());

inOrder.verify(mockSubchannel1).requestConnection();
stateListener.onSubchannelState(ConnectivityStateInfo.forTransientFailure(CONNECTION_ERROR));

inOrder.verify(mockSubchannel2).start(stateListenerCaptor.capture());
SubchannelStateListener stateListener2 = stateListenerCaptor.getValue();
stateListener2.onSubchannelState(ConnectivityStateInfo.forTransientFailure(CONNECTION_ERROR));

inOrder.verify(mockHelper).updateBalancingState(eq(TRANSIENT_FAILURE), pickerCaptor.capture());
assertEquals(CONNECTION_ERROR, pickerCaptor.getValue().pickSubchannel(mockArgs).getStatus());
}

@Test
public void updateAddresses_emptyEagList_returns_false() {
loadBalancer.acceptResolvedAddresses(