Skip to content

Commit 511f85d

Browse files
clueclue
committed
Fix resolution delay (50ms) and simplify related timer logic
1 parent b400a57 commit 511f85d

File tree

3 files changed

+61
-63
lines changed

3 files changed

+61
-63
lines changed

‎src/HappyEyeBallsConnectionBuilder.php‎

Lines changed: 35 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,21 @@
1414
*/
1515
final class HappyEyeBallsConnectionBuilder
1616
{
17-
const CONNECT_INTERVAL = 0.1;
18-
const RESOLVE_WAIT = 0.5;
17+
/**
18+
* As long as we haven't connected yet keep popping an IP address of the connect queue until one of them
19+
* succeeds or they all fail. We will wait 100ms between connection attempts as per RFC.
20+
*
21+
* @link https://tools.ietf.org/html/rfc8305#section-5
22+
*/
23+
const CONNECTION_ATTEMPT_DELAY = 0.1;
24+
25+
/**
26+
* Delay `A` lookup by 50ms sending out connection to IPv4 addresses when IPv6 records haven't
27+
* resolved yet as per RFC.
28+
*
29+
* @link https://tools.ietf.org/html/rfc8305#section-3
30+
*/
31+
const RESOLUTION_DELAY = 0.05;
1932

2033
public $loop;
2134
public $connector;
@@ -29,7 +42,7 @@ final class HappyEyeBallsConnectionBuilder
2942
public $resolverPromises = array();
3043
public $connectionPromises = array();
3144
public $connectQueue = array();
32-
public $timer;
45+
public $nextAttemptTimer;
3346
public $parts;
3447
public $ipsCount = 0;
3548
public $failureCount = 0;
@@ -58,40 +71,28 @@ public function connect()
5871

5972
$that->mixIpsIntoConnectQueue($ips);
6073

61-
if ($that->timer instanceof TimerInterface) {
74+
if ($that->nextAttemptTimer instanceof TimerInterface) {
6275
return;
6376
}
6477

6578
$that->check($resolve, $reject);
6679
};
6780
};
6881

