Merge branch 'master' into inspection-methods

reactphp · Sep 19, 2017 · 3dbee04 · 3dbee04
2 parents a430cac + 9dcffcc
commit 3dbee04
Show file tree

Hide file tree

Showing 11 changed files with 361 additions and 92 deletions.
diff --git a/README.md b/README.md
@@ -208,8 +208,8 @@ $promise->done(callable $onFulfilled = null, callable $onRejected = null);
 Consumes the promise's ultimate value if the promise fulfills, or handles the
 ultimate error.
 
-It will cause a fatal error if either `$onFulfilled` or `$onRejected` throw or
-return a rejected promise.
+It will cause a fatal error (`E_USER_ERROR`) if either `$onFulfilled` or
+`$onRejected` throw or return a rejected promise.
 
 Since the purpose of `done()` is consumption rather than transformation,
 `done()` always returns `null`.
@@ -713,8 +713,8 @@ by the promise machinery and used to reject the promise returned by `then()`.
 
 Calling `done()` transfers all responsibility for errors to your code. If an
 error (either a thrown exception or returned rejection) escapes the
-`$onFulfilled` or `$onRejected` callbacks you provide to done, it will be
-rethrown in an uncatchable way causing a fatal error.
+`$onFulfilled` or `$onRejected` callbacks you provide to `done()`, it will cause
+a fatal error.
 
 ```php
 function getJsonResult()

diff --git a/src/FulfilledPromise.php b/src/FulfilledPromise.php
@@ -43,7 +43,13 @@ public function done(callable $onFulfilled = null, callable $onRejected = null)
         }
 
         enqueue(function () use ($onFulfilled) {
-            $result = $onFulfilled($this->value);
+            try {
+                $result = $onFulfilled($this->value);
+            } catch (\Throwable $exception) {
+                return fatalError($exception);
+            } catch (\Exception $exception) {
+                return fatalError($exception);
+            }
 
             if ($result instanceof PromiseInterface) {
                 $result->done();

diff --git a/src/Promise.php b/src/Promise.php
@@ -15,7 +15,7 @@ final class Promise implements PromiseInterface
 
     private $handlers = [];
 
-    private $remainingCancelRequests = 0;
+    private $requiredCancelRequests = 0;
     private $isCancelled = false;
 
     public function __construct(callable $resolver, callable $canceller = null)
@@ -34,14 +34,14 @@ public function then(callable $onFulfilled = null, callable $onRejected = null)
             return new static($this->resolver($onFulfilled, $onRejected));
         }
 
-        $this->remainingCancelRequests++;
+        $this->requiredCancelRequests++;
 
         return new static($this->resolver($onFulfilled, $onRejected), function () {
-            if (--$this->remainingCancelRequests > 0) {
-                return;
-            }
+            $this->requiredCancelRequests--;
 
-            $this->cancel();
+            if ($this->requiredCancelRequests <= 0) {
+                $this->cancel();
+            }
         });
     }
 
@@ -83,20 +83,39 @@ public function always(callable $onFulfilledOrRejected)
 
     public function cancel()
     {
+        $canceller = $this->canceller;
+        $this->canceller = null;
+
+        $parentCanceller = null;
+
         if (null !== $this->result) {
-            return;
+            // Go up the promise chain and reach the top most promise which is
+            // itself not following another promise
+            $root = $this->unwrap($this->result);
+
+            // Return if the root promise is already resolved or a
+            // FulfilledPromise or RejectedPromise
+            if (!$root instanceof self || null !== $root->result) {
+                return;
+            }
+
+            $root->requiredCancelRequests--;
+
+            if ($root->requiredCancelRequests <= 0) {
+                $parentCanceller = [$root, 'cancel'];
+            }
         }
 
         $this->isCancelled = true;
 
-        if (null === $this->canceller) {
-            return;
+        if (null !== $canceller) {
+            $this->call($canceller);
         }
 
-        $canceller = $this->canceller;
-        $this->canceller = null;
-
-        $this->call($canceller);
+        // For BC, we call the parent canceller after our own canceller
+        if ($parentCanceller) {
+            $parentCanceller();
+        }
     }
 
     public function isFulfilled()
@@ -182,10 +201,22 @@ private function settle(PromiseInterface $result)
     {
         $result = $this->unwrap($result);
 
+        if ($result === $this) {
+            $result = new RejectedPromise(
+                LogicException::circularResolution()
+            );
+        }
+
+        if ($result instanceof self) {
+            $result->requiredCancelRequests++;
+        } else {
+            // Unset canceller only when not following a pending promise
+            $this->canceller = null;
+        }
+
         $handlers = $this->handlers;
 
         $this->handlers = [];
-        $this->canceller = null;
         $this->result = $result;
 
         foreach ($handlers as $handler) {
@@ -199,12 +230,6 @@ private function unwrap($promise)
             $promise = $promise->result;
         }
 
-        if ($promise === $this) {
-            return new RejectedPromise(
-                LogicException::circularResolution()
-            );
-        }
-
         return $promise;
     }
 

diff --git a/src/RejectedPromise.php b/src/RejectedPromise.php
@@ -40,13 +40,23 @@ public function done(callable $onFulfilled = null, callable $onRejected = null)
     {
         enqueue(function () use ($onRejected) {
             if (null === $onRejected) {
-                throw UnhandledRejectionException::resolve($this->reason);
+                return fatalError(
+                    UnhandledRejectionException::resolve($this->reason)
+                );
             }
 
-            $result = $onRejected($this->reason);
+            try {
+                $result = $onRejected($this->reason);
+            } catch (\Throwable $exception) {
+                return fatalError($exception);
+            } catch (\Exception $exception) {
+                return fatalError($exception);
+            }
 
             if ($result instanceof self) {
-                throw UnhandledRejectionException::resolve($result->reason);
+                return fatalError(
+                    UnhandledRejectionException::resolve($result->reason)
+                );
             }
 
             if ($result instanceof PromiseInterface) {

diff --git a/src/functions.php b/src/functions.php
@@ -203,6 +203,22 @@ function enqueue(callable $task)
     $queue->enqueue($task);
 }
 
+/**
+ * @internal
+ */
+function fatalError($error)
+{
+    try {
+        trigger_error($error, E_USER_ERROR);
+    } catch (\Throwable $e) {
+        set_error_handler(null);
+        trigger_error($error, E_USER_ERROR);
+    } catch (\Exception $e) {
+        set_error_handler(null);
+        trigger_error($error, E_USER_ERROR);
+    }
+}
+
 /**
  * @internal
  */

diff --git a/tests/ErrorCollector.php b/tests/ErrorCollector.php
@@ -0,0 +1,29 @@
+<?php
+
+namespace React\Promise;
+
+final class ErrorCollector
+{
+    private $errors = [];
+
+    public function start()
+    {
+        $errors = [];
+
+        set_error_handler(function ($errno, $errstr, $errfile, $errline, $errcontext) use (&$errors) {
+            $errors[] = compact('errno', 'errstr', 'errfile', 'errline', 'errcontext');
+        });
+
+        $this->errors = &$errors;
+    }
+
+    public function stop()
+    {
+        $errors = $this->errors;
+        $this->errors = [];
+
+        restore_error_handler();
+
+        return $errors;
+    }
+}
diff --git a/tests/PromiseTest/CancelTestTrait.php b/tests/PromiseTest/CancelTestTrait.php
@@ -205,6 +205,74 @@ public function cancelShouldAlwaysTriggerCancellerWhenCalledOnRootPromise()
         $adapter->promise()->cancel();
     }
 
+    /** @test */
+    public function cancelShouldTriggerCancellerWhenFollowerCancels()
+    {
+        $adapter1 = $this->getPromiseTestAdapter($this->expectCallableOnce());
+
+        $root = $adapter1->promise();
+
+        $adapter2 = $this->getPromiseTestAdapter($this->expectCallableOnce());
+
+        $follower = $adapter2->promise();
+        $adapter2->resolve($root);
+
+        $follower->cancel();
+    }
+
+    /** @test */
+    public function cancelShouldNotTriggerCancellerWhenCancellingOnlyOneFollower()
+    {
+        $adapter1 = $this->getPromiseTestAdapter($this->expectCallableNever());
+
+        $root = $adapter1->promise();
+
+        $adapter2 = $this->getPromiseTestAdapter($this->expectCallableOnce());
+
+        $follower1 = $adapter2->promise();
+        $adapter2->resolve($root);
+
+        $adapter3 = $this->getPromiseTestAdapter($this->expectCallableNever());
+        $adapter3->resolve($root);
+
+        $follower1->cancel();
+    }
+
+    /** @test */
+    public function cancelCalledOnFollowerShouldOnlyCancelWhenAllChildrenAndFollowerCancelled()
+    {
+        $adapter1 = $this->getPromiseTestAdapter($this->expectCallableOnce());
+
+        $root = $adapter1->promise();
+
+        $child = $root->then();
+
+        $adapter2 = $this->getPromiseTestAdapter($this->expectCallableOnce());
+
+        $follower = $adapter2->promise();
+        $adapter2->resolve($root);
+
+        $follower->cancel();
+        $child->cancel();
+    }
+
+    /** @test */
+    public function cancelShouldNotTriggerCancellerWhenCancellingFollowerButNotChildren()
+    {
+        $adapter1 = $this->getPromiseTestAdapter($this->expectCallableNever());
+
+        $root = $adapter1->promise();
+
+        $root->then();
+
+        $adapter2 = $this->getPromiseTestAdapter($this->expectCallableOnce());
+
+        $follower = $adapter2->promise();
+        $adapter2->resolve($root);
+
+        $follower->cancel();
+    }
+
     /** @test */
     public function inspectionForACancelledPromiseWhenCancellerFulfills()
     {

diff --git a/tests/PromiseTest/PromiseFulfilledTestTrait.php b/tests/PromiseTest/PromiseFulfilledTestTrait.php
@@ -2,6 +2,8 @@
 
 namespace React\Promise\PromiseTest;
 
+use React\Promise\ErrorCollector;
+
 trait PromiseFulfilledTestTrait
 {
     /**
@@ -212,29 +214,43 @@ public function doneShouldInvokeFulfillmentHandlerForFulfilledPromise()
     }
 
     /** @test */
-    public function doneShouldThrowExceptionThrownFulfillmentHandlerForFulfilledPromise()
+    public function doneShouldTriggerFatalErrorThrownFulfillmentHandlerForFulfilledPromise()
     {
         $adapter = $this->getPromiseTestAdapter();
 
-        $this->setExpectedException('\Exception', 'UnhandledRejectionException');
-
         $adapter->resolve(1);
+
+        $errorCollector = new ErrorCollector();
+        $errorCollector->start();
+
         $this->assertNull($adapter->promise()->done(function () {
-            throw new \Exception('UnhandledRejectionException');
+            throw new \Exception('Unhandled Rejection');
         }));
+
+        $errors = $errorCollector->stop();
+
+        $this->assertEquals(E_USER_ERROR, $errors[0]['errno']);
+        $this->assertContains('Unhandled Rejection', $errors[0]['errstr']);
     }
 
     /** @test */
-    public function doneShouldThrowUnhandledRejectionExceptionWhenFulfillmentHandlerRejectsForFulfilledPromise()
+    public function doneShouldTriggerFatalErrorUnhandledRejectionExceptionWhenFulfillmentHandlerRejectsForFulfilledPromise()
     {
         $adapter = $this->getPromiseTestAdapter();
 
-        $this->setExpectedException('React\\Promise\\UnhandledRejectionException');
-
         $adapter->resolve(1);
+
+        $errorCollector = new ErrorCollector();
+        $errorCollector->start();
+
         $this->assertNull($adapter->promise()->done(function () {
             return \React\Promise\reject();
         }));
+
+        $errors = $errorCollector->stop();
+
+        $this->assertEquals(E_USER_ERROR, $errors[0]['errno']);
+        $this->assertContains('Unhandled Rejection: null', $errors[0]['errstr']);
     }
 
     /** @test */