Skip to content

Commit df4ac59

Browse files
temawitemawi
authored
core: Exit idle mode in enterIdle() if there are pending calls or delayed transport.
This change assures that if there are only calls in real transport the channel will remain in idle mode. Idle mode will be exited if there are calls in delayed transport to allow them to be processed.
1 parent f1b699b commit df4ac59

4 files changed

Lines changed: 157 additions & 1 deletion

File tree

‎core/src/main/java/io/grpc/internal/InUseStateAggregator.java‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,21 @@ public final boolean isInUse() {
5353
return !inUseObjects.isEmpty();
5454
}
5555

56+
/**
57+
* Returns {@code true} if any of the given objects are in use.
58+
*
59+
* @param objects The objects to consider.
60+
* @return {@code true} if any of the given objects are in use.
61+
*/
62+
public final boolean anyObjectInUse(Object... objects) {
63+
for (Object object : objects) {
64+
if (inUseObjects.contains(object)) {
65+
return true;
66+
}
67+
}
68+
return false;
69+
}
70+
5671
/**
5772
* Called when the aggregated in-use state has changed to true, which means at least one object is
5873
* in use.

‎core/src/main/java/io/grpc/internal/ManagedChannelImpl.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -423,7 +423,10 @@ private void enterIdleMode() {
423423
delayedTransport.reprocess(null);
424424
channelLogger.log(ChannelLogLevel.INFO, "Entering IDLE state");
425425
channelStateManager.gotoState(IDLE);
426-
if (inUseStateAggregator.isInUse()) {
426+
// If the inUseStateAggregator still considers pending calls to be queued up or the delayed
427+
// transport to be holding some we need to exit idle mode to give these calls a chance to
428+
// be processed.
429+
if (inUseStateAggregator.anyObjectInUse(pendingCallsInUseObject, delayedTransport)) {
427430
exitIdleMode();
428431
}
429432
}

‎core/src/test/java/io/grpc/internal/InUseStateAggregatorTest.java‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,63 @@
1+
/*
2+
* Copyright 2021 The gRPC Authors
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* http://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package io.grpc.internal;
18+
19+
import static org.junit.Assert.assertTrue;
20+
21+
import org.junit.Before;
22+
import org.junit.Test;
23+
import org.junit.runner.RunWith;
24+
import org.junit.runners.JUnit4;
25+
26+
/**
27+
* Unit tests for {@link InUseStateAggregator}.
28+
*/
29+
@RunWith(JUnit4.class)
30+
public class InUseStateAggregatorTest {
31+
32+
private InUseStateAggregator<String> aggregator;
33+
34+
@Before
35+
public void setUp() {
36+
aggregator = new InUseStateAggregator<String>() {
37+
@Override
38+
protected void handleInUse() {
39+
}
40+
41+
@Override
42+
protected void handleNotInUse() {
43+
}
44+
};
45+
}
46+
47+
@Test
48+
public void anyObjectInUse() {
49+
String objectOne = "1";
50+
String objectTwo = "2";
51+
String objectThree = "3";
52+
53+
aggregator.updateObjectInUse(objectOne, true);
54+
assertTrue(aggregator.anyObjectInUse(objectOne));
55+
56+
aggregator.updateObjectInUse(objectTwo, true);
57+
aggregator.updateObjectInUse(objectThree, true);
58+
assertTrue(aggregator.anyObjectInUse(objectOne, objectTwo, objectThree));
59+
60+
aggregator.updateObjectInUse(objectTwo, false);
61+
assertTrue(aggregator.anyObjectInUse(objectOne, objectThree));
62+
}
63+
}

‎core/src/test/java/io/grpc/internal/ManagedChannelImplIdlenessTest.java‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,9 @@
2626
import static org.mockito.AdditionalAnswers.delegatesTo;
2727
import static org.mockito.ArgumentMatchers.any;
2828
import static org.mockito.ArgumentMatchers.eq;
29+
import static org.mockito.ArgumentMatchers.isA;
2930
import static org.mockito.ArgumentMatchers.same;
31+
import static org.mockito.Mockito.atMostOnce;
3032
import static org.mockito.Mockito.mock;
3133
import static org.mockito.Mockito.never;
3234
import static org.mockito.Mockito.times;
@@ -284,6 +286,35 @@ public void delayedTransportHoldsOffIdleness() throws Exception {
284286
verify(mockLoadBalancer).shutdown();
285287
}
286288

