Trigger an E_USER_ERROR instead of throwing an exception from done()

If an error (either a thrown exception or returned rejection) escapes the done() callbacks, it will now cause a fatal error by using trigger_error() with E_USER_ERROR instead of (re)throwing. Because promise resolution is synchronous, those exceptions bubbled up to the reject() call which mixed resolution and consumption parts.
reactphp · Aug 24, 2017 · b7555d8 · b7555d8
1 parent 3675021
commit b7555d8
Show file tree

Hide file tree

Showing 9 changed files with 248 additions and 72 deletions.
diff --git a/README.md b/README.md
@@ -203,8 +203,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`.
@@ -674,8 +674,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
@@ -41,7 +41,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/RejectedPromise.php b/src/RejectedPromise.php
@@ -38,13 +38,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/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 */

diff --git a/tests/PromiseTest/PromiseRejectedTestTrait.php b/tests/PromiseTest/PromiseRejectedTestTrait.php
@@ -3,6 +3,7 @@
 namespace React\Promise\PromiseTest;
 
 use React\Promise\Deferred;
+use React\Promise\ErrorCollector;
 use React\Promise\UnhandledRejectionException;
 
 trait PromiseRejectedTestTrait
@@ -200,27 +201,40 @@ public function doneShouldInvokeRejectionHandlerForRejectedPromise()
     }
 
     /** @test */
-    public function doneShouldThrowExceptionThrownByRejectionHandlerForRejectedPromise()
+    public function doneShouldTriggerFatalErrorExceptionThrownByRejectionHandlerForRejectedPromise()
     {
-        $adapter = $this->getPromiseTestAdapter();
+        $errorCollector = new ErrorCollector();
+        $errorCollector->start();
 
-        $this->setExpectedException('\Exception', 'UnhandledRejectionException');
+        $adapter = $this->getPromiseTestAdapter();
 
         $adapter->reject(1);
         $this->assertNull($adapter->promise()->done(null, 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 doneShouldThrowUnhandledRejectionExceptionWhenRejectedWithNonExceptionForRejectedPromise()
+    public function doneShouldTriggerFatalErrorUnhandledRejectionExceptionWhenRejectedWithNonExceptionForRejectedPromise()
     {
         $adapter = $this->getPromiseTestAdapter();
 
-        $this->setExpectedException('React\\Promise\\UnhandledRejectionException');
-
         $adapter->reject(1);
+
+        $errorCollector = new ErrorCollector();
+        $errorCollector->start();
+
         $this->assertNull($adapter->promise()->done());
+
+        $errors = $errorCollector->stop();
+
+        $this->assertEquals(E_USER_ERROR, $errors[0]['errno']);
+        $this->assertContains('Unhandled Rejection: 1', $errors[0]['errstr']);
     }
 
     /** @test */
@@ -232,57 +246,83 @@ public function unhandledRejectionExceptionThrownByDoneHoldsRejectionValue()
 
         $adapter->reject($expected);
 
-        try {
-            $adapter->promise()->done();
-        } catch (UnhandledRejectionException $e) {
-            $this->assertSame($expected, $e->getReason());
-            return;
-        }
+        $errorCollector = new ErrorCollector();
+        $errorCollector->start();
+
+        $adapter->promise()->done();
+
+        $errors = $errorCollector->stop();
 
-        $this->fail();
+        $this->assertEquals(E_USER_ERROR, $errors[0]['errno']);
+        $this->assertContains('Unhandled Rejection: {}', $errors[0]['errstr']);
+
+        $this->assertArrayHasKey('error', $errors[0]['errcontext']);
+        $this->assertInstanceOf('React\Promise\UnhandledRejectionException', $errors[0]['errcontext']['error']);
+        $this->assertSame($expected, $errors[0]['errcontext']['error']->getReason());
     }
 
     /** @test */
-    public function doneShouldThrowUnhandledRejectionExceptionWhenRejectionHandlerRejectsForRejectedPromise()
+    public function doneShouldTriggerFatalErrorUnhandledRejectionExceptionWhenRejectionHandlerRejectsForRejectedPromise()
     {
-        $adapter = $this->getPromiseTestAdapter();
+        $errorCollector = new ErrorCollector();
+        $errorCollector->start();
 
-        $this->setExpectedException('React\\Promise\\UnhandledRejectionException');
+        $adapter = $this->getPromiseTestAdapter();
 
         $adapter->reject(1);
         $this->assertNull($adapter->promise()->done(null, 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 */
-    public function doneShouldThrowRejectionExceptionWhenRejectionHandlerRejectsWithExceptionForRejectedPromise()
+    public function doneShouldTriggerFatalErrorRejectionExceptionWhenRejectionHandlerRejectsWithExceptionForRejectedPromise()
     {
-        $adapter = $this->getPromiseTestAdapter();
+        $errorCollector = new ErrorCollector();
+        $errorCollector->start();
 
-        $this->setExpectedException('\Exception', 'UnhandledRejectionException');
+        $adapter = $this->getPromiseTestAdapter();
 
         $adapter->reject(1);
         $this->assertNull($adapter->promise()->done(null, function () {
-            return \React\Promise\reject(new \Exception('UnhandledRejectionException'));
+            return \React\Promise\reject(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 doneShouldThrowExceptionProvidedAsRejectionValueForRejectedPromise()
+    public function doneShouldTriggerFatalErrorExceptionProvidedAsRejectionValueForRejectedPromise()
     {
+        $errorCollector = new ErrorCollector();
+        $errorCollector->start();
+
         $adapter = $this->getPromiseTestAdapter();
 
-        $this->setExpectedException('\Exception', 'UnhandledRejectionException');
+        $exception = new \Exception('Unhandled Rejection');
 
-        $adapter->reject(new \Exception('UnhandledRejectionException'));
+        $adapter->reject($exception);
         $this->assertNull($adapter->promise()->done());
+
+        $errors = $errorCollector->stop();
+
+        $this->assertEquals(E_USER_ERROR, $errors[0]['errno']);
+        $this->assertEquals((string) $exception, $errors[0]['errstr']);
     }
 
     /** @test */
-    public function doneShouldThrowWithDeepNestingPromiseChainsForRejectedPromise()
+    public function doneShouldTriggerFatalErrorWithDeepNestingPromiseChainsForRejectedPromise()
     {
-        $this->setExpectedException('\Exception', 'UnhandledRejectionException');
+        $errorCollector = new ErrorCollector();
+        $errorCollector->start();
 
         $exception = new \Exception('UnhandledRejectionException');
 
@@ -301,6 +341,11 @@ function () use ($exception) {
         })));
 
         $result->done();
+
+        $errors = $errorCollector->stop();
+
+        $this->assertEquals(E_USER_ERROR, $errors[0]['errno']);
+        $this->assertEquals((string) $exception, $errors[0]['errstr']);
     }
 
     /** @test */