diff --git a/src/Promise.php b/src/Promise.php index 824a46cc..d234b643 100644 --- a/src/Promise.php +++ b/src/Promise.php @@ -14,7 +14,13 @@ final class Promise implements PromiseInterface 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): PromiseInterface @@ -27,15 +33,26 @@ public function then(callable $onFulfilled = null, callable $onRejected = null): return new static($this->resolver($onFulfilled, $onRejected)); } - $this->requiredCancelRequests++; + // This promise has a canceller, so we create a new child promise which + // has a canceller that invokes the parent canceller if all other + // followers are also cancelled. We keep a reference to this promise + // instance for the static canceller function and clear this to avoid + // keeping a cyclic reference between parent and follower. + $parent = $this; + ++$parent->requiredCancelRequests; + + return new static( + $this->resolver($onFulfilled, $onRejected), + static function () use (&$parent) { + --$parent->requiredCancelRequests; - return new static($this->resolver($onFulfilled, $onRejected), function () { - $this->requiredCancelRequests--; + if ($parent->requiredCancelRequests <= 0) { + $parent->cancel(); + } - if ($this->requiredCancelRequests <= 0) { - $this->cancel(); + $parent = null; } - }); + ); } public function done(callable $onFulfilled = null, callable $onRejected = null): void @@ -45,7 +62,7 @@ public function done(callable $onFulfilled = null, callable $onRejected = null): return; } - $this->handlers[] = function (PromiseInterface $promise) use ($onFulfilled, $onRejected) { + $this->handlers[] = static function (PromiseInterface $promise) use ($onFulfilled, $onRejected) { $promise ->done($onFulfilled, $onRejected); }; @@ -53,7 +70,7 @@ public function done(callable $onFulfilled = null, callable $onRejected = null): public function otherwise(callable $onRejected): PromiseInterface { - 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); } @@ -64,11 +81,11 @@ public function otherwise(callable $onRejected): PromiseInterface public function always(callable $onFulfilledOrRejected): PromiseInterface { - 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); }); @@ -113,7 +130,7 @@ public function cancel(): void private function resolver(callable $onFulfilled = null, callable $onRejected = null): callable { return function ($resolve, $reject) use ($onFulfilled, $onRejected) { - $this->handlers[] = function (PromiseInterface $promise) use ($onFulfilled, $onRejected, $resolve, $reject) { + $this->handlers[] = static function (PromiseInterface $promise) use ($onFulfilled, $onRejected, $resolve, $reject) { $promise ->then($onFulfilled, $onRejected) ->done($resolve, $reject); @@ -121,15 +138,6 @@ private function resolver(callable $onFulfilled = null, callable $onRejected = n }; } - private function resolve($value = null): void - { - if (null !== $this->result) { - return; - } - - $this->settle(resolve($value)); - } - private function reject(\Throwable $reason): void { if (null !== $this->result) { @@ -175,8 +183,13 @@ private function unwrap($promise): PromiseInterface return $promise; } - private function call(callable $callback): void + private function call(callable $cb): void { + // 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. @@ -195,16 +208,33 @@ private function call(callable $callback): void if ($args === 0) { $callback(); } else { + // Keep references to this promise instance for the static resolve/reject functions. + // By using static callbacks that are not bound to this instance + // and 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! + $target =& $this; + $callback( - function ($value = null) { - $this->resolve($value); + static function ($value = null) use (&$target) { + if ($target !== null) { + $target->settle(resolve($value)); + $target = null; + } }, - function (\Throwable $reason) { - $this->reject($reason); + static function (\Throwable $reason) use (&$target) { + if ($target !== null) { + $target->reject($reason); + $target = null; + } } ); } } catch (\Throwable $e) { + $target = null; $this->reject($e); } } diff --git a/tests/DeferredTest.php b/tests/DeferredTest.php index 5c86eb83..172cb246 100644 --- a/tests/DeferredTest.php +++ b/tests/DeferredTest.php @@ -19,4 +19,41 @@ public function getPromiseTestAdapter(callable $canceller = null) 'settle' => [$d, 'resolve'], ]); } + + /** @test */ + public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerRejectsWithException() + { + gc_collect_cycles(); + $deferred = new Deferred(function ($resolve, $reject) { + $reject(new \Exception('foo')); + }); + $deferred->promise()->cancel(); + unset($deferred); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldRejectWithoutCreatingGarbageCyclesIfParentCancellerRejectsWithException() + { + gc_collect_cycles(); + $deferred = new Deferred(function ($resolve, $reject) { + $reject(new \Exception('foo')); + }); + $deferred->promise()->then()->cancel(); + unset($deferred); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @test */ + public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerHoldsReferenceAndExplicitlyRejectWithException() + { + gc_collect_cycles(); + $deferred = new Deferred(function () use (&$deferred) { }); + $deferred->reject(new \Exception('foo')); + unset($deferred); + + $this->assertSame(0, gc_collect_cycles()); + } } diff --git a/tests/PromiseTest.php b/tests/PromiseTest.php index 26ebdf9b..6af65ce5 100644 --- a/tests/PromiseTest.php +++ b/tests/PromiseTest.php @@ -47,8 +47,217 @@ public function shouldRejectIfResolverThrowsException() ->then($this->expectCallableNever(), $mock); } + /** @test */ + 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 () { + throw new \Exception('foo'); + }); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + + /** @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 shouldRejectWithoutCreatingGarbageCyclesIfParentCancellerRejectsWithException() + { + gc_collect_cycles(); + $promise = new Promise(function ($resolve, $reject) { }, function ($resolve, $reject) { + $reject(new \Exception('foo')); + }); + $promise->then()->then()->then()->cancel(); + unset($promise); + + $this->assertSame(0, gc_collect_cycles()); + } + /** @test */ public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsException() + { + gc_collect_cycles(); + $promise = new Promise(function ($resolve, $reject) { + throw new \Exception('foo'); + }); + unset($promise); + + $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 + * @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 + * @requires PHP 7 + * @see self::shouldRejectWithoutCreatingGarbageCyclesIfCancellerWithReferenceThrowsException + */ + public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerHoldsReferenceAndResolverThrowsException() + { + gc_collect_cycles(); + $promise = new Promise(function () { + throw new \Exception('foo'); + }, function () use (&$promise) { }); + 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 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() { gc_collect_cycles(); $promise = new Promise(function () {