Skip to content

Commit f710d6e

Browse files
jsorjsor
authored
Merge pull request #232 from clue-labs/eyeballs-faster
Immediately try next connection when one attempt fails and fix possible race conditions (happy eyeballs)
2 parents e88fcf3 + 6b3aa7b commit f710d6e

File tree

3 files changed

+179
-52
lines changed

3 files changed

+179
-52
lines changed

‎src/HappyEyeBallsConnectionBuilder.php

Lines changed: 23 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -145,18 +145,33 @@ public function check($resolve, $reject)
145145
{
146146
$ip = \array_shift($this->connectQueue);
147147

148+
// start connection attempt and remember array position to later unset again
149+
$this->connectionPromises[] = $this->attemptConnection($ip);
150+
\end($this->connectionPromises);
151+
$index = \key($this->connectionPromises);
152+
148153
$that = $this;
149-
$that->connectionPromises[$ip] = $this->attemptConnection($ip)->then(function ($connection) use ($that, $ip, $resolve) {
150-
unset($that->connectionPromises[$ip]);
154+
$that->connectionPromises[$index]->then(function ($connection) use ($that, $index, $resolve) {
155+
unset($that->connectionPromises[$index]);
151156

152157
$that->cleanUp();
153158

154159
$resolve($connection);
155-
}, function (\Exception $e) use ($that, $ip, $reject) {
156-
unset($that->connectionPromises[$ip]);
160+
}, function (\Exception $e) use ($that, $index, $resolve, $reject) {
161+
unset($that->connectionPromises[$index]);
157162

158163
$that->failureCount++;
159164

165+
// start next connection attempt immediately on error
166+
if ($that->connectQueue) {
167+
if ($that->nextAttemptTimer !== null) {
168+
$that->loop->cancelTimer($that->nextAttemptTimer);
169+
$that->nextAttemptTimer = null;
170+
}
171+
172+
$that->check($resolve, $reject);
173+
}
174+
160175
if ($that->hasBeenResolved() === false) {
161176
return;
162177
}
@@ -170,7 +185,7 @@ public function check($resolve, $reject)
170185

171186
// Allow next connection attempt in 100ms: https://tools.ietf.org/html/rfc8305#section-5
172187
// Only start timer when more IPs are queued or when DNS query is still pending (might add more IPs)
173-
if (\count($this->connectQueue) > 0 || $this->resolved[Message::TYPE_A] === false || $this->resolved[Message::TYPE_AAAA] === false) {
188+
if ($this->nextAttemptTimer === null && (\count($this->connectQueue) > 0 || $this->resolved[Message::TYPE_A] === false || $this->resolved[Message::TYPE_AAAA] === false)) {
174189
$this->nextAttemptTimer = $this->loop->addTimer(self::CONNECTION_ATTEMPT_DELAY, function () use ($that, $resolve, $reject) {
175190
$that->nextAttemptTimer = null;
176191

@@ -236,6 +251,9 @@ public function attemptConnection($ip)
236251
*/
237252
public function cleanUp()
238253
{
254+
// clear list of outstanding IPs to avoid creating new connections
255+
$this->connectQueue = array();
256+
239257
foreach ($this->connectionPromises as $connectionPromise) {
240258
if ($connectionPromise instanceof CancellablePromiseInterface) {
241259
$connectionPromise->cancel();

‎tests/HappyEyeBallsConnectionBuilderTest.php

Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -217,6 +217,77 @@ public function testConnectWillStartConnectingWithAttemptTimerButWithoutResoluti
217217
$deferred->reject(new \RuntimeException());
218218
}
219219

220+
public function testConnectWillStartConnectingWithAttemptTimerWhenIpv6AndIpv4ResolvesAndWillStartNextConnectionAttemptWithoutAttemptTimerImmediatelyWhenFirstConnectionAttemptFails()
221+
{
222+
$timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock();
223+
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
224+
$loop->expects($this->once())->method('addTimer')->with(0.1, $this->anything())->willReturn($timer);
225+
$loop->expects($this->once())->method('cancelTimer')->with($timer);
226+
227+
$deferred = new Deferred();
228+
$connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
229+
$connector->expects($this->exactly(2))->method('connect')->withConsecutive(
230+
array('tcp://[::1]:80?hostname=reactphp.org'),
231+
array('tcp://127.0.0.1:80?hostname=reactphp.org')
232+
)->willReturnOnConsecutiveCalls(
233+
$deferred->promise(),
234+
new Promise(function () { })
235+
);
236+
237+
$resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
238+
$resolver->expects($this->exactly(2))->method('resolveAll')->withConsecutive(
239+
array('reactphp.org', Message::TYPE_AAAA),
240+
array('reactphp.org', Message::TYPE_A)
241+
)->willReturnOnConsecutiveCalls(
242+
\React\Promise\resolve(array('::1')),
243+
\React\Promise\resolve(array('127.0.0.1'))
244+
);
245+
246+
$uri = 'tcp://reactphp.org:80';
247+
$host = 'reactphp.org';
248+
$parts = parse_url($uri);
249+
250+
$builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
251+
252+
$builder->connect();
253+
254+
$deferred->reject(new \RuntimeException());
255+
}
256+
257+
public function testConnectWillStartConnectingWithAttemptTimerWhenOnlyIpv6ResolvesAndWillStartNextConnectionAttemptWithoutAttemptTimerImmediatelyWhenFirstConnectionAttemptFails()
258+
{
259+
$timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock();
260+
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
261+
$loop->expects($this->once())->method('addTimer')->with(0.1, $this->anything())->willReturn($timer);
262+
$loop->expects($this->once())->method('cancelTimer')->with($timer);
263+
264+
$connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
265+
$connector->expects($this->exactly(2))->method('connect')->withConsecutive(
266+
array('tcp://[::1]:80?hostname=reactphp.org'),
267+
array('tcp://[::2]:80?hostname=reactphp.org')
268+
)->willReturnOnConsecutiveCalls(
269+
\React\Promise\reject(new \RuntimeException()),
270+
new Promise(function () { })
271+
);
272+
273+
$resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
274+
$resolver->expects($this->exactly(2))->method('resolveAll')->withConsecutive(
275+
array('reactphp.org', Message::TYPE_AAAA),
276+
array('reactphp.org', Message::TYPE_A)
277+
)->willReturnOnConsecutiveCalls(
278+
\React\Promise\resolve(array('::1', '::2')),
279+
\React\Promise\reject(new \RuntimeException())
280+
);
281+
282+
$uri = 'tcp://reactphp.org:80';
283+
$host = 'reactphp.org';
284+
$parts = parse_url($uri);
285+
286+
$builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
287+
288+
$builder->connect();
289+
}
290+
220291
public function testConnectWillStartConnectingAndWillStartNextConnectionWithoutNewAttemptTimerWhenNextAttemptTimerFiresAfterIpv4Rejected()
221292
{
222293
$timer = null;
@@ -492,4 +563,89 @@ public function testAttemptConnectionWillConnectViaConnectorToGivenIpv6WithAllUr
492563

493564
$builder->attemptConnection('::1');
494565
}
566+
567+
public function testCheckCallsRejectFunctionImmediateWithoutLeavingDanglingPromiseWhenConnectorRejectsImmediately()
568+
{
569+
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
570+
571+
$connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
572+
$connector->expects($this->once())->method('connect')->with('tcp://[::1]:80/path?test=yes&hostname=reactphp.org#start')->willReturn(\React\Promise\reject(new \RuntimeException()));
573+
574+
$resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
575+
$resolver->expects($this->never())->method('resolveAll');
576+
577+
$uri = 'tcp://reactphp.org:80/path?test=yes#start';
578+
$host = 'reactphp.org';
579+
$parts = parse_url($uri);
580+
581+
$builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
582+
583+
$ref = new \ReflectionProperty($builder, 'connectQueue');
584+
$ref->setAccessible(true);
585+
$ref->setValue($builder, array('::1'));
586+
587+
$builder->check($this->expectCallableNever(), function () { });
588+
589+
$ref = new \ReflectionProperty($builder, 'connectionPromises');
590+
$ref->setAccessible(true);
591+
$promises = $ref->getValue($builder);
592+
593+
$this->assertEquals(array(), $promises);
594+
}
595+
596+
public function testCleanUpCancelsAllPendingConnectionAttempts()
597+
{
598+
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
599+
600+
$connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
601+
$connector->expects($this->exactly(2))->method('connect')->with('tcp://[::1]:80/path?test=yes&hostname=reactphp.org#start')->willReturnOnConsecutiveCalls(
602+
new Promise(function () { }, $this->expectCallableOnce()),
603+
new Promise(function () { }, $this->expectCallableOnce())
604+
);
605+
606+
$resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
607+
$resolver->expects($this->never())->method('resolveAll');
608+
609+
$uri = 'tcp://reactphp.org:80/path?test=yes#start';
610+
$host = 'reactphp.org';
611+
$parts = parse_url($uri);
612+
613+
$builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
614+
615+
$ref = new \ReflectionProperty($builder, 'connectQueue');
616+
$ref->setAccessible(true);
617+
$ref->setValue($builder, array('::1', '::1'));
618+
619+
$builder->check($this->expectCallableNever(), function () { });
620+
$builder->check($this->expectCallableNever(), function () { });
621+
622+
$builder->cleanUp();
623+
}
624+
625+
public function testCleanUpCancelsAllPendingConnectionAttemptsWithoutStartingNewAttemptsDueToCancellationRejection()
626+
{
627+
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
628+
629+
$connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
630+
$connector->expects($this->once())->method('connect')->with('tcp://[::1]:80/path?test=yes&hostname=reactphp.org#start')->willReturn(new Promise(function () { }, function () {
631+
throw new \RuntimeException();
632+
}));
633+
634+
$resolver = $this->getMockBuilder('React\Dns\Resolver\ResolverInterface')->getMock();
635+
$resolver->expects($this->never())->method('resolveAll');
636+
637+
$uri = 'tcp://reactphp.org:80/path?test=yes#start';
638+
$host = 'reactphp.org';
639+
$parts = parse_url($uri);
640+
641+
$builder = new HappyEyeBallsConnectionBuilder($loop, $connector, $resolver, $uri, $host, $parts);
642+
643+
$ref = new \ReflectionProperty($builder, 'connectQueue');
644+
$ref->setAccessible(true);
645+
$ref->setValue($builder, array('::1', '::1'));
646+
647+
$builder->check($this->expectCallableNever(), function () { });
648+
649+
$builder->cleanUp();
650+
}
495651
}

‎tests/HappyEyeBallsConnectorTest.php

Lines changed: 0 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -138,30 +138,6 @@ public function testPassThroughResolverIfGivenExplicitHost()
138138
$this->loop->run();
139139
}
140140

141-
/**
142-
* @dataProvider provideIpvAddresses
143-
*/
144-
public function testIpv4ResolvesFirstSoButIPv6IsTheFirstToConnect(array $ipv6, array $ipv4)
145-
{
146-
$this->resolver->expects($this->at(0))->method('resolveAll')->with('google.com', Message::TYPE_AAAA)->will($this->returnValue(Promise\Timer\resolve(0.001, $this->loop)->then(function () use ($ipv6) {
147-
return Promise\resolve($ipv6);
148-
})));
149-
$this->resolver->expects($this->at(1))->method('resolveAll')->with('google.com', Message::TYPE_A)->will($this->returnValue(Promise\resolve($ipv4)));
150-
$i = 0;
151-
while (count($ipv6) > 0 || count($ipv4) > 0) {
152-
if (count($ipv6) > 0) {
153-
$this->tcp->expects($this->at($i++))->method('connect')->with($this->equalTo('scheme://[' . array_shift($ipv6) . ']:80/?hostname=google.com'))->will($this->returnValue(Promise\resolve()));
154-
}
155-
if (count($ipv4) > 0) {
156-
$this->tcp->expects($this->at($i++))->method('connect')->with($this->equalTo('scheme://' . array_shift($ipv4) . ':80/?hostname=google.com'))->will($this->returnValue(Promise\resolve()));
157-
}
158-
}
159-
160-
$this->connector->connect('scheme://google.com:80/?hostname=google.com');
161-
162-
$this->loop->run();
163-
}
164-
165141
/**
166142
* @dataProvider provideIpvAddresses
167143
*/
@@ -202,29 +178,6 @@ public function testIpv6DoesntResolvesWhileIpv4DoesFirstSoIpv4Connects(array $ip
202178
$this->loop->run();
203179
}
204180

205-
/**
206-
* @dataProvider provideIpvAddresses
207-
*/
208-
public function testAttemptsToConnectBothIpv6AndIpv4Addresses(array $ipv6, array $ipv4)
209-
{
210-
$this->resolver->expects($this->at(0))->method('resolveAll')->with('google.com', Message::TYPE_AAAA)->will($this->returnValue(Promise\resolve($ipv6)));
211-
$this->resolver->expects($this->at(1))->method('resolveAll')->with('google.com', Message::TYPE_A)->will($this->returnValue(Promise\resolve($ipv4)));
212-
213-
$i = 0;
214-
while (count($ipv6) > 0 || count($ipv4) > 0) {
215-
if (count($ipv6) > 0) {
216-
$this->tcp->expects($this->at($i++))->method('connect')->with($this->equalTo('scheme://[' . array_shift($ipv6) . ']:80/?hostname=google.com'))->will($this->returnValue(Promise\resolve()));
217-
}
218-
if (count($ipv4) > 0) {
219-
$this->tcp->expects($this->at($i++))->method('connect')->with($this->equalTo('scheme://' . array_shift($ipv4) . ':80/?hostname=google.com'))->will($this->returnValue(Promise\resolve()));
220-
}
221-
}
222-
223-
$this->connector->connect('scheme://google.com:80/?hostname=google.com');
224-
225-
$this->loop->run();
226-
}
227-
228181
public function testThatTheIpv4ConnectionWillWait100MilisecondsWhenIpv6AndIpv4ResolveSimultaniously()
229182
{
230183
$timings = array();

0 commit comments

Comments
 (0)