diff --git a/src/Promise.php b/src/Promise.php index 1ccb8c50..74905358 100644 --- a/src/Promise.php +++ b/src/Promise.php @@ -164,17 +164,6 @@ private function reject($reason = null) $this->settle(reject($reason)); } - private function notify($update = null) - { - if (null !== $this->result) { - return; - } - - foreach ($this->progressHandlers as $handler) { - $handler($update); - } - } - private function settle(ExtendedPromiseInterface $promise) { $promise = $this->unwrap($promise); @@ -246,9 +235,7 @@ function ($value = null) { function ($reason = null) { $this->reject($reason); }, - function ($update = null) { - $this->notify($update); - } + self::notifier($this->progressHandlers) ); } } catch (\Throwable $e) { @@ -257,4 +244,28 @@ function ($update = null) { $this->reject($e); } } + + /** + * Creates a static progress 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 its progress + * handlers by reference, we can still execute them when requested 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 array $progressHandlers + * @return callable + */ + private static function notifier(&$progressHandlers) + { + return function ($update = null) use (&$progressHandlers) { + foreach ($progressHandlers as $handler) { + $handler($update); + } + }; + } } diff --git a/tests/PromiseTest.php b/tests/PromiseTest.php index 60b0c0d0..b12b6bc2 100644 --- a/tests/PromiseTest.php +++ b/tests/PromiseTest.php @@ -60,6 +60,39 @@ 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() + { + gc_collect_cycles(); + $promise = new Promise(function ($resolve, $reject) { + $resolve = $reject = null; + throw new \Exception('foo'); + }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldIgnoreNotifyAfterReject() + { + $promise = new Promise(function () { }, function ($resolve, $reject, $notify) { + $reject(new \Exception('foo')); + $notify(42); + }); + + $promise->then(null, null, $this->expectCallableNever()); + $promise->cancel(); + } + /** @test */ public function shouldFulfillIfFullfilledWithSimplePromise() {