Avoid garbage references when resolving rejects on legacy PHP <= 5.6

These garbage references only show up when a DNS lookup rejects with an Exception on legacy PHP <= 5.6. These issues have been fixed upstream in the Promise component for current PHP versions with https://github.com/reactphp/promise/releases/tag/v2.6.0 but this PR explicitly unsets known garbage references to avoid unexpected memory growth on legacy PHP versions. The downstream Socket component implicitly depends on this because its test suite currently fails. This changeset can be considered a new feature in this component which fixes a bug in the test suite of a downstream component.
reactphp · Jul 10, 2019 · 8e8d861 · 8e8d861
1 parent b6778e7
commit 8e8d861
Show file tree

Hide file tree

Showing 4 changed files with 106 additions and 2 deletions.
diff --git a/src/Query/CachingExecutor.php b/src/Query/CachingExecutor.php
@@ -52,10 +52,14 @@ function (Message $message) use ($cache, $id, $that) {
                         }
                     );
                 }
-            )->then($resolve, $reject);
+            )->then($resolve, function ($e) use ($reject, &$pending) {
+                $reject($e);
+                $pending = null;
+            });
         }, function ($_, $reject) use (&$pending, $query) {
             $reject(new \RuntimeException('DNS query for ' . $query->name . ' has been cancelled'));
             $pending->cancel();
+            $pending = null;
         });
     }
 

diff --git a/src/Query/CoopExecutor.php b/src/Query/CoopExecutor.php
@@ -75,10 +75,11 @@ public function query($nameserver, Query $query)
         $counts =& $this->counts;
         return new Promise(function ($resolve, $reject) use ($promise) {
             $promise->then($resolve, $reject);
-        }, function () use ($promise, $key, $query, &$pending, &$counts) {
+        }, function () use (&$promise, $key, $query, &$pending, &$counts) {
             if (--$counts[$key] < 1) {
                 unset($pending[$key], $counts[$key]);
                 $promise->cancel();
+                $promise = null;
             }
             throw new \RuntimeException('DNS query for ' . $query->name . ' has been cancelled');
         });

diff --git a/tests/FunctionalResolverTest.php b/tests/FunctionalResolverTest.php
@@ -98,4 +98,74 @@ public function testInvalidResolverDoesNotResolveGoogle()
         $promise = $this->resolver->resolve('google.com');
         $promise->then($this->expectCallableNever(), $this->expectCallableOnce());
     }
+
+    public function testResolveShouldNotCauseGarbageReferencesWhenUsingInvalidNameserver()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        $factory = new Factory();
+        $this->resolver = $factory->create('255.255.255.255', $this->loop);
+
+        gc_collect_cycles();
+
+        $promise = $this->resolver->resolve('google.com');
+        unset($promise);
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
+
+    public function testResolveCachedShouldNotCauseGarbageReferencesWhenUsingInvalidNameserver()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        $factory = new Factory();
+        $this->resolver = $factory->createCached('255.255.255.255', $this->loop);
+
+        gc_collect_cycles();
+
+        $promise = $this->resolver->resolve('google.com');
+        unset($promise);
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
+
+    public function testCancelResolveShouldNotCauseGarbageReferences()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        $factory = new Factory();
+        $this->resolver = $factory->create('127.0.0.1', $this->loop);
+
+        gc_collect_cycles();
+
+        $promise = $this->resolver->resolve('google.com');
+        $promise->cancel();
+        $promise = null;
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
+
+    public function testCancelResolveCachedShouldNotCauseGarbageReferences()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        $factory = new Factory();
+        $this->resolver = $factory->createCached('127.0.0.1', $this->loop);
+
+        gc_collect_cycles();
+
+        $promise = $this->resolver->resolve('google.com');
+        $promise->cancel();
+        $promise = null;
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
 }
diff --git a/tests/Query/CoopExecutorTest.php b/tests/Query/CoopExecutorTest.php
@@ -205,4 +205,33 @@ public function testQueryTwiceWillQueryBaseExecutorTwiceIfFirstQueryHasAlreadyBe
 
         $promise2->then(null, $this->expectCallableNever());
     }
+
+    /**
+     * @covers React\Dns\Query\RetryExecutor
+     * @test
+     */
+    public function cancelQueryShouldNotCauseGarbageReferences()
+    {
+        if (class_exists('React\Promise\When')) {
+            $this->markTestSkipped('Not supported on legacy Promise v1 API');
+        }
+
+        $deferred = new Deferred(function () {
+            throw new \RuntimeException();
+        });
+
+        $base = $this->getMockBuilder('React\Dns\Query\ExecutorInterface')->getMock();
+        $base->expects($this->once())->method('query')->willReturn($deferred->promise());
+        $connector = new CoopExecutor($base);
+
+        gc_collect_cycles();
+
+        $query = new Query('reactphp.org', Message::TYPE_A, Message::CLASS_IN);
+
+        $promise = $connector->query('8.8.8.8', $query);
+        $promise->cancel();
+        $promise = null;
+
+        $this->assertEquals(0, gc_collect_cycles());
+    }
 }