Skip to content

Commit e6cb8d8

Browse files
committed
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.
1 parent b6778e7 commit e6cb8d8

File tree

4 files changed

+102
-2
lines changed

4 files changed

+102
-2
lines changed

src/Query/CachingExecutor.php

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,14 @@ function (Message $message) use ($cache, $id, $that) {
5252
}
5353
);
5454
}
55-
)->then($resolve, $reject);
55+
)->then($resolve, function ($e) use ($reject, &$pending) {
56+
$reject($e);
57+
$pending = null;
58+
});
5659
}, function ($_, $reject) use (&$pending, $query) {
5760
$reject(new \RuntimeException('DNS query for ' . $query->name . ' has been cancelled'));
5861
$pending->cancel();
62+
$pending = null;
5963
});
6064
}
6165

src/Query/CoopExecutor.php

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,10 +75,11 @@ public function query($nameserver, Query $query)
7575
$counts =& $this->counts;
7676
return new Promise(function ($resolve, $reject) use ($promise) {
7777
$promise->then($resolve, $reject);
78-
}, function () use ($promise, $key, $query, &$pending, &$counts) {
78+
}, function () use (&$promise, $key, $query, &$pending, &$counts) {
7979
if (--$counts[$key] < 1) {
8080
unset($pending[$key], $counts[$key]);
8181
$promise->cancel();
82+
$promise = null;
8283
}
8384
throw new \RuntimeException('DNS query for ' . $query->name . ' has been cancelled');
8485
});

tests/FunctionalResolverTest.php

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,4 +98,74 @@ public function testInvalidResolverDoesNotResolveGoogle()
9898
$promise = $this->resolver->resolve('google.com');
9999
$promise->then($this->expectCallableNever(), $this->expectCallableOnce());
100100
}
101+
102+
public function testResolveShouldNotCauseGarbageReferencesWhenUsingInvalidNameserver()
103+
{
104+
if (class_exists('React\Promise\When')) {
105+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
106+
}
107+
108+
$factory = new Factory();
109+
$this->resolver = $factory->create('255.255.255.255', $this->loop);
110+
111+
gc_collect_cycles();
112+
113+
$promise = $this->resolver->resolve('google.com');
114+
unset($promise);
115+
116+
$this->assertEquals(0, gc_collect_cycles());
117+
}
118+
119+
public function testResolveCachedShouldNotCauseGarbageReferencesWhenUsingInvalidNameserver()
120+
{
121+
if (class_exists('React\Promise\When')) {
122+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
123+
}
124+
125+
$factory = new Factory();
126+
$this->resolver = $factory->createCached('255.255.255.255', $this->loop);
127+
128+
gc_collect_cycles();
129+
130+
$promise = $this->resolver->resolve('google.com');
131+
unset($promise);
132+
133+
$this->assertEquals(0, gc_collect_cycles());
134+
}
135+
136+
public function testCancelResolveShouldNotCauseGarbageReferences()
137+
{
138+
if (class_exists('React\Promise\When')) {
139+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
140+
}
141+
142+
$factory = new Factory();
143+
$this->resolver = $factory->create('127.0.0.1', $this->loop);
144+
145+
gc_collect_cycles();
146+
147+
$promise = $this->resolver->resolve('google.com');
148+
$promise->cancel();
149+
$promise = null;
150+
151+
$this->assertEquals(0, gc_collect_cycles());
152+
}
153+
154+
public function testCancelResolveCachedShouldNotCauseGarbageReferences()
155+
{
156+
if (class_exists('React\Promise\When')) {
157+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
158+
}
159+
160+
$factory = new Factory();
161+
$this->resolver = $factory->createCached('127.0.0.1', $this->loop);
162+
163+
gc_collect_cycles();
164+
165+
$promise = $this->resolver->resolve('google.com');
166+
$promise->cancel();
167+
$promise = null;
168+
169+
$this->assertEquals(0, gc_collect_cycles());
170+
}
101171
}

tests/Query/CoopExecutorTest.php

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -205,4 +205,29 @@ public function testQueryTwiceWillQueryBaseExecutorTwiceIfFirstQueryHasAlreadyBe
205205

206206
$promise2->then(null, $this->expectCallableNever());
207207
}
208+
209+
public function testCancelQueryShouldNotCauseGarbageReferences()
210+
{
211+
if (class_exists('React\Promise\When')) {
212+
$this->markTestSkipped('Not supported on legacy Promise v1 API');
213+
}
214+
215+
$deferred = new Deferred(function () {
216+
throw new \RuntimeException();
217+
});
218+
219+
$base = $this->getMockBuilder('React\Dns\Query\ExecutorInterface')->getMock();
220+
$base->expects($this->once())->method('query')->willReturn($deferred->promise());
221+
$connector = new CoopExecutor($base);
222+
223+
gc_collect_cycles();
224+
225+
$query = new Query('reactphp.org', Message::TYPE_A, Message::CLASS_IN);
226+
227+
$promise = $connector->query('8.8.8.8', $query);
228+
$promise->cancel();
229+
$promise = null;
230+
231+
$this->assertEquals(0, gc_collect_cycles());
232+
}
208233
}

0 commit comments

Comments
 (0)