diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index 94a485c815..d3c46445e2 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -308,16 +308,20 @@ final class ManagedChannelImpl extends ManagedChannel implements } // Must be called from syncContext - private void shutdownNameResolverAndLoadBalancer(boolean verifyActive) { - if (verifyActive) { - checkState(nameResolver != null, "nameResolver is null"); + private void shutdownNameResolverAndLoadBalancer(boolean channelIsActive) { + if (channelIsActive) { + checkState(nameResolverStarted, "nameResolver is not started"); checkState(lbHelper != null, "lbHelper is null"); } if (nameResolver != null) { cancelNameResolverBackoff(); nameResolver.shutdown(); - nameResolver = null; nameResolverStarted = false; + if (channelIsActive) { + nameResolver = getNameResolver(target, nameResolverFactory, nameResolverParams); + } else { + nameResolver = null; + } } if (lbHelper != null) { lbHelper.lb.shutdown(); @@ -372,7 +376,6 @@ final class ManagedChannelImpl extends ManagedChannel implements // which are bugs. shutdownNameResolverAndLoadBalancer(true); delayedTransport.reprocess(null); - nameResolver = getNameResolver(target, nameResolverFactory, nameResolverParams); channelLogger.log(ChannelLogLevel.INFO, "Entering IDLE state"); channelStateManager.gotoState(IDLE); if (inUseStateAggregator.isInUse()) { @@ -399,9 +402,7 @@ final class ManagedChannelImpl extends ManagedChannel implements @Override public void run() { scheduledNameResolverRefresh = null; - if (nameResolver != null) { - nameResolver.refresh(); - } + refreshNameResolution(); } } @@ -421,11 +422,19 @@ final class ManagedChannelImpl extends ManagedChannel implements } } - // Must be run from syncContext - private void refreshNameResolutionNow() { + /** + * Force name resolution refresh to happen immediately and reset refresh back-off. Must be run + * from syncContext. + */ + private void refreshAndResetNameResolution() { syncContext.throwIfNotInThisSynchronizationContext(); cancelNameResolverBackoff(); - if (nameResolver != null) { + refreshNameResolution(); + } + + private void refreshNameResolution() { + syncContext.throwIfNotInThisSynchronizationContext(); + if (nameResolverStarted) { nameResolver.refresh(); } } @@ -871,7 +880,7 @@ final class ManagedChannelImpl extends ManagedChannel implements } if (scheduledNameResolverRefresh != null && scheduledNameResolverRefresh.isPending()) { checkState(nameResolverStarted, "name resolver must be started"); - refreshNameResolutionNow(); + refreshAndResetNameResolution(); } for (InternalSubchannel subchannel : subchannels) { subchannel.resetConnectBackoff(); @@ -990,7 +999,7 @@ final class ManagedChannelImpl extends ManagedChannel implements // Must be called from syncContext private void handleInternalSubchannelState(ConnectivityStateInfo newState) { if (newState.getState() == TRANSIENT_FAILURE || newState.getState() == IDLE) { - refreshNameResolutionNow(); + refreshAndResetNameResolution(); } } @@ -1122,7 +1131,7 @@ final class ManagedChannelImpl extends ManagedChannel implements final class LoadBalancerRefreshNameResolution implements Runnable { @Override public void run() { - refreshNameResolutionNow(); + refreshAndResetNameResolution(); } } diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 2f6dcad507..61e44dda36 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -1612,24 +1612,33 @@ public class ManagedChannelImplTest { } @Test - public void refreshNameResolutionWhenSubchannelConnectionFailed() { - subtestRefreshNameResolutionWhenConnectionFailed(false); + public void refreshNameResolution_whenSubchannelConnectionFailed_notIdle() { + subtestNameResolutionRefreshWhenConnectionFailed(false, false); } @Test - public void refreshNameResolutionWhenOobChannelConnectionFailed() { - subtestRefreshNameResolutionWhenConnectionFailed(true); + public void refreshNameResolution_whenOobChannelConnectionFailed_notIdle() { + subtestNameResolutionRefreshWhenConnectionFailed(true, false); } - private void subtestRefreshNameResolutionWhenConnectionFailed(boolean isOobChannel) { + @Test + public void notRefreshNameResolution_whenSubchannelConnectionFailed_idle() { + subtestNameResolutionRefreshWhenConnectionFailed(false, true); + } + + @Test + public void notRefreshNameResolution_whenOobChannelConnectionFailed_idle() { + subtestNameResolutionRefreshWhenConnectionFailed(true, true); + } + + private void subtestNameResolutionRefreshWhenConnectionFailed( + boolean isOobChannel, boolean isIdle) { FakeNameResolverFactory nameResolverFactory = new FakeNameResolverFactory.Builder(expectedUri) .setServers(Collections.singletonList(new EquivalentAddressGroup(socketAddress))) .build(); channelBuilder.nameResolverFactory(nameResolverFactory); createChannel(); - FakeNameResolverFactory.FakeNameResolver resolver = nameResolverFactory.resolvers.get(0); - if (isOobChannel) { OobChannel oobChannel = (OobChannel) helper.createOobChannel(addressGroup, "oobAuthority"); oobChannel.getSubchannel().requestConnection(); @@ -1641,10 +1650,26 @@ public class ManagedChannelImplTest { MockClientTransportInfo transportInfo = transports.poll(); assertNotNull(transportInfo); + FakeNameResolverFactory.FakeNameResolver resolver = nameResolverFactory.resolvers.remove(0); + + if (isIdle) { + channel.enterIdle(); + // Entering idle mode will result in a new resolver + resolver = nameResolverFactory.resolvers.remove(0); + } + + assertEquals(0, nameResolverFactory.resolvers.size()); + + int expectedRefreshCount = 0; + // Transport closed when connecting - assertEquals(0, resolver.refreshCalled); + assertEquals(expectedRefreshCount, resolver.refreshCalled); transportInfo.listener.transportShutdown(Status.UNAVAILABLE); - assertEquals(1, resolver.refreshCalled); + // When channel enters idle, new resolver is created but not started. + if (!isIdle) { + expectedRefreshCount++; + } + assertEquals(expectedRefreshCount, resolver.refreshCalled); timer.forwardNanos(RECONNECT_BACKOFF_INTERVAL_NANOS); transportInfo = transports.poll(); @@ -1653,9 +1678,13 @@ public class ManagedChannelImplTest { transportInfo.listener.transportReady(); // Transport closed when ready - assertEquals(1, resolver.refreshCalled); + assertEquals(expectedRefreshCount, resolver.refreshCalled); transportInfo.listener.transportShutdown(Status.UNAVAILABLE); - assertEquals(2, resolver.refreshCalled); + // When channel enters idle, new resolver is created but not started. + if (!isIdle) { + expectedRefreshCount++; + } + assertEquals(expectedRefreshCount, resolver.refreshCalled); } @Test @@ -3316,7 +3345,6 @@ public class ManagedChannelImplTest { } @Override public void refresh() { - assertNotNull(listener); refreshCalled++; resolved(); }