Merge pull request #213 from clue-labs/include-timeout

Include timeout logic to avoid dependency on reactphp/promise-timer
reactphp · Jun 2, 2023 · 4083648 · 4083648
2 parents d449e24 + 5463a48
commit 4083648
Show file tree

Hide file tree

Showing 3 changed files with 121 additions and 20 deletions.
diff --git 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
@@ -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
@@ -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());
     }
 }