diff --git a/src/Promise.php b/src/Promise.php index 8944c3f3..5a39dc68 100644 --- a/src/Promise.php +++ b/src/Promise.php @@ -28,15 +28,44 @@ public function then(callable $onFulfilled = null, callable $onRejected = null, return new static($this->resolver($onFulfilled, $onRejected, $onProgress)); } - $this->requiredCancelRequests++; + // keep a reference to this promise instance for the static canceller function. + // see also parentCancellerFunction() for more details. + $parent = $this; + ++$parent->requiredCancelRequests; + + return new static( + $this->resolver($onFulfilled, $onRejected, $onProgress), + self::parentCancellerFunction($parent) + ); + } - return new static($this->resolver($onFulfilled, $onRejected, $onProgress), function () { - $this->requiredCancelRequests--; + /** + * Creates a static parent canceller 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 cancellation logic + * and still clear this reference after invocation (canceller can only ever + * be called once). This helps avoiding garbage cycles if the parent canceller + * 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 $parent + * @return callable + */ + private static function parentCancellerFunction(self &$parent) + { + return function () use (&$parent) { + --$parent->requiredCancelRequests; - if ($this->requiredCancelRequests <= 0) { - $this->cancel(); + if ($parent->requiredCancelRequests <= 0) { + $parent->cancel(); } - }); + + $parent = null; + }; } public function done(callable $onFulfilled = null, callable $onRejected = null, callable $onProgress = null) diff --git a/tests/PromiseTest.php b/tests/PromiseTest.php index 763536a3..a0c3551d 100644 --- a/tests/PromiseTest.php +++ b/tests/PromiseTest.php @@ -97,6 +97,19 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerRejectsWithEx $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() {