diff --git a/composer.json b/composer.json index 305f1aa2..181f2d88 100644 --- a/composer.json +++ b/composer.json @@ -29,12 +29,12 @@ "php": ">=5.3.0", "react/cache": "^1.0 || ^0.6 || ^0.5", "react/event-loop": "^1.2", - "react/promise": "^3.0 || ^2.7 || ^1.2.1", - "react/promise-timer": "^1.9" + "react/promise": "^3.0 || ^2.7 || ^1.2.1" }, "require-dev": { "phpunit/phpunit": "^9.5 || ^4.8.35", - "react/async": "^4 || ^3 || ^2" + "react/async": "^4 || ^3 || ^2", + "react/promise-timer": "^1.9" }, "autoload": { "psr-4": { diff --git a/src/Query/TimeoutExecutor.php b/src/Query/TimeoutExecutor.php index 15c8c22a..06c51b15 100644 --- a/src/Query/TimeoutExecutor.php +++ b/src/Query/TimeoutExecutor.php @@ -4,7 +4,7 @@ use React\EventLoop\Loop; use React\EventLoop\LoopInterface; -use React\Promise\Timer; +use React\Promise\Promise; final class TimeoutExecutor implements ExecutorInterface { @@ -21,11 +21,49 @@ public function __construct(ExecutorInterface $executor, $timeout, LoopInterface public function query(Query $query) { - return Timer\timeout($this->executor->query($query), $this->timeout, $this->loop)->then(null, function ($e) use ($query) { - if ($e instanceof Timer\TimeoutException) { - $e = new TimeoutException(sprintf("DNS query for %s timed out", $query->describe()), 0, $e); + $promise = $this->executor->query($query); + + $loop = $this->loop; + $time = $this->timeout; + return new Promise(function ($resolve, $reject) use ($loop, $time, $promise, $query) { + $timer = null; + $promise = $promise->then(function ($v) use (&$timer, $loop, $resolve) { + if ($timer) { + $loop->cancelTimer($timer); + } + $timer = false; + $resolve($v); + }, function ($v) use (&$timer, $loop, $reject) { + if ($timer) { + $loop->cancelTimer($timer); + } + $timer = false; + $reject($v); + }); + + // promise already resolved => no need to start timer + if ($timer === false) { + return; } - throw $e; + + // start timeout timer which will cancel the pending promise + $timer = $loop->addTimer($time, function () use ($time, &$promise, $reject, $query) { + $reject(new TimeoutException( + 'DNS query for ' . $query->describe() . ' timed out' + )); + + // Cancel pending query to clean up any underlying resources and references. + // Avoid garbage references in call stack by passing pending promise by reference. + assert(\method_exists($promise, 'cancel')); + $promise->cancel(); + $promise = null; + }); + }, function () use (&$promise) { + // Cancelling this promise will cancel the pending query, thus triggering the rejection logic above. + // Avoid garbage references in call stack by passing pending promise by reference. + assert(\method_exists($promise, 'cancel')); + $promise->cancel(); + $promise = null; }); } } diff --git a/tests/Query/TimeoutExecutorTest.php b/tests/Query/TimeoutExecutorTest.php index bab6d616..b7857783 100644 --- a/tests/Query/TimeoutExecutorTest.php +++ b/tests/Query/TimeoutExecutorTest.php @@ -15,6 +15,7 @@ class TimeoutExecutorTest extends TestCase { private $wrapped; private $executor; + private $loop; /** * @before @@ -23,7 +24,9 @@ public function setUpExecutor() { $this->wrapped = $this->getMockBuilder('React\Dns\Query\ExecutorInterface')->getMock(); - $this->executor = new TimeoutExecutor($this->wrapped, 5.0); + $this->loop = $this->getMockBuilder('React\EventLoop\LoopInterface')->getMock(); + + $this->executor = new TimeoutExecutor($this->wrapped, 5.0, $this->loop); } public function testCtorWithoutLoopShouldAssignDefaultLoop() @@ -39,6 +42,10 @@ public function testCtorWithoutLoopShouldAssignDefaultLoop() public function testCancellingPromiseWillCancelWrapped() { + $timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock(); + $this->loop->expects($this->once())->method('addTimer')->with(5.0, $this->anything())->willReturn($timer); + $this->loop->expects($this->once())->method('cancelTimer')->with($timer); + $cancelled = 0; $this->wrapped @@ -63,8 +70,11 @@ public function testCancellingPromiseWillCancelWrapped() $promise->then($this->expectCallableNever(), $this->expectCallableOnce()); } - public function testResolvesPromiseWhenWrappedResolves() + public function testResolvesPromiseWithoutStartingTimerWhenWrappedReturnsResolvedPromise() { + $this->loop->expects($this->never())->method('addTimer'); + $this->loop->expects($this->never())->method('cancelTimer'); + $this->wrapped ->expects($this->once()) ->method('query') @@ -76,8 +86,31 @@ public function testResolvesPromiseWhenWrappedResolves() $promise->then($this->expectCallableOnce(), $this->expectCallableNever()); } - public function testRejectsPromiseWhenWrappedRejects() + public function testResolvesPromiseAfterCancellingTimerWhenWrappedReturnsPendingPromiseThatResolves() + { + $timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock(); + $this->loop->expects($this->once())->method('addTimer')->with(5.0, $this->anything())->willReturn($timer); + $this->loop->expects($this->once())->method('cancelTimer')->with($timer); + + $deferred = new Deferred(); + $this->wrapped + ->expects($this->once()) + ->method('query') + ->willReturn($deferred->promise()); + + $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN); + $promise = $this->executor->query($query); + + $deferred->resolve('0.0.0.0'); + + $promise->then($this->expectCallableOnce(), $this->expectCallableNever()); + } + + public function testRejectsPromiseWithoutStartingTimerWhenWrappedReturnsRejectedPromise() { + $this->loop->expects($this->never())->method('addTimer'); + $this->loop->expects($this->never())->method('cancelTimer'); + $this->wrapped ->expects($this->once()) ->method('query') @@ -89,9 +122,35 @@ public function testRejectsPromiseWhenWrappedRejects() $promise->then($this->expectCallableNever(), $this->expectCallableOnceWith(new \RuntimeException())); } - public function testWrappedWillBeCancelledOnTimeout() + public function testRejectsPromiseAfterCancellingTimerWhenWrappedReturnsPendingPromiseThatRejects() + { + $timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock(); + $this->loop->expects($this->once())->method('addTimer')->with(5.0, $this->anything())->willReturn($timer); + $this->loop->expects($this->once())->method('cancelTimer')->with($timer); + + $deferred = new Deferred(); + $this->wrapped + ->expects($this->once()) + ->method('query') + ->willReturn($deferred->promise()); + + $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN); + $promise = $this->executor->query($query); + + $deferred->reject(new \RuntimeException()); + + $promise->then($this->expectCallableNever(), $this->expectCallableOnceWith(new \RuntimeException())); + } + + public function testRejectsPromiseAndCancelsPendingQueryWhenTimeoutTriggers() { - $this->executor = new TimeoutExecutor($this->wrapped, 0); + $timerCallback = null; + $timer = $this->getMockBuilder('React\EventLoop\TimerInterface')->getMock(); + $this->loop->expects($this->once())->method('addTimer')->with(5.0, $this->callback(function ($callback) use (&$timerCallback) { + $timerCallback = $callback; + return true; + }))->willReturn($timer); + $this->loop->expects($this->once())->method('cancelTimer')->with($timer); $cancelled = 0; @@ -112,14 +171,18 @@ public function testWrappedWillBeCancelledOnTimeout() $this->assertEquals(0, $cancelled); - try { - \React\Async\await(\React\Promise\Timer\sleep(0)); - \React\Async\await($promise); - $this->fail(); - } catch (TimeoutException $exception) { - $this->assertEquals('DNS query for igor.io (A) timed out' , $exception->getMessage()); - } + $this->assertNotNull($timerCallback); + $timerCallback(); $this->assertEquals(1, $cancelled); + + $exception = null; + $promise->then(null, function ($reason) use (&$exception) { + $exception = $reason; + }); + + assert($exception instanceof TimeoutException); + $this->assertInstanceOf('React\Dns\Query\TimeoutException', $exception); + $this->assertEquals('DNS query for igor.io (A) timed out' , $exception->getMessage()); } }