Merge pull request #116 from clue-labs/static-settle

Improve memory consumption by using static resolve and reject callback without binding to promise

Author: jsor

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

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