From 10dda04129a27cd497758a30451e226a317a0291 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Christian=20L=C3=BCck?= <christian@lueck.tv>
Date: Tue, 24 Apr 2018 14:00:45 +0200
Subject: [PATCH] Only pass args to resolver and canceller if callback requires
 them

---
 src/Promise.php                       | 40 +++++++++++++++++++--------
 tests/PromiseTest.php                 | 12 ++++++++
 tests/PromiseTest/CancelTestTrait.php | 27 ++++++++++++++----
 3 files changed, 62 insertions(+), 17 deletions(-)

diff --git a/src/Promise.php b/src/Promise.php
index 889926df..1ccb8c50 100644
--- a/src/Promise.php
+++ b/src/Promise.php
@@ -221,18 +221,36 @@ private function extract($promise)
 
     private function call(callable $callback)
     {
+        // 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.
+        // Also, this helps avoiding unnecessary function arguments in the call stack
+        // if the callback creates an Exception (creating garbage cycles).
+        if (is_array($callback)) {
+            $ref = new \ReflectionMethod($callback[0], $callback[1]);
+        } elseif (is_object($callback) && !$callback instanceof \Closure) {
+            $ref = new \ReflectionMethod($callback, '__invoke');
+        } else {
+            $ref = new \ReflectionFunction($callback);
+        }
+        $args = $ref->getNumberOfParameters();
+
         try {
-            $callback(
-                function ($value = null) {
-                    $this->resolve($value);
-                },
-                function ($reason = null) {
-                    $this->reject($reason);
-                },
-                function ($update = null) {
-                    $this->notify($update);
-                }
-            );
+            if ($args === 0) {
+                $callback();
+            } else {
+                $callback(
+                    function ($value = null) {
+                        $this->resolve($value);
+                    },
+                    function ($reason = null) {
+                        $this->reject($reason);
+                    },
+                    function ($update = null) {
+                        $this->notify($update);
+                    }
+                );
+            }
         } catch (\Throwable $e) {
             $this->reject($e);
         } catch (\Exception $e) {
diff --git a/tests/PromiseTest.php b/tests/PromiseTest.php
index dc7b733d..60b0c0d0 100644
--- a/tests/PromiseTest.php
+++ b/tests/PromiseTest.php
@@ -48,6 +48,18 @@ public function shouldRejectIfResolverThrowsException()
             ->then($this->expectCallableNever(), $mock);
     }
 
+    /** @test */
+    public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsException()
+    {
+        gc_collect_cycles();
+        $promise = new Promise(function () {
+            throw new \Exception('foo');
+        });
+        unset($promise);
+
+        $this->assertSame(0, gc_collect_cycles());
+    }
+
     /** @test */
     public function shouldFulfillIfFullfilledWithSimplePromise()
     {
diff --git a/tests/PromiseTest/CancelTestTrait.php b/tests/PromiseTest/CancelTestTrait.php
index 0e2b8883..aafe0688 100644
--- a/tests/PromiseTest/CancelTestTrait.php
+++ b/tests/PromiseTest/CancelTestTrait.php
@@ -14,15 +14,30 @@ abstract public function getPromiseTestAdapter(callable $canceller = null);
     /** @test */
     public function cancelShouldCallCancellerWithResolverArguments()
     {
-        $mock = $this->createCallableMock();
-        $mock
-            ->expects($this->once())
-            ->method('__invoke')
-            ->with($this->isType('callable'), $this->isType('callable'), $this->isType('callable'));
+        $args = null;
+        $adapter = $this->getPromiseTestAdapter(function ($resolve, $reject, $notify) use (&$args) {
+            $args = func_get_args();
+        });
 
-        $adapter = $this->getPromiseTestAdapter($mock);
+        $adapter->promise()->cancel();
+
+        $this->assertCount(3, $args);
+        $this->assertTrue(is_callable($args[0]));
+        $this->assertTrue(is_callable($args[1]));
+        $this->assertTrue(is_callable($args[2]));
+    }
+
+    /** @test */
+    public function cancelShouldCallCancellerWithoutArgumentsIfNotAccessed()
+    {
+        $args = null;
+        $adapter = $this->getPromiseTestAdapter(function () use (&$args) {
+            $args = func_num_args();
+        });
 
         $adapter->promise()->cancel();
+
+        $this->assertSame(0, $args);
     }
 
     /** @test */