From 1482a9ecbf0f7b817eddd1929445e5b7a3d231d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20L=C3=BCck?= Date: Fri, 27 Apr 2018 17:40:17 +0200 Subject: [PATCH] Use static resolve and reject callback without binding to promise --- src/Promise.php | 77 +++++++++++++++++++++++++++++++++---------- tests/PromiseTest.php | 52 ++++++++++++++++++++++------- 2 files changed, 100 insertions(+), 29 deletions(-) diff --git a/src/Promise.php b/src/Promise.php index 74905358..8944c3f3 100644 --- a/src/Promise.php +++ b/src/Promise.php @@ -146,15 +146,6 @@ private function resolver(callable $onFulfilled = null, callable $onRejected = n }; } - private function resolve($value = null) - { - if (null !== $this->result) { - return; - } - - $this->settle(resolve($value)); - } - private function reject($reason = null) { if (null !== $this->result) { @@ -228,23 +219,75 @@ private function call(callable $callback) if ($args === 0) { $callback(); } else { + // keep a reference to this promise instance for the static resolve/reject functions. + // see also resolveFunction() and rejectFunction() for more details. + $target =& $this; + $callback( - function ($value = null) { - $this->resolve($value); - }, - function ($reason = null) { - $this->reject($reason); - }, - self::notifier($this->progressHandlers) + self::resolveFunction($target), + self::rejectFunction($target), + self::notifyFunction($this->progressHandlers) ); } } catch (\Throwable $e) { + $target = null; $this->reject($e); } catch (\Exception $e) { + $target = null; $this->reject($e); } } + /** + * Creates a static resolver callback that is not bound to a promise instance. + * + * Moving the closure creation to a static method allows us to create a + * callback that is not bound to a promise instance. By passing the target + * promise instance by reference, we can still execute its resolving logic + * and still clear this reference when settling the promise. This helps + * avoiding garbage cycles if any callback creates an Exception. + * + * These assumptions are covered by the test suite, so if you ever feel like + * refactoring this, go ahead, any alternative suggestions are welcome! + * + * @param Promise $target + * @return callable + */ + private static function resolveFunction(self &$target) + { + return function ($value = null) use (&$target) { + if ($target !== null) { + $target->settle(resolve($value)); + $target = null; + } + }; + } + + /** + * Creates a static rejection callback that is not bound to a promise instance. + * + * Moving the closure creation to a static method allows us to create a + * callback that is not bound to a promise instance. By passing the target + * promise instance by reference, we can still execute its rejection logic + * and still clear this reference when settling the promise. This helps + * avoiding garbage cycles if any callback creates an Exception. + * + * These assumptions are covered by the test suite, so if you ever feel like + * refactoring this, go ahead, any alternative suggestions are welcome! + * + * @param Promise $target + * @return callable + */ + private static function rejectFunction(self &$target) + { + return function ($reason = null) use (&$target) { + if ($target !== null) { + $target->reject($reason); + $target = null; + } + }; + } + /** * Creates a static progress callback that is not bound to a promise instance. * @@ -260,7 +303,7 @@ function ($reason = null) { * @param array $progressHandlers * @return callable */ - private static function notifier(&$progressHandlers) + private static function notifyFunction(&$progressHandlers) { return function ($update = null) use (&$progressHandlers) { foreach ($progressHandlers as $handler) { diff --git a/tests/PromiseTest.php b/tests/PromiseTest.php index b12b6bc2..763536a3 100644 --- a/tests/PromiseTest.php +++ b/tests/PromiseTest.php @@ -49,7 +49,19 @@ public function shouldRejectIfResolverThrowsException() } /** @test */ - public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsException() + public function shouldResolveWithoutCreatingGarbageCyclesIfResolverResolvesWithException() + { + gc_collect_cycles(); + $promise = new Promise(function ($resolve) { + $resolve(new \Exception('foo')); + }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsExceptionWithoutResolver() { gc_collect_cycles(); $promise = new Promise(function () { @@ -60,20 +72,36 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsExceptio $this->assertSame(0, gc_collect_cycles()); } - /** - * test that checks number of garbage cycles after throwing from a resolver - * that has its arguments explicitly set to null (reassigned arguments only - * show up in the stack trace in PHP 7, so we can't test this on legacy PHP) - * - * @test - * @requires PHP 7 - * @link https://3v4l.org/OiDr4 - */ - public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsExceptionWithResolveAndRejectUnset() + /** @test */ + public function shouldRejectWithoutCreatingGarbageCyclesIfResolverRejectsWithException() + { + gc_collect_cycles(); + $promise = new Promise(function ($resolve, $reject) { + $reject(new \Exception('foo')); + }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerRejectsWithException() + { + gc_collect_cycles(); + $promise = new Promise(function ($resolve, $reject) { }, function ($resolve, $reject) { + $reject(new \Exception('foo')); + }); + $promise->cancel(); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsException() { gc_collect_cycles(); $promise = new Promise(function ($resolve, $reject) { - $resolve = $reject = null; throw new \Exception('foo'); }); unset($promise);