From 24923df6362b339117ca65352560ff1657fdb10d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 4 May 2018 16:16:47 +0200 Subject: [PATCH 1/2] Avoid garbage reference by hiding canceller from call stack on PHP 7+ --- src/Promise.php | 8 +++++++- tests/PromiseTest.php | 26 ++++++++++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/src/Promise.php b/src/Promise.php index 8944c3f3..f45ffb88 100644 --- a/src/Promise.php +++ b/src/Promise.php @@ -15,6 +15,7 @@ class Promise implements ExtendedPromiseInterface, CancellablePromiseInterface public function __construct(callable $resolver, callable $canceller = null) { $this->canceller = $canceller; + $this->call($resolver); } @@ -199,8 +200,13 @@ private function extract($promise) return $promise; } - private function call(callable $callback) + private function call(callable $cb) { + // Explicitly overwrite argument with null value. This ensure that this + // argument does not show up in the stack trace in PHP 7+ only. + $callback = $cb; + $cb = null; + // Use reflection to inspect number of arguments expected by this callback. // We did some careful benchmarking here: Using reflection to avoid unneeded // function arguments is actually faster than blindly passing them. diff --git a/tests/PromiseTest.php b/tests/PromiseTest.php index 763536a3..7ba945b3 100644 --- a/tests/PromiseTest.php +++ b/tests/PromiseTest.php @@ -109,6 +109,32 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsExceptio $this->assertSame(0, gc_collect_cycles()); } + /** + * Test that checks number of garbage cycles after throwing from a canceller + * that explicitly uses a reference to the promise. This is rather synthetic, + * actual use cases often have implicit (hidden) references which ought not + * to be stored in the stack trace. + * + * Reassigned arguments only show up in the stack trace in PHP 7, so we can't + * avoid this on legacy PHP. As an alternative, consider explicitly unsetting + * any references before throwing. + * + * @test + * @requires PHP 7 + */ + public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerWithReferenceThrowsException() + { + gc_collect_cycles(); + + $promise = new Promise(function () {}, function () use (&$promise) { + throw new \Exception('foo'); + }); + $promise->cancel(); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + /** @test */ public function shouldIgnoreNotifyAfterReject() { From c00b39f484bcad8231a4a5ebb198d407155d84c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 4 May 2018 16:32:53 +0200 Subject: [PATCH 2/2] Avoid garbage reference by hiding resolver from call stack on PHP 7+ --- src/Promise.php | 7 ++++++- tests/PromiseTest.php | 18 ++++++++++++++++++ 2 files changed, 24 insertions(+), 1 deletion(-) diff --git a/src/Promise.php b/src/Promise.php index f45ffb88..915ed259 100644 --- a/src/Promise.php +++ b/src/Promise.php @@ -16,7 +16,12 @@ public function __construct(callable $resolver, callable $canceller = null) { $this->canceller = $canceller; - $this->call($resolver); + // Explicitly overwrite arguments with null values before invoking + // resolver function. This ensure that these arguments do not show up + // in the stack trace in PHP 7+ only. + $cb = $resolver; + $resolver = $canceller = null; + $this->call($cb); } public function then(callable $onFulfilled = null, callable $onRejected = null, callable $onProgress = null) diff --git a/tests/PromiseTest.php b/tests/PromiseTest.php index 7ba945b3..31fbbab4 100644 --- a/tests/PromiseTest.php +++ b/tests/PromiseTest.php @@ -135,6 +135,24 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerWithReference $this->assertSame(0, gc_collect_cycles()); } + /** + * @test + * @requires PHP 7 + * @see self::shouldRejectWithoutCreatingGarbageCyclesIfCancellerWithReferenceThrowsException + */ + public function shouldRejectWithoutCreatingGarbageCyclesIfResolverWithReferenceThrowsException() + { + gc_collect_cycles(); + + $promise = new Promise(function () use (&$promise) { + throw new \Exception('foo'); + }); + + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + /** @test */ public function shouldIgnoreNotifyAfterReject() {