From 72087fdb3686cf99b4404b26074f6a2aa0ff11b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Mon, 11 Jun 2018 19:29:33 +0200 Subject: [PATCH 1/2] Use static internal callbacks without binding to parent promise --- src/Promise.php | 12 ++++---- tests/PromiseTest.php | 66 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 72 insertions(+), 6 deletions(-) diff --git a/src/Promise.php b/src/Promise.php index 5ac74de6..77819ac4 100644 --- a/src/Promise.php +++ b/src/Promise.php @@ -61,7 +61,7 @@ public function done(callable $onFulfilled = null, callable $onRejected = null, return $this->result->done($onFulfilled, $onRejected, $onProgress); } - $this->handlers[] = function (ExtendedPromiseInterface $promise) use ($onFulfilled, $onRejected) { + $this->handlers[] = static function (ExtendedPromiseInterface $promise) use ($onFulfilled, $onRejected) { $promise ->done($onFulfilled, $onRejected); }; @@ -73,7 +73,7 @@ public function done(callable $onFulfilled = null, callable $onRejected = null, public function otherwise(callable $onRejected) { - return $this->then(null, function ($reason) use ($onRejected) { + return $this->then(null, static function ($reason) use ($onRejected) { if (!_checkTypehint($onRejected, $reason)) { return new RejectedPromise($reason); } @@ -84,11 +84,11 @@ public function otherwise(callable $onRejected) public function always(callable $onFulfilledOrRejected) { - return $this->then(function ($value) use ($onFulfilledOrRejected) { + return $this->then(static function ($value) use ($onFulfilledOrRejected) { return resolve($onFulfilledOrRejected())->then(function () use ($value) { return $value; }); - }, function ($reason) use ($onFulfilledOrRejected) { + }, static function ($reason) use ($onFulfilledOrRejected) { return resolve($onFulfilledOrRejected())->then(function () use ($reason) { return new RejectedPromise($reason); }); @@ -116,7 +116,7 @@ private function resolver(callable $onFulfilled = null, callable $onRejected = n { return function ($resolve, $reject, $notify) use ($onFulfilled, $onRejected, $onProgress) { if ($onProgress) { - $progressHandler = function ($update) use ($notify, $onProgress) { + $progressHandler = static function ($update) use ($notify, $onProgress) { try { $notify($onProgress($update)); } catch (\Throwable $e) { @@ -129,7 +129,7 @@ private function resolver(callable $onFulfilled = null, callable $onRejected = n $progressHandler = $notify; } - $this->handlers[] = function (ExtendedPromiseInterface $promise) use ($onFulfilled, $onRejected, $resolve, $reject, $progressHandler) { + $this->handlers[] = static function (ExtendedPromiseInterface $promise) use ($onFulfilled, $onRejected, $resolve, $reject, $progressHandler) { $promise ->then($onFulfilled, $onRejected) ->done($resolve, $reject, $progressHandler); diff --git a/tests/PromiseTest.php b/tests/PromiseTest.php index 8ca46150..344b4114 100644 --- a/tests/PromiseTest.php +++ b/tests/PromiseTest.php @@ -191,6 +191,72 @@ public function shouldIgnoreNotifyAfterReject() $promise->cancel(); } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPromise() + { + gc_collect_cycles(); + $promise = new Promise(function () { }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPromiseWithThenFollowers() + { + gc_collect_cycles(); + $promise = new Promise(function () { }); + $promise->then()->then()->then(); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPromiseWithDoneFollowers() + { + gc_collect_cycles(); + $promise = new Promise(function () { }); + $promise->done(); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPromiseWithOtherwiseFollowers() + { + gc_collect_cycles(); + $promise = new Promise(function () { }); + $promise->otherwise(function () { }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPromiseWithAlwaysFollowers() + { + gc_collect_cycles(); + $promise = new Promise(function () { }); + $promise->always(function () { }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingPromiseWithProgressFollowers() + { + gc_collect_cycles(); + $promise = new Promise(function () { }); + $promise->then(null, null, function () { }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + /** @test */ public function shouldFulfillIfFullfilledWithSimplePromise() { From ef228a93f9839ddb31a6199ce49138cb966ca3bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Mon, 11 Jun 2018 19:46:41 +0200 Subject: [PATCH 2/2] Additional tests to avoid future garbage cycle regressions --- tests/DeferredTest.php | 33 +++++++++++++++++++++++++++++++++ tests/FulfilledPromiseTest.php | 26 ++++++++++++++++++++++++++ tests/RejectedPromiseTest.php | 26 ++++++++++++++++++++++++++ 3 files changed, 85 insertions(+) diff --git a/tests/DeferredTest.php b/tests/DeferredTest.php index de52c89d..8ee40b8a 100644 --- a/tests/DeferredTest.php +++ b/tests/DeferredTest.php @@ -76,4 +76,37 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerHoldsReferenc $this->assertSame(0, gc_collect_cycles()); } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingDeferred() + { + gc_collect_cycles(); + $deferred = new Deferred(); + $deferred->promise(); + unset($deferred); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingDeferredWithUnusedCanceller() + { + gc_collect_cycles(); + $deferred = new Deferred(function () { }); + $deferred->promise(); + unset($deferred); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToPendingDeferredWithNoopCanceller() + { + gc_collect_cycles(); + $deferred = new Deferred(function () { }); + $deferred->promise()->cancel(); + unset($deferred); + + $this->assertSame(0, gc_collect_cycles()); + } } diff --git a/tests/FulfilledPromiseTest.php b/tests/FulfilledPromiseTest.php index 97fc8f6c..f5a2da80 100644 --- a/tests/FulfilledPromiseTest.php +++ b/tests/FulfilledPromiseTest.php @@ -47,4 +47,30 @@ public function shouldThrowExceptionIfConstructedWithAPromise() return new FulfilledPromise(new FulfilledPromise()); } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToFulfilledPromiseWithAlwaysFollowers() + { + gc_collect_cycles(); + $promise = new FulfilledPromise(1); + $promise->always(function () { + throw new \RuntimeException(); + }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToFulfilledPromiseWithThenFollowers() + { + gc_collect_cycles(); + $promise = new FulfilledPromise(1); + $promise = $promise->then(function () { + throw new \RuntimeException(); + }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } } diff --git a/tests/RejectedPromiseTest.php b/tests/RejectedPromiseTest.php index c886b009..825f56cd 100644 --- a/tests/RejectedPromiseTest.php +++ b/tests/RejectedPromiseTest.php @@ -47,4 +47,30 @@ public function shouldThrowExceptionIfConstructedWithAPromise() return new RejectedPromise(new RejectedPromise()); } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToRejectedPromiseWithAlwaysFollowers() + { + gc_collect_cycles(); + $promise = new RejectedPromise(1); + $promise->always(function () { + throw new \RuntimeException(); + }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldNotLeaveGarbageCyclesWhenRemovingLastReferenceToRejectedPromiseWithThenFollowers() + { + gc_collect_cycles(); + $promise = new RejectedPromise(1); + $promise = $promise->then(null, function () { + throw new \RuntimeException(); + }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } }