Eliminate NPE after recovering from a temporary name resolution failure.#11298
Eliminate NPE after recovering from a temporary name resolution failure.#11298larry-safran merged 3 commits intogrpc:masterfrom
Conversation
|
|
||
| @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.
ejona86
left a comment
There was a problem hiding this comment.
FWIW, at some point we should make PF continue using the old addresses in the face of resolution errors.
| 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