Skip to content

Commit 33b026a

Browse files
WyriHaximusWyriHaximus
authored
Merge pull request #118 from clue-labs/cancellation
Improve promise cancellation for DNS lookup retries and clean up any garbage references
2 parents 6f2f761 + b5a8542 commit 33b026a

File tree

2 files changed

+200
-12
lines changed

2 files changed

+200
-12
lines changed

‎src/Query/RetryExecutor.php‎

Lines changed: 47 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
namespace React\Dns\Query;
44

5+
use React\Promise\CancellablePromiseInterface;
56
use React\Promise\Deferred;
67

78
class RetryExecutor implements ExecutorInterface
@@ -22,23 +23,57 @@ public function query($nameserver, Query $query)
2223

2324
public function tryQuery($nameserver, Query $query, $retries)
2425
{
25-
$that = $this;
26-
$errorback = function ($error) use ($nameserver, $query, $retries, $that) {
27-
if (!$error instanceof TimeoutException) {
28-
throw $error;
26+
$deferred = new Deferred(function () use (&$promise) {
27+
if ($promise instanceof CancellablePromiseInterface) {
28+
$promise->cancel();
2929
}
30-
if (0 >= $retries) {
31-
throw new \RuntimeException(
32-
sprintf("DNS query for %s failed: too many retries", $query->name),
30+
});
31+
32+
$success = function ($value) use ($deferred, &$errorback) {
33+
$errorback = null;
34+
$deferred->resolve($value);
35+
};
36+
37+
$executor = $this->executor;
38+
$errorback = function ($e) use ($deferred, &$promise, $nameserver, $query, $success, &$errorback, &$retries, $executor) {
39+
if (!$e instanceof TimeoutException) {
40+
$errorback = null;
41+
$deferred->reject($e);
42+
} elseif ($retries <= 0) {
43+
$errorback = null;
44+
$deferred->reject($e = new \RuntimeException(
45+
'DNS query for ' . $query->name . ' failed: too many retries',
3346
0,
34-
$error
47+
$e
48+
));
49+
50+
// avoid garbage references by replacing all closures in call stack.
51+
// what a lovely piece of code!
52+
$r = new \ReflectionProperty('Exception', 'trace');
53+
$r->setAccessible(true);
54+
$trace = $r->getValue($e);
55+
foreach ($trace as &$one) {
56+
foreach ($one['args'] as &$arg) {
57+
if ($arg instanceof \Closure) {
58+
$arg = 'Object(' . \get_class($arg) . ')';
59+
}
60+
}
61+
}
62+
$r->setValue($e, $trace);
63+
} else {
64+
--$retries;
65+
$promise = $executor->query($nameserver, $query)->then(
66+
$success,
67+
$errorback
3568
);
3669
}
37-
return $that->tryQuery($nameserver, $query, $retries-1);
3870
};
3971

40-
return $this->executor
41-
->query($nameserver, $query)
42-
->then(null, $errorback);
72+
$promise = $this->executor->query($nameserver, $query)->then(
73+
$success,
74+
$errorback
75+
);
76+
77+
return $deferred->promise();
4378
}
4479
}

‎tests/Query/RetryExecutorTest.php‎

Lines changed: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -162,6 +162,159 @@ public function queryShouldCancelQueryOnCancel()
162162
$this->assertEquals(1, $cancelled);
163163
}
164164

165+
/**
166+
* @covers React\Dns\Query\RetryExecutor
167+
* @test
168+
*/
169+
public function queryShouldCancelSecondQueryOnCancel()
170+
{
171+
$deferred = new Deferred();
172+
$cancelled = 0;
173+
174+
$executor = $this->createExecutorMock();
175+
$executor
176+
->expects($this->exactly(2))
177+
->method('query')
178+
->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
179+
->will($this->onConsecutiveCalls(
180+
$this->returnValue($deferred->promise()),
181+
$this->returnCallback(function ($domain, $query) use (&$cancelled) {
182+
$deferred = new Deferred(function ($resolve, $reject) use (&$cancelled) {
183+
++$cancelled;
184+
$reject(new CancellationException('Cancelled'));
185+
});
186+
187+
return $deferred->promise();
188+
})
189+
));
190+
191+
$retryExecutor = new RetryExecutor($executor, 2);
192+
193+
$query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
194+
$promise = $retryExecutor->query('8.8.8.8', $query);
195+
196+
$promise->then($this->expectCallableNever(), $this->expectCallableOnce());
197+
198+
// first query will time out after a while and this sends the next query
199+
$deferred->reject(new TimeoutException());
200+
201+
$this->assertEquals(0, $cancelled);
202+
$promise->cancel();
203+
$this->assertEquals(1, $cancelled);
204+
}
205+
206+
/**
207+
* @covers React\Dns\Query\RetryExecutor
208+
* @test
209+
*/
210+
public function queryShouldNotCauseGarbageReferencesOnSuccess()
211+
{
212+
if (class_exists('React\Promise\When')) {
213+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
214+
}
215+
216+
$executor = $this->createExecutorMock();
217+
$executor
218+
->expects($this->once())
219+
->method('query')
220+
->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
221+
->willReturn(Promise\resolve($this->createStandardResponse()));
222+
223+
$retryExecutor = new RetryExecutor($executor, 0);
224+
225+
gc_collect_cycles();
226+
$query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
227+
$retryExecutor->query('8.8.8.8', $query);
228+
229+
$this->assertEquals(0, gc_collect_cycles());
230+
}
231+
232+
/**
233+
* @covers React\Dns\Query\RetryExecutor
234+
* @test
235+
*/
236+
public function queryShouldNotCauseGarbageReferencesOnTimeoutErrors()
237+
{
238+
if (class_exists('React\Promise\When')) {
239+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
240+
}
241+
242+
$executor = $this->createExecutorMock();
243+
$executor
244+
->expects($this->any())
245+
->method('query')
246+
->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
247+
->willReturn(Promise\reject(new TimeoutException("timeout")));
248+
249+
$retryExecutor = new RetryExecutor($executor, 0);
250+
251+
gc_collect_cycles();
252+
$query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
253+
$retryExecutor->query('8.8.8.8', $query);
254+
255+
$this->assertEquals(0, gc_collect_cycles());
256+
}
257+
258+
/**
259+
* @covers React\Dns\Query\RetryExecutor
260+
* @test
261+
*/
262+
public function queryShouldNotCauseGarbageReferencesOnCancellation()
263+
{
264+
if (class_exists('React\Promise\When')) {
265+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
266+
}
267+
268+
$deferred = new Deferred(function () {
269+
throw new \RuntimeException();
270+
});
271+
272+
$executor = $this->createExecutorMock();
273+
$executor
274+
->expects($this->once())
275+
->method('query')
276+
->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
277+
->willReturn($deferred->promise());
278+
279+
$retryExecutor = new RetryExecutor($executor, 0);
280+
281+
gc_collect_cycles();
282+
$query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
283+
$promise = $retryExecutor->query('8.8.8.8', $query);
284+
$promise->cancel();
285+
$promise = null;
286+
287+
$this->assertEquals(0, gc_collect_cycles());
288+
}
289+
290+
/**
291+
* @covers React\Dns\Query\RetryExecutor
292+
* @test
293+
*/
294+
public function queryShouldNotCauseGarbageReferencesOnNonTimeoutErrors()
295+
{
296+
if (class_exists('React\Promise\When')) {
297+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
298+
}
299+
300+
$executor = $this->createExecutorMock();
301+
$executor
302+
->expects($this->once())
303+
->method('query')
304+
->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
305+
->will($this->returnCallback(function ($domain, $query) {
306+
return Promise\reject(new \Exception);
307+
}));
308+
309+
$retryExecutor = new RetryExecutor($executor, 2);
310+
311+
gc_collect_cycles();
312+
$query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
313+
$retryExecutor->query('8.8.8.8', $query);
314+
315+
$this->assertEquals(0, gc_collect_cycles());
316+
}
317+
165318
protected function expectPromiseOnce($return = null)
166319
{
167320
$mock = $this->createPromiseMock();

0 commit comments

Comments
 (0)