Improve promise cancellation for DNS lookup retries

reactphp · Nov 5, 2018 · b5a8542 · b5a8542
1 parent 6f2f761
commit b5a8542
Show file tree

Hide file tree

Showing 2 changed files with 200 additions and 12 deletions.
diff --git a/src/Query/RetryExecutor.php b/src/Query/RetryExecutor.php
@@ -2,6 +2,7 @@
 
 namespace React\Dns\Query;
 
+use React\Promise\CancellablePromiseInterface;
 use React\Promise\Deferred;
 
 class RetryExecutor implements ExecutorInterface
@@ -22,23 +23,57 @@ public function query($nameserver, Query $query)
 
     public function tryQuery($nameserver, Query $query, $retries)
     {
-        $that = $this;
-        $errorback = function ($error) use ($nameserver, $query, $retries, $that) {
-            if (!$error instanceof TimeoutException) {
-                throw $error;
+        $deferred = new Deferred(function () use (&$promise) {
+            if ($promise instanceof CancellablePromiseInterface) {
+                $promise->cancel();
             }
-            if (0 >= $retries) {
-                throw new \RuntimeException(
-                    sprintf("DNS query for %s failed: too many retries", $query->name),
+        });
+
+        $success = function ($value) use ($deferred, &$errorback) {
+            $errorback = null;
+            $deferred->resolve($value);
+        };
+
+        $executor = $this->executor;
+        $errorback = function ($e) use ($deferred, &$promise, $nameserver, $query, $success, &$errorback, &$retries, $executor) {
+            if (!$e instanceof TimeoutException) {
+                $errorback = null;
+                $deferred->reject($e);
+            } elseif ($retries <= 0) {
+                $errorback = null;
+                $deferred->reject($e = new \RuntimeException(
+                    'DNS query for ' . $query->name . ' failed: too many retries',
                     0,
-                    $error
+                    $e
+                ));
+
+                // avoid garbage references by replacing all closures in call stack.
+                // what a lovely piece of code!
+                $r = new \ReflectionProperty('Exception', 'trace');
+                $r->setAccessible(true);
+                $trace = $r->getValue($e);
+                foreach ($trace as &$one) {
+                    foreach ($one['args'] as &$arg) {
+                        if ($arg instanceof \Closure) {
+                            $arg = 'Object(' . \get_class($arg) . ')';
+                        }
+                    }
+                }
+                $r->setValue($e, $trace);
+            } else {
+                --$retries;
+                $promise = $executor->query($nameserver, $query)->then(
+                    $success,
+                    $errorback
                 );
             }
-            return $that->tryQuery($nameserver, $query, $retries-1);
         };
 
-        return $this->executor
-            ->query($nameserver, $query)
-            ->then(null, $errorback);
+        $promise = $this->executor->query($nameserver, $query)->then(
+            $success,
+            $errorback
+        );
+
+        return $deferred->promise();
     }
 }
diff --git a/tests/Query/RetryExecutorTest.php b/tests/Query/RetryExecutorTest.php
@@ -162,6 +162,159 @@ public function queryShouldCancelQueryOnCancel()
         $this->assertEquals(1, $cancelled);
     }
 
+    /**
+     * @covers React\Dns\Query\RetryExecutor
+     * @test
+     */
+    public function queryShouldCancelSecondQueryOnCancel()
+    {
+        $deferred = new Deferred();
+        $cancelled = 0;
+
+        $executor = $this->createExecutorMock();
+        $executor
+            ->expects($this->exactly(2))
+            ->method('query')
+            ->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
+            ->will($this->onConsecutiveCalls(
+                $this->returnValue($deferred->promise()),
+                $this->returnCallback(function ($domain, $query) use (&$cancelled) {
+                    $deferred = new Deferred(function ($resolve, $reject) use (&$cancelled) {
+                        ++$cancelled;
+                        $reject(new CancellationException('Cancelled'));
+                    });
+
+                    return $deferred->promise();
+                })
+        ));
+
+        $retryExecutor = new RetryExecutor($executor, 2);
+
+        $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
+        $promise = $retryExecutor->query('8.8.8.8', $query);
+
+        $promise->then($this->expectCallableNever(), $this->expectCallableOnce());
+
+        // first query will time out after a while and this sends the next query
+        $deferred->reject(new TimeoutException());
+
+        $this->assertEquals(0, $cancelled);
+        $promise->cancel();
+        $this->assertEquals(1, $cancelled);
+    }
+
+    /**
+     * @covers React\Dns\Query\RetryExecutor
+     * @test
+     */
+    public function queryShouldNotCauseGarbageReferencesOnSuccess()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        $executor = $this->createExecutorMock();
+        $executor
+            ->expects($this->once())
+            ->method('query')
+            ->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
+            ->willReturn(Promise\resolve($this->createStandardResponse()));
+
+        $retryExecutor = new RetryExecutor($executor, 0);
+
+        gc_collect_cycles();
+        $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
+        $retryExecutor->query('8.8.8.8', $query);
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
+
+    /**
+     * @covers React\Dns\Query\RetryExecutor
+     * @test
+     */
+    public function queryShouldNotCauseGarbageReferencesOnTimeoutErrors()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        $executor = $this->createExecutorMock();
+        $executor
+            ->expects($this->any())
+            ->method('query')
+            ->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
+            ->willReturn(Promise\reject(new TimeoutException("timeout")));
+
+        $retryExecutor = new RetryExecutor($executor, 0);
+
+        gc_collect_cycles();
+        $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
+        $retryExecutor->query('8.8.8.8', $query);
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
+
+    /**
+     * @covers React\Dns\Query\RetryExecutor
+     * @test
+     */
+    public function queryShouldNotCauseGarbageReferencesOnCancellation()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        $deferred = new Deferred(function () {
+            throw new \RuntimeException();
+        });
+
+        $executor = $this->createExecutorMock();
+        $executor
+            ->expects($this->once())
+            ->method('query')
+            ->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
+            ->willReturn($deferred->promise());
+
+        $retryExecutor = new RetryExecutor($executor, 0);
+
+        gc_collect_cycles();
+        $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
+        $promise = $retryExecutor->query('8.8.8.8', $query);
+        $promise->cancel();
+        $promise = null;
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
+
+    /**
+     * @covers React\Dns\Query\RetryExecutor
+     * @test
+     */
+    public function queryShouldNotCauseGarbageReferencesOnNonTimeoutErrors()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        $executor = $this->createExecutorMock();
+        $executor
+            ->expects($this->once())
+            ->method('query')
+            ->with('8.8.8.8', $this->isInstanceOf('React\Dns\Query\Query'))
+            ->will($this->returnCallback(function ($domain, $query) {
+                return Promise\reject(new \Exception);
+            }));
+
+        $retryExecutor = new RetryExecutor($executor, 2);
+
+        gc_collect_cycles();
+        $query = new Query('igor.io', Message::TYPE_A, Message::CLASS_IN, 1345656451);
+        $retryExecutor->query('8.8.8.8', $query);
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
+
     protected function expectPromiseOnce($return = null)
     {
         $mock = $this->createPromiseMock();