69-
$ipv4Deferred = null;
70-
$that->resolverPromises[Message::TYPE_AAAA] = $that->resolve(Message::TYPE_AAAA, $reject)->then($lookupResolve(Message::TYPE_AAAA))->then(function () use (&$ipv4Deferred) {
71-
if ($ipv4Deferred instanceof Promise\Deferred) {
72-
$ipv4Deferred->resolve();
73-
}
74-
});
75-
$that->resolverPromises[Message::TYPE_A] = $that->resolve(Message::TYPE_A, $reject)->then(function ($ips) use ($that, &$ipv4Deferred, &$timer) {
82+
$that->resolverPromises[Message::TYPE_AAAA] = $that->resolve(Message::TYPE_AAAA, $reject)->then($lookupResolve(Message::TYPE_AAAA));
83+
$that->resolverPromises[Message::TYPE_A] = $that->resolve(Message::TYPE_A, $reject)->then(function ($ips) use ($that, &$timer) {
84+
// happy path: IPv6 has resolved already, continue with IPv4 addresses
7685
if ($that->resolved[Message::TYPE_AAAA] === true) {
77-
return Promise\resolve($ips);
86+
return $ips;
7887
}
7988

80-
/**
81-
* Delay A lookup by 50ms sending out connection to IPv4 addresses when IPv6 records haven't
82-
* resolved yet as per RFC.
83-
*
84-
* @link https://tools.ietf.org/html/rfc8305#section-3
85-
*/
86-
$ipv4Deferred = new Promise\Deferred();
89+
// Otherwise delay processing IPv4 lookup until short timer passes or IPv6 resolves in the meantime
8790
$deferred = new Promise\Deferred();
88-
89-
$timer = $that->loop->addTimer($that::RESOLVE_WAIT, function () use ($deferred, $ips) {
90-
$ipv4Deferred = null;
91+
$timer = $that->loop->addTimer($that::RESOLUTION_DELAY, function () use ($deferred, $ips) {
9192
$deferred->resolve($ips);
9293
});
9394

94-
$ipv4Deferred->promise()->then(function () use ($that, &$timer, $deferred, $ips) {
95+
$that->resolverPromises[Message::TYPE_AAAA]->then(function () use ($that, $timer, $deferred, $ips) {
9596
$that->loop->cancelTimer($timer);
9697
$deferred->resolve($ips);
9798
});
@@ -124,7 +125,6 @@ public function resolve($type, $reject)
124125
}
125126

126127
if ($that->ipsCount === 0) {
127-
$that->resolved = null;
128128
$that->resolverPromises = null;
129129
$reject(new \RuntimeException('Connection to ' . $that->uri . ' failed during DNS lookup: DNS error'));
130130
}
@@ -136,9 +136,9 @@ public function resolve($type, $reject)
136136
*/
137137
public function check($resolve, $reject)
138138
{
139-
if (\count($this->connectQueue) === 0 && $this->resolved[Message::TYPE_A] === true && $this->resolved[Message::TYPE_AAAA] === true && $this->timer instanceof TimerInterface) {
140-
$this->loop->cancelTimer($this->timer);
141-
$this->timer = null;
139+
if (\count($this->connectQueue) === 0 && $this->resolved[Message::TYPE_A] === true && $this->resolved[Message::TYPE_AAAA] === true && $this->nextAttemptTimer instanceof TimerInterface) {
140+
$this->loop->cancelTimer($this->nextAttemptTimer);
141+
$this->nextAttemptTimer = null;
142142
}
143143

144144
if (\count($this->connectQueue) === 0) {
@@ -154,7 +154,7 @@ public function check($resolve, $reject)
154154
$that->cleanUp();
155155

156156
$resolve($connection);
157-
}, function () use ($that, $ip, $resolve, $reject) {
157+
}, function () use ($that, $ip, $reject) {
158158
unset($that->connectionPromises[$ip]);
159159

160160
$that->failureCount++;
@@ -176,8 +176,8 @@ public function check($resolve, $reject)
176176
*
177177
* @link https://tools.ietf.org/html/rfc8305#section-5
178178
*/
179-
if ((\count($this->connectQueue) > 0 || ($this->resolved[Message::TYPE_A] === false || $this->resolved[Message::TYPE_AAAA] === false)) && $this->timer === null) {
180-
$this->timer = $this->loop->addPeriodicTimer(self::CONNECT_INTERVAL, function () use ($that, $resolve, $reject) {
179+
if ((\count($this->connectQueue) > 0 || ($this->resolved[Message::TYPE_A] === false || $this->resolved[Message::TYPE_AAAA] === false)) && $this->nextAttemptTimer === null) {
180+
$this->nextAttemptTimer = $this->loop->addPeriodicTimer(self::CONNECTION_ATTEMPT_DELAY, function () use ($that, $resolve, $reject) {
181181
$that->check($resolve, $reject);
182182
});
183183
}
@@ -238,23 +238,21 @@ public function attemptConnection($ip)
238238
*/
239239
public function cleanUp()
240240
{
241-
/** @var CancellablePromiseInterface $promise */
242-
foreach ($this->connectionPromises as $index => $connectionPromise) {
241+
foreach ($this->connectionPromises as $connectionPromise) {
243242
if ($connectionPromise instanceof CancellablePromiseInterface) {
244243
$connectionPromise->cancel();
245244
}
246245
}
247246

248-
/** @var CancellablePromiseInterface $promise */
249-
foreach ($this->resolverPromises as $index => $resolverPromise) {
247+
foreach ($this->resolverPromises as $resolverPromise) {
250248
if ($resolverPromise instanceof CancellablePromiseInterface) {
251249
$resolverPromise->cancel();
252250
}
253251
}
254252

255-
if ($this->timer instanceof TimerInterface) {
256-
$this->loop->cancelTimer($this->timer);
257-
$this->timer = null;
253+
if ($this->nextAttemptTimer instanceof TimerInterface) {
254+
$this->loop->cancelTimer($this->nextAttemptTimer);
255+
$this->nextAttemptTimer = null;
258256
}
259257
}
260258

‎tests/FunctionalConnectorTest.php‎

Lines changed: 25 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -56,15 +56,16 @@ public function connectionToRemoteTCP4n6ServerShouldResultInOurIP()
5656
*/
5757
public function connectionToRemoteTCP4ServerShouldResultInOurIP()
5858
{
59-
if ($this->ipv4() === false) {
60-
$this->markTestSkipped('IPv4 connection not supported on this system');
61-
}
62-
6359
$loop = Factory::create();
6460

6561
$connector = new Connector($loop, array('happy_eyeballs' => true));
6662

67-
$ip = Block\await($this->request('ipv4.tlund.se', $connector), $loop, self::TIMEOUT);
63+
try {
64+
$ip = Block\await($this->request('ipv4.tlund.se', $connector), $loop, self::TIMEOUT);
65+
} catch (\Exception $e) {
66+
$this->checkIpv4();
67+
throw $e;
68+
}
6869

6970
$this->assertSame($ip, filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4), $ip);
7071
$this->assertFalse(filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6), $ip);
@@ -76,15 +77,16 @@ public function connectionToRemoteTCP4ServerShouldResultInOurIP()
7677
*/
7778
public function connectionToRemoteTCP6ServerShouldResultInOurIP()
7879
{
79-
if ($this->ipv6() === false) {
80-
$this->markTestSkipped('IPv6 connection not supported on this system');
81-
}
82-
8380
$loop = Factory::create();
8481

8582
$connector = new Connector($loop, array('happy_eyeballs' => true));
8683

87-
$ip = Block\await($this->request('ipv6.tlund.se', $connector), $loop, self::TIMEOUT);
84+
try {
85+
$ip = Block\await($this->request('ipv6.tlund.se', $connector), $loop, self::TIMEOUT);
86+
} catch (\Exception $e) {
87+
$this->checkIpv6();
88+
throw $e;
89+
}
8890

8991
$this->assertFalse(filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV4), $ip);
9092
$this->assertSame($ip, filter_var($ip, FILTER_VALIDATE_IP, FILTER_FLAG_IPV6), $ip);
@@ -105,33 +107,33 @@ private function request($host, ConnectorInterface $connector)
105107
{
106108
$that = $this;
107109
return $connector->connect($host . ':80')->then(function (ConnectionInterface $connection) use ($host) {
108-
$connection->write("GET / HTTP/1.1\r\nHost: " . $host . "\r\n\r\n");
110+
$connection->write("GET / HTTP/1.1\r\nHost: " . $host . "\r\nConnection: close\r\n\r\n");
109111

110112
return \React\Promise\Stream\buffer($connection);
111113
})->then(function ($response) use ($that) {
112114
return $that->parseIpFromPage($response);
113115
});
114116
}
115117

116-
private function ipv4()
118+
private function checkIpv4()
117119
{
118-
if ($this->ipv4 !== null) {
119-
return $this->ipv4;
120+
if ($this->ipv4 === null) {
121+
$this->ipv4 = !!@file_get_contents('http://ipv4.tlund.se/');
120122
}
121123

122-
$this->ipv4 = !!@file_get_contents('http://ipv4.tlund.se/');
123-
124-
return $this->ipv4;
124+
if (!$this->ipv4) {
125+
$this->markTestSkipped('IPv4 connection not supported on this system');
126+
}
125127
}
126128

127-
private function ipv6()
129+
private function checkIpv6()
128130
{
129-
if ($this->ipv6 !== null) {
130-
return $this->ipv6;
131+
if ($this->ipv6 === null) {
132+
$this->ipv6 = !!@file_get_contents('http://ipv6.tlund.se/');
131133
}
132134

133-
$this->ipv6 = !!@file_get_contents('http://ipv6.tlund.se/');
134-
135-
return $this->ipv6;
135+
if (!$this->ipv6) {
136+
$this->markTestSkipped('IPv6 connection not supported on this system');
137+
}
136138
}
137139
}

‎tests/HappyEyeBallsConnectionBuilderTest.php‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,10 +118,8 @@ public function testConnectWillStartTimerAndCancelTimerWhenIpv4ResolvesAndIpv6Re
118118

119119
public function testCancelConnectWillRejectPromiseAndCancelBothDnsLookups()
120120
{
121-
$timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock();
122121
$loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock();
123-
$loop->expects($this->once())->method('addTimer')->willReturn($timer);
124-
$loop->expects($this->once())->method('cancelTimer')->with($timer);
122+
$loop->expects($this->never())->method('addTimer');
125123

126124
$connector = $this->getMockBuilder('React\Socket\ConnectorInterface')->getMock();
127125
$connector->expects($this->never())->method('connect');

0 commit comments

Comments
 (0)