289+
@Test
290+
public void pendingCallExitsIdleAfterEnter() throws Exception {
291+
// Create a pending call without starting it.
292+
channel.newCall(method, CallOptions.DEFAULT);
293+
294+
channel.enterIdle();
295+
296+
// Just the existence of a non-started, pending call means the channel cannot stay
297+
// in idle mode because the expectation is that the pending call will also need to
298+
// be handled.
299+
verify(mockNameResolver, times(2)).start(any(NameResolver.Listener2.class));
300+
}
301+
302+
@Test
303+
public void delayedTransportExitsIdleAfterEnter() throws Exception {
304+
// Start a new call that will go to the delayed transport
305+
ClientCall<String, Integer> call = channel.newCall(method, CallOptions.DEFAULT);
306+
call.start(mockCallListener, new Metadata());
307+
deliverResolutionResult();
308+
309+
channel.enterIdle();
310+
311+
// Since we have a call in delayed transport, the call to enterIdle() should have resulted in
312+
// the channel going to idle mode and then immediately exiting. We confirm this by verifying
313+
// that the name resolver was started up twice - once when the call was first created and a
314+
// second time after exiting idle mode.
315+
verify(mockNameResolver, times(2)).start(any(NameResolver.Listener2.class));
316+
}
317+
287318
@Test
288319
public void realTransportsHoldsOffIdleness() throws Exception {
289320
final EquivalentAddressGroup addressGroup = servers.get(1);
@@ -332,6 +363,50 @@ public void realTransportsHoldsOffIdleness() throws Exception {
332363
verify(mockLoadBalancer).shutdown();
333364
}
334365

366+
@Test
367+
public void enterIdleWhileRealTransportInProgress() {
368+
final EquivalentAddressGroup addressGroup = servers.get(1);
369+
370+
// Start a call, which goes to delayed transport
371+
ClientCall<String, Integer> call = channel.newCall(method, CallOptions.DEFAULT);
372+
call.start(mockCallListener, new Metadata());
373+
374+
// Verify that we have exited the idle mode
375+
ArgumentCaptor<Helper> helperCaptor = ArgumentCaptor.forClass(null);
376+
verify(mockLoadBalancerProvider).newLoadBalancer(helperCaptor.capture());
377+
deliverResolutionResult();
378+
Helper helper = helperCaptor.getValue();
379+
380+
// Create a subchannel for the real transport to happen on.
381+
Subchannel subchannel = createSubchannelSafely(helper, addressGroup, Attributes.EMPTY);
382+
requestConnectionSafely(helper, subchannel);
383+
MockClientTransportInfo t0 = newTransports.poll();
384+
t0.listener.transportReady();
385+
386+
SubchannelPicker mockPicker = mock(SubchannelPicker.class);
387+
when(mockPicker.pickSubchannel(any(PickSubchannelArgs.class)))
388+
.thenReturn(PickResult.withSubchannel(subchannel));
389+
updateBalancingStateSafely(helper, READY, mockPicker);
390+
391+
// Delayed transport creates real streams in the app executor
392+
executor.runDueTasks();
393+
394+
// Move transport to the in-use state
395+
t0.listener.transportInUse(true);
396+
397+
// Now we enter Idle mode while real transport is happening
398+
channel.enterIdle();
399+
400+
// Verify that the name resolver and the load balance were shut down.
401+
verify(mockNameResolver).shutdown();
402+
verify(mockLoadBalancer).shutdown();
403+
404+
// When there are no pending streams, the call to enterIdle() should stick and
405+
// we remain in idle mode. We verify this by making sure that the name resolver
406+
// was not started up more than once (the initial startup).
407+
verify(mockNameResolver, atMostOnce()).start(isA(NameResolver.Listener2.class));
408+
}
409+
335410
@Test
336411
public void updateSubchannelAddresses_newAddressConnects() {
337412
ClientCall<String, Integer> call = channel.newCall(method, CallOptions.DEFAULT);

0 commit comments

Comments
 (0)