Skip to content

Commit 4fbe726

Browse files
jsorjsor
authored
Merge pull request #225 from clue-labs/eyeball-cleanup
Fix cancelling happy eyeballs to stop timer, fix rejection reason and simplify timer logic
2 parents 13d8259 + 511f85d commit 4fbe726

File tree

5 files changed

+297
-116
lines changed

5 files changed

+297
-116
lines changed

‎composer.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
"php": ">=5.3.0",
88
"evenement/evenement": "^3.0 || ^2.0 || ^1.0",
99
"react/dns": "^1.1",
10-
"react/event-loop": "^1.0 || ^0.5 || ^0.4 || ^0.3.5",
10+
"react/event-loop": "^1.0 || ^0.5",
1111
"react/promise": "^2.6.0 || ^1.2.1",
1212
"react/promise-timer": "^1.4.0",
1313
"react/stream": "^1.1"

‎src/HappyEyeBallsConnectionBuilder.php‎

Lines changed: 40 additions & 44 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;
@@ -48,66 +61,52 @@ public function __construct(LoopInterface $loop, ConnectorInterface $connector,
4861

4962
public function connect()
5063
{
64+
$timer = null;
5165
$that = $this;
52-
return new Promise\Promise(function ($resolve, $reject) use ($that) {
66+
return new Promise\Promise(function ($resolve, $reject) use ($that, &$timer) {
5367
$lookupResolve = function ($type) use ($that, $resolve, $reject) {
5468
return function (array $ips) use ($that, $type, $resolve, $reject) {
5569
unset($that->resolverPromises[$type]);
5670
$that->resolved[$type] = true;
5771

5872
$that->mixIpsIntoConnectQueue($ips);
5973

60-
if ($that->timer instanceof TimerInterface) {
74+
if ($that->nextAttemptTimer instanceof TimerInterface) {
6175
return;
6276
}
6377

6478
$that->check($resolve, $reject);
6579
};
6680
};
6781

68-
$ipv4Deferred = null;
69-
$timer = 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
});
9899

99100
return $deferred->promise();
100101
})->then($lookupResolve(Message::TYPE_A));
101102
}, function ($_, $reject) use ($that, &$timer) {
102-
$that->cleanUp();
103+
$reject(new \RuntimeException('Connection to ' . $that->uri . ' cancelled' . (!$that->connectionPromises ? ' during DNS lookup' : '')));
104+
$_ = $reject = null;
103105

106+
$that->cleanUp();
104107
if ($timer instanceof TimerInterface) {
105108
$that->loop->cancelTimer($timer);
106109
}
107-
108-
$reject(new \RuntimeException('Connection to ' . $that->uri . ' cancelled during DNS lookup'));
109-
110-
$_ = $reject = null;
111110
});
112111
}
113112

@@ -126,7 +125,6 @@ public function resolve($type, $reject)
126125
}
127126

128127
if ($that->ipsCount === 0) {
129-
$that->resolved = null;
130128
$that->resolverPromises = null;
131129
$reject(new \RuntimeException('Connection to ' . $that->uri . ' failed during DNS lookup: DNS error'));
132130
}
@@ -138,9 +136,9 @@ public function resolve($type, $reject)
138136
*/
139137
public function check($resolve, $reject)
140138
{
141-
if (\count($this->connectQueue) === 0 && $this->resolved[Message::TYPE_A] === true && $this->resolved[Message::TYPE_AAAA] === true && $this->timer instanceof TimerInterface) {
142-
$this->loop->cancelTimer($this->timer);
143-
$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;
144142
}
145143

146144
if (\count($this->connectQueue) === 0) {
@@ -156,7 +154,7 @@ public function check($resolve, $reject)
156154
$that->cleanUp();
157155

158156
$resolve($connection);
159-
}, function () use ($that, $ip, $resolve, $reject) {
157+
}, function () use ($that, $ip, $reject) {
160158
unset($that->connectionPromises[$ip]);
161159

162160
$that->failureCount++;
@@ -178,8 +176,8 @@ public function check($resolve, $reject)
178176
*
179177
* @link https://tools.ietf.org/html/rfc8305#section-5
180178
*/
181-
if ((\count($this->connectQueue) > 0 || ($this->resolved[Message::TYPE_A] === false || $this->resolved[Message::TYPE_AAAA] === false)) && $this->timer === null) {
182-
$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) {
183181
$that->check($resolve, $reject);
184182
});
185183
}
@@ -240,23 +238,21 @@ public function attemptConnection($ip)
240238
*/
241239
public function cleanUp()
242240
{
243-
/** @var CancellablePromiseInterface $promise */
244-
foreach ($this->connectionPromises as $index => $connectionPromise) {
241+
foreach ($this->connectionPromises as $connectionPromise) {
245242
if ($connectionPromise instanceof CancellablePromiseInterface) {
246243
$connectionPromise->cancel();
247244
}
248245
}
249246

250-
/** @var CancellablePromiseInterface $promise */
251-
foreach ($this->resolverPromises as $index => $resolverPromise) {
247+
foreach ($this->resolverPromises as $resolverPromise) {
252248
if ($resolverPromise instanceof CancellablePromiseInterface) {
253249
$resolverPromise->cancel();
254250
}
255251
}
256252

257-
if ($this->timer instanceof TimerInterface) {
258-
$this->loop->cancelTimer($this->timer);
259-
$this->timer = null;
253+
if ($this->nextAttemptTimer instanceof TimerInterface) {
254+
$this->loop->cancelTimer($this->nextAttemptTimer);
255+
$this->nextAttemptTimer = null;
260256
}
261257
}
262258

‎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
}

0 commit comments

Comments
 (0)