Merge pull request #115 from clue-labs/static-progress

Improve memory consumption by using static progress callback without binding to promise

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);
+            }
+        };
+    }
 }

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()
     {