diff --git a/core/src/main/java/io/grpc/internal/InUseStateAggregator.java b/core/src/main/java/io/grpc/internal/InUseStateAggregator.java index 3cd2104f398..45e3b26de3c 100644 --- a/core/src/main/java/io/grpc/internal/InUseStateAggregator.java +++ b/core/src/main/java/io/grpc/internal/InUseStateAggregator.java @@ -33,6 +33,7 @@ import java.util.HashSet; +import javax.annotation.CheckReturnValue; import javax.annotation.concurrent.GuardedBy; /** @@ -44,15 +45,18 @@ abstract class InUseStateAggregator { private final HashSet inUseObjects = new HashSet(); /** - * Update the in-use state of an object. Initially no object is in use. + * Update the in-use state of an object. Initially no object is in use. When the return value is + * non-{@code null}, the caller should execute the runnable after releasing locks. */ - final void updateObjectInUse(T object, boolean inUse) { + @CheckReturnValue + final Runnable updateObjectInUse(T object, boolean inUse) { + Runnable runnable = null; synchronized (getLock()) { int origSize = inUseObjects.size(); if (inUse) { inUseObjects.add(object); if (origSize == 0) { - handleInUse(); + runnable = handleInUse(); } } else { boolean removed = inUseObjects.remove(object); @@ -61,8 +65,10 @@ final void updateObjectInUse(T object, boolean inUse) { } } } + return runnable; } + @CheckReturnValue final boolean isInUse() { synchronized (getLock()) { return !inUseObjects.isEmpty(); @@ -73,12 +79,13 @@ final boolean isInUse() { /** * Called when the aggregated in-use state has changed to true, which means at least one object is - * in use. + * in use. When the return value is non-{@code null}, then the runnable will be executed by the + * caller of {@link #updateObjectInUse} after releasing locks. * *

This method is called under the lock returned by {@link #getLock}. */ @GuardedBy("getLock()") - abstract void handleInUse(); + abstract Runnable handleInUse(); /** * Called when the aggregated in-use state has changed to false, which means no object is in use. diff --git a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java index de4a14dcfaf..2f1f5dbeea0 100644 --- a/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java +++ b/core/src/main/java/io/grpc/internal/ManagedChannelImpl.java @@ -165,8 +165,8 @@ Object getLock() { @Override @GuardedBy("lock") - void handleInUse() { - exitIdleMode(); + Runnable handleInUse() { + return exitIdleMode(); } @GuardedBy("lock") @@ -220,32 +220,46 @@ public void run() { /** * Make the channel exit idle mode, if it's in it. Return a LoadBalancer that can be used for * making new requests. Return null if the channel is shutdown. - * - *

May be called under the lock. */ @VisibleForTesting - LoadBalancer exitIdleMode() { + LoadBalancer exitIdleModeAndGetLb() { + Runnable runnable; + LoadBalancer balancer; + synchronized (lock) { + runnable = exitIdleMode(); + balancer = loadBalancer; + } + if (runnable != null) { + runnable.run(); + } + return balancer; + } + + /** + * Make the channel exit idle mode, if it's in it. If the returned runnable is non-{@code null}, + * then it should be executed by the caller after releasing {@code lock}. + */ + @GuardedBy("lock") + private Runnable exitIdleMode() { final LoadBalancer balancer; final NameResolver resolver; - synchronized (lock) { - if (shutdown) { - return null; - } - if (inUseStateAggregator.isInUse()) { - cancelIdleTimer(); - } else { - // exitIdleMode() may be called outside of inUseStateAggregator, which may still in - // "not-in-use" state. If it's the case, we start the timer which will be soon cancelled if - // the aggregator receives actual uses. - rescheduleIdleTimer(); - } - if (loadBalancer != null) { - return loadBalancer; - } - balancer = loadBalancerFactory.newLoadBalancer(nameResolver.getServiceAuthority(), tm); - this.loadBalancer = balancer; - resolver = this.nameResolver; + if (shutdown) { + return null; } + if (inUseStateAggregator.isInUse()) { + cancelIdleTimer(); + } else { + // exitIdleMode() may be called outside of inUseStateAggregator, which may still in + // "not-in-use" state. If it's the case, we start the timer which will be soon cancelled if + // the aggregator receives actual uses. + rescheduleIdleTimer(); + } + if (loadBalancer != null) { + return null; + } + balancer = loadBalancerFactory.newLoadBalancer(nameResolver.getServiceAuthority(), tm); + this.loadBalancer = balancer; + resolver = this.nameResolver; class NameResolverStartTask implements Runnable { @Override public void run() { @@ -255,8 +269,7 @@ public void run() { } } - scheduledExecutor.execute(new NameResolverStartTask()); - return balancer; + return new NameResolverStartTask(); } @GuardedBy("lock") @@ -294,7 +307,7 @@ private void rescheduleIdleTimer() { private final ClientTransportProvider transportProvider = new ClientTransportProvider() { @Override public ClientTransport get(CallOptions callOptions) { - LoadBalancer balancer = exitIdleMode(); + LoadBalancer balancer = exitIdleModeAndGetLb(); if (balancer == null) { return SHUTDOWN_TRANSPORT; } @@ -618,13 +631,14 @@ public void onConnectionClosedByServer(Status status) { } @Override - public void onInUse(TransportSet ts) { - inUseStateAggregator.updateObjectInUse(ts, true); + public Runnable onInUse(TransportSet ts) { + return inUseStateAggregator.updateObjectInUse(ts, true); } @Override public void onNotInUse(TransportSet ts) { - inUseStateAggregator.updateObjectInUse(ts, false); + Runnable r = inUseStateAggregator.updateObjectInUse(ts, false); + assert r == null; } }); if (log.isLoggable(Level.FINE)) { @@ -709,13 +723,17 @@ private class InterimTransportImpl implements InterimTransport delayedTransports.remove(delayedTransport); maybeTerminateChannel(); } - inUseStateAggregator.updateObjectInUse(delayedTransport, false); + Runnable r = inUseStateAggregator.updateObjectInUse(delayedTransport, false); + assert r == null; } @Override public void transportReady() {} @Override public void transportInUse(boolean inUse) { - inUseStateAggregator.updateObjectInUse(delayedTransport, inUse); + Runnable r = inUseStateAggregator.updateObjectInUse(delayedTransport, inUse); + if (r != null) { + r.run(); + } } }); boolean savedShutdown; diff --git a/core/src/main/java/io/grpc/internal/TransportSet.java b/core/src/main/java/io/grpc/internal/TransportSet.java index 3941a88d6c5..b216d7fd170 100644 --- a/core/src/main/java/io/grpc/internal/TransportSet.java +++ b/core/src/main/java/io/grpc/internal/TransportSet.java @@ -110,8 +110,8 @@ Object getLock() { } @Override - void handleInUse() { - callback.onInUse(TransportSet.this); + Runnable handleInUse() { + return callback.onInUse(TransportSet.this); } @Override @@ -370,7 +370,10 @@ public void transportReady() {} @Override public void transportInUse(boolean inUse) { - inUseStateAggregator.updateObjectInUse(transport, inUse); + Runnable r = inUseStateAggregator.updateObjectInUse(transport, inUse); + if (r != null) { + r.run(); + } } @Override @@ -379,7 +382,8 @@ public void transportShutdown(Status status) {} @Override public void transportTerminated() { boolean runCallback = false; - inUseStateAggregator.updateObjectInUse(transport, false); + Runnable r = inUseStateAggregator.updateObjectInUse(transport, false); + assert r == null; synchronized (lock) { transports.remove(transport); if (shutdown && transports.isEmpty()) { @@ -519,9 +523,13 @@ public void onConnectionClosedByServer(Status status) { } /** * Called when the TransportSet's in-use state has changed to true, which means at least one - * transport is in use. This method is called under a lock thus externally synchronized. + * transport is in use. This method is called under a lock thus externally synchronized. If the + * return value is non-{@code null}, the runnable will be executed after releasing the lock. */ - public void onInUse(TransportSet ts) { } + @CheckReturnValue + public Runnable onInUse(TransportSet ts) { + return null; + } /** * Called when the TransportSet's in-use state has changed to false, which means no transport is diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java index 166095819fd..83901969d6d 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java @@ -338,9 +338,7 @@ private void walkIntoIdleMode(Collection currentTranspo } private void forceExitIdleMode() { - channel.exitIdleMode(); - // NameResolver is started in the scheduled executor - timer.runDueTasks(); + channel.exitIdleModeAndGetLb(); } private ClientTransport channelTmGetTransportUnwrapped(EquivalentAddressGroup addressGroup) { diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java index 1a5e0ff0e07..3c07ab21769 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTest.java @@ -151,9 +151,7 @@ private void createChannel( ManagedChannelImpl.IDLE_TIMEOUT_MILLIS_DISABLE, executor.scheduledExecutorService, userAgent, interceptors); // Force-exit the initial idle-mode - channel.exitIdleMode(); - // Will start NameResolver in the scheduled executor - assertEquals(1, timer.runDueTasks()); + channel.exitIdleModeAndGetLb(); } @Before diff --git a/core/src/test/java/io/grpc/internal/ManagedChannelImplTransportManagerTest.java b/core/src/test/java/io/grpc/internal/ManagedChannelImplTransportManagerTest.java index 9a97e95a14d..6dfaad91ceb 100644 --- a/core/src/test/java/io/grpc/internal/ManagedChannelImplTransportManagerTest.java +++ b/core/src/test/java/io/grpc/internal/ManagedChannelImplTransportManagerTest.java @@ -140,7 +140,7 @@ public void setUp() { ArgumentCaptor> tmCaptor = ArgumentCaptor.forClass(null); // Force Channel to exit the initial idleness to get NameResolver and LoadBalancer created. - channel.exitIdleMode(); + channel.exitIdleModeAndGetLb(); verify(mockNameResolverFactory).newNameResolver(any(URI.class), any(Attributes.class)); verify(mockLoadBalancerFactory).newLoadBalancer(anyString(), tmCaptor.capture()); tm = tmCaptor.getValue();