Eliminate NPE after recovering from a temporary name resolution failure. #11298
Conversation
| @@ -180,10 +180,18 @@ public Status acceptResolvedAddresses(ResolvedAddresses resolvedAddresses) { | |||
|
|
|||
| @Override | |||
| public void handleNameResolutionError(Status error) { | |||
| if (rawConnectivityState == SHUTDOWN) { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| @@ -196,7 +204,7 @@ void processSubchannelState(Subchannel subchannel, ConnectivityStateInfo stateIn | |||
| if (subchannelData == null || subchannelData.getSubchannel() != subchannel) { | |||
| return; | |||
| } | |||
| if (newState == SHUTDOWN) { | |||
| if (newState == SHUTDOWN || rawConnectivityState == SHUTDOWN) { | |||
There was a problem hiding this comment.
if rawConnectivityState == SHUTDOWN then subchannelData == null. Thus is this change is guaranteed to do nothing. It seems fair to rely on subchannelData == null.
| if (addressIndex != null) { | ||
| addressIndex.updateGroups(null); | ||
| } | ||
| rawConnectivityState = IDLE; |
There was a problem hiding this comment.
Am I right in believing this line is the main fix?
Can things work if we use TF here instead? That might be less prone to error.
There was a problem hiding this comment.
Yes, lines 191-193 are the critical part and I think having an appropriate rawConnectivityState state may be helpful and is definitely a good idea. Changed it to be TF instead of IDLE.
…re. (grpc#11298) * Eliminate NPE after recovering from a temporary name resolution failure. * Add test case for 2 failing subchannels to make sure it causes channel to go into TF.
Fixes #11227