Skip to content

Commit 51f811d

Browse files
larry-safranlarry-safran
authored
Enable Happy Eyeballs by default (#11022)
* Flip the flag * Fix test flakiness where IPv6 was not considered loopback
1 parent 2c83ef0 commit 51f811d

11 files changed

Lines changed: 179 additions & 75 deletions

File tree

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

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import io.grpc.ExperimentalApi;
3535
import io.grpc.LoadBalancer;
3636
import io.grpc.Status;
37+
import io.grpc.SynchronizationContext;
3738
import io.grpc.SynchronizationContext.ScheduledHandle;
3839
import java.net.SocketAddress;
3940
import java.util.ArrayList;
@@ -72,7 +73,7 @@ final class PickFirstLeafLoadBalancer extends LoadBalancer {
7273
private ConnectivityState rawConnectivityState = IDLE;
7374
private ConnectivityState concludedState = IDLE;
7475
private final boolean enableHappyEyeballs =
75-
GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false);
76+
GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true);
7677

7778
PickFirstLeafLoadBalancer(Helper helper) {
7879
this.helper = checkNotNull(helper, "helper");
@@ -406,7 +407,16 @@ public void run() {
406407
}
407408
}
408409

409-
scheduleConnectionTask = helper.getSynchronizationContext().schedule(
410+
SynchronizationContext synchronizationContext = null;
411+
try {
412+
synchronizationContext = helper.getSynchronizationContext();
413+
} catch (NullPointerException e) {
414+
// All helpers should have a sync context, but if one doesn't (ex. user had a custom test)
415+
// we don't want to break previously working functionality.
416+
return;
417+
}
418+
419+
scheduleConnectionTask = synchronizationContext.schedule(
410420
new StartNextConnection(),
411421
CONNECTION_DELAY_INTERVAL_MS,
412422
TimeUnit.MILLISECONDS,

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import io.grpc.LoadBalancerRegistry;
4949
import io.grpc.NameResolver.ConfigOrError;
5050
import io.grpc.Status;
51+
import io.grpc.SynchronizationContext;
5152
import io.grpc.internal.AutoConfiguredLoadBalancerFactory.AutoConfiguredLoadBalancer;
5253
import io.grpc.internal.PickFirstLeafLoadBalancer.PickFirstLeafLoadBalancerConfig;
5354
import io.grpc.internal.PickFirstLoadBalancer.PickFirstLoadBalancerConfig;
@@ -58,6 +59,7 @@
5859
import java.util.Collections;
5960
import java.util.List;
6061
import java.util.Map;
62+
import java.util.concurrent.ScheduledExecutorService;
6163
import java.util.concurrent.atomic.AtomicBoolean;
6264
import java.util.concurrent.atomic.AtomicInteger;
6365
import java.util.concurrent.atomic.AtomicReference;
@@ -691,6 +693,16 @@ private static class TestLoadBalancer extends ForwardingLoadBalancer {
691693
}
692694

693695
private class TestHelper extends ForwardingLoadBalancerHelper {
696+
final SynchronizationContext syncContext = new SynchronizationContext(
697+
new Thread.UncaughtExceptionHandler() {
698+
@Override
699+
public void uncaughtException(Thread t, Throwable e) {
700+
throw new AssertionError(e);
701+
}
702+
});
703+
704+
final FakeClock fakeClock = new FakeClock();
705+
694706
@Override
695707
protected Helper delegate() {
696708
return null;
@@ -705,6 +717,16 @@ public ChannelLogger getChannelLogger() {
705717
public void updateBalancingState(ConnectivityState newState, SubchannelPicker newPicker) {
706718
// noop
707719
}
720+
721+
@Override
722+
public SynchronizationContext getSynchronizationContext() {
723+
return syncContext;
724+
}
725+
726+
@Override
727+
public ScheduledExecutorService getScheduledExecutorService() {
728+
return fakeClock.getScheduledExecutorService();
729+
}
708730
}
709731

710732
private static class TestSubchannel extends Subchannel {

‎interop-testing/src/test/java/io/grpc/testing/integration/Http2Test.java‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import static org.junit.Assert.assertEquals;
2020
import static org.junit.Assert.assertNotEquals;
21+
import static org.junit.Assert.assertTrue;
2122

2223
import io.grpc.ChannelCredentials;
2324
import io.grpc.ManagedChannelBuilder;
@@ -38,7 +39,6 @@
3839
import io.grpc.stub.MetadataUtils;
3940
import io.grpc.testing.TlsTesting;
4041
import java.io.IOException;
41-
import java.net.InetAddress;
4242
import java.net.InetSocketAddress;
4343
import java.util.Arrays;
4444
import org.junit.Test;
@@ -139,15 +139,15 @@ protected ManagedChannelBuilder<?> createChannelBuilder() {
139139
@Test
140140
public void remoteAddr() {
141141
InetSocketAddress isa = (InetSocketAddress) obtainRemoteClientAddr();
142-
assertEquals(InetAddress.getLoopbackAddress(), isa.getAddress());
142+
assertTrue(isa.getAddress().isLoopbackAddress());
143143
// It should not be the same as the server
144144
assertNotEquals(((InetSocketAddress) getListenAddress()).getPort(), isa.getPort());
145145
}
146146

147147
@Test
148148
public void localAddr() throws Exception {
149149
InetSocketAddress isa = (InetSocketAddress) obtainLocalServerAddr();
150-
assertEquals(InetAddress.getLoopbackAddress(), isa.getAddress());
150+
assertTrue(isa.getAddress().isLoopbackAddress());
151151
assertEquals(((InetSocketAddress) getListenAddress()).getPort(), isa.getPort());
152152
}
153153

‎rls/src/test/java/io/grpc/rls/RlsLoadBalancerTest.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,8 @@ public void lb_working_withDefaultTarget_rlsResponding() throws Exception {
217217
inOrder.verify(helper).createSubchannel(any(CreateSubchannelArgs.class));
218218
inOrder.verify(helper, atLeast(0))
219219
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
220+
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
221+
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
220222
inOrder.verifyNoMoreInteractions();
221223
assertThat(res.getStatus().isOk()).isTrue();
222224
assertThat(subchannels).hasSize(1);
@@ -325,6 +327,8 @@ public void lb_working_withoutDefaultTarget() throws Exception {
325327
inOrder.verify(helper).createSubchannel(any(CreateSubchannelArgs.class));
326328
inOrder.verify(helper, atLeast(0))
327329
.updateBalancingState(eq(ConnectivityState.CONNECTING), any(SubchannelPicker.class));
330+
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
331+
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
328332
inOrder.verifyNoMoreInteractions();
329333
assertThat(res.getStatus().isOk()).isTrue();
330334

‎util/src/test/java/io/grpc/util/MultiChildLoadBalancerTest.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,6 @@
3232
import static org.mockito.Mockito.never;
3333
import static org.mockito.Mockito.times;
3434
import static org.mockito.Mockito.verify;
35-
import static org.mockito.Mockito.verifyNoMoreInteractions;
3635

3736
import com.google.common.collect.Lists;
3837
import io.grpc.Attributes;
@@ -128,7 +127,7 @@ public void pickAfterResolved() {
128127
(TestLb.TestSubchannelPicker) pickerCaptor.getValue();
129128
assertThat(subchannelPicker.getReadySubchannels()).containsExactly(readySubchannel);
130129

131-
verifyNoMoreInteractions(mockHelper);
130+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
132131
}
133132

134133
@Test
@@ -192,7 +191,7 @@ public void pickAfterResolvedUpdatedHosts() {
192191
verify(mockHelper, times(3)).createSubchannel(any(LoadBalancer.CreateSubchannelArgs.class));
193192
inOrder.verify(mockHelper, times(2)).updateBalancingState(eq(READY), pickerCaptor.capture());
194193

195-
verifyNoMoreInteractions(mockHelper);
194+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
196195
}
197196

198197
@Test

‎util/src/test/java/io/grpc/util/RoundRobinLoadBalancerTest.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -153,7 +153,7 @@ public void pickAfterResolved() throws Exception {
153153
assertEquals(READY, stateCaptor.getAllValues().get(1));
154154
assertThat(getList(pickerCaptor.getValue())).containsExactly(readySubchannel);
155155

156-
verifyNoMoreInteractions(mockHelper);
156+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
157157
}
158158

159159
@Test
@@ -234,7 +234,7 @@ public void pickAfterResolvedUpdatedHosts() throws Exception {
234234
picker = pickerCaptor.getValue();
235235
assertThat(getList(picker)).containsExactly(oldSubchannel, newSubchannel);
236236

237-
verifyNoMoreInteractions(mockHelper);
237+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
238238
}
239239

240240
@Test
@@ -269,7 +269,7 @@ public void pickAfterStateChange() throws Exception {
269269

270270
verify(subchannel, atLeastOnce()).requestConnection();
271271
verify(mockHelper, times(3)).createSubchannel(any(CreateSubchannelArgs.class));
272-
verifyNoMoreInteractions(mockHelper);
272+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
273273
}
274274

275275
@Test
@@ -355,7 +355,7 @@ public void refreshNameResolutionWhenSubchannelConnectionBroken() {
355355
inOrder.verify(mockHelper).updateBalancingState(eq(CONNECTING), isA(EmptyPicker.class));
356356
}
357357

358-
verifyNoMoreInteractions(mockHelper);
358+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
359359
}
360360

361361
@Test
@@ -432,7 +432,7 @@ public void nameResolutionErrorWithActiveChannels() throws Exception {
432432

433433
LoadBalancer.PickResult pickResult2 = pickerCaptor.getValue().pickSubchannel(mockArgs);
434434
assertEquals(readySubchannel, pickResult2.getSubchannel());
435-
verifyNoMoreInteractions(mockHelper);
435+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(mockHelper);
436436
}
437437

438438
@Test

‎util/src/testFixtures/java/io/grpc/util/AbstractTestHelper.java‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,10 @@
1818

1919
import static org.mockito.AdditionalAnswers.delegatesTo;
2020
import static org.mockito.ArgumentMatchers.eq;
21+
import static org.mockito.Mockito.atLeast;
2122
import static org.mockito.Mockito.mock;
23+
import static org.mockito.Mockito.verify;
24+
import static org.mockito.Mockito.verifyNoMoreInteractions;
2225

2326
import com.google.common.collect.Maps;
2427
import io.grpc.Attributes;
@@ -32,12 +35,15 @@
3235
import io.grpc.LoadBalancer.Subchannel;
3336
import io.grpc.LoadBalancer.SubchannelPicker;
3437
import io.grpc.LoadBalancer.SubchannelStateListener;
38+
import io.grpc.SynchronizationContext;
39+
import io.grpc.internal.FakeClock;
3540
import io.grpc.internal.PickFirstLoadBalancerProvider;
3641
import java.net.SocketAddress;
3742
import java.util.Collections;
3843
import java.util.HashMap;
3944
import java.util.List;
4045
import java.util.Map;
46+
import java.util.concurrent.ScheduledExecutorService;
4147
import org.mockito.ArgumentCaptor;
4248
import org.mockito.InOrder;
4349

@@ -60,9 +66,26 @@ public abstract class AbstractTestHelper extends ForwardingLoadBalancerHelper {
6066
protected final Map<Subchannel, Subchannel> realToMockSubChannelMap = new HashMap<>();
6167
private final Map<Subchannel, SubchannelStateListener> subchannelStateListeners =
6268
Maps.newLinkedHashMap();
69+
private final FakeClock fakeClock;
70+
private final SynchronizationContext syncContext;
6371

6472
public abstract Map<List<EquivalentAddressGroup>, Subchannel> getSubchannelMap();
6573

74+
public AbstractTestHelper() {
75+
this(new FakeClock(), new SynchronizationContext(new Thread.UncaughtExceptionHandler() {
76+
@Override
77+
public void uncaughtException(Thread t, Throwable e) {
78+
throw new RuntimeException(e);
79+
}
80+
}));
81+
}
82+
83+
public AbstractTestHelper(FakeClock fakeClock, SynchronizationContext syncContext) {
84+
super();
85+
this.fakeClock = fakeClock;
86+
this.syncContext = syncContext;
87+
}
88+
6689
public Map<Subchannel, Subchannel> getMockToRealSubChannelMap() {
6790
return mockToRealSubChannelMap;
6891
}
@@ -79,6 +102,18 @@ public Map<Subchannel, SubchannelStateListener> getSubchannelStateListeners() {
79102
return subchannelStateListeners;
80103
}
81104

105+
public static final FakeClock.TaskFilter NOT_START_NEXT_CONNECTION =
106+
new FakeClock.TaskFilter() {
107+
@Override
108+
public boolean shouldAccept(Runnable command) {
109+
return !command.toString().contains("StartNextConnection");
110+
}
111+
};
112+
113+
public static int getNumFilteredPendingTasks(FakeClock fakeClock) {
114+
return fakeClock.getPendingTasks(NOT_START_NEXT_CONNECTION).size();
115+
}
116+
82117
public void deliverSubchannelState(Subchannel subchannel, ConnectivityStateInfo newState) {
83118
Subchannel realSc = getMockToRealSubChannelMap().get(subchannel);
84119
if (realSc == null) {
@@ -128,6 +163,16 @@ public void setChannel(Subchannel subchannel, Channel channel) {
128163
((TestSubchannel)subchannel).channel = channel;
129164
}
130165

166+
@Override
167+
public SynchronizationContext getSynchronizationContext() {
168+
return syncContext;
169+
}
170+
171+
@Override
172+
public ScheduledExecutorService getScheduledExecutorService() {
173+
return fakeClock.getScheduledExecutorService();
174+
}
175+
131176
@Override
132177
public String toString() {
133178
return "Test Helper";
@@ -148,6 +193,17 @@ public static void refreshInvokedAndUpdateBS(InOrder inOrder, ConnectivityState
148193
}
149194
}
150195

196+
public static void verifyNoMoreMeaningfulInteractions(Helper helper) {
197+
verify(helper, atLeast(0)).getSynchronizationContext();
198+
verify(helper, atLeast(0)).getScheduledExecutorService();
199+
verifyNoMoreInteractions(helper);
200+
}
201+
202+
public static void verifyNoMoreMeaningfulInteractions(Helper helper, InOrder inOrder) {
203+
inOrder.verify(helper, atLeast(0)).getSynchronizationContext();
204+
inOrder.verify(helper, atLeast(0)).getScheduledExecutorService();
205+
inOrder.verifyNoMoreInteractions();
206+
}
151207

152208
protected class TestSubchannel extends ForwardingSubchannel {
153209
CreateSubchannelArgs args;

‎xds/src/main/java/io/grpc/xds/XdsEndpointResource.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ protected EdsUpdate doParse(Args args, Message unpackedMessage) throws ResourceI
101101
}
102102

103103
private static boolean isEnabledXdsDualStack() {
104-
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, false);
104+
return GrpcUtil.getFlag(GRPC_EXPERIMENTAL_XDS_DUALSTACK_ENDPOINTS, true);
105105
}
106106

107107
private static EdsUpdate processClusterLoadAssignment(ClusterLoadAssignment assignment)

‎xds/src/test/java/io/grpc/xds/LeastRequestLoadBalancerTest.java‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -159,7 +159,7 @@ public void pickAfterResolved() throws Exception {
159159
assertEquals(READY, stateCaptor.getAllValues().get(1));
160160
assertThat(getList(pickerCaptor.getValue())).containsExactly(readySubchannel);
161161

162-
verifyNoMoreInteractions(helper);
162+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
163163
}
164164

165165
@Test
@@ -226,7 +226,7 @@ public void pickAfterResolvedUpdatedHosts() throws Exception {
226226

227227
assertThat(getList(pickerCaptor.getValue())).containsExactly(oldSubchannel, newSubchannel);
228228

229-
verifyNoMoreInteractions(helper);
229+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
230230
}
231231

232232
private Subchannel getSubchannel(EquivalentAddressGroup removedEag) {
@@ -288,7 +288,7 @@ public void pickAfterStateChange() throws Exception {
288288
int expectedCount = PickFirstLoadBalancerProvider.isEnabledNewPickFirst() ? 1 : 2;
289289
verify(subchannel, times(expectedCount)).requestConnection();
290290
verify(helper, times(3)).createSubchannel(any(CreateSubchannelArgs.class));
291-
verifyNoMoreInteractions(helper);
291+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
292292
}
293293

294294
@Test
@@ -319,7 +319,7 @@ public void pickAfterConfigChange() {
319319
// At this point it should use a ReadyPicker with newConfig
320320
pickerCaptor.getValue().pickSubchannel(mockArgs);
321321
verify(mockRandom, times(oldConfig.choiceCount + newConfig.choiceCount)).nextInt(1);
322-
verifyNoMoreInteractions(helper);
322+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
323323
}
324324

325325
@Test
@@ -375,7 +375,7 @@ public void stayTransientFailureUntilReady() {
375375
inOrder.verify(helper).updateBalancingState(eq(READY), isA(ReadyPicker.class));
376376

377377
verify(helper, times(3)).createSubchannel(any(CreateSubchannelArgs.class));
378-
verifyNoMoreInteractions(helper);
378+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
379379
}
380380

381381
private String getStatusString(SubchannelPicker picker) {
@@ -426,7 +426,7 @@ public void refreshNameResolutionWhenSubchannelConnectionBroken() {
426426
inOrder.verify(helper).updateBalancingState(eq(CONNECTING), isA(EmptyPicker.class));
427427
}
428428

429-
verifyNoMoreInteractions(helper);
429+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
430430
}
431431

432432
@Test
@@ -548,7 +548,7 @@ public void nameResolutionErrorWithActiveChannels() throws Exception {
548548
LoadBalancer.PickResult pickResult2 = pickerCaptor.getValue().pickSubchannel(mockArgs);
549549
verify(mockRandom, times(choiceCount * 2)).nextInt(1);
550550
assertEquals(readySubchannel, pickResult2.getSubchannel());
551-
verifyNoMoreInteractions(helper);
551+
AbstractTestHelper.verifyNoMoreMeaningfulInteractions(helper);
552552
}
553553

554554
@Test

0 commit comments

Comments
 (0)