Merge pull request #248 from clue-labs/report-unhandled

Report any unhandled promise rejections

Author: WyriHaximus

README.md

@@ -125,6 +125,8 @@ having `$onFulfilled` (which they registered via `$promise->then()`) called with
 If `$value` itself is a promise, the promise will transition to the state of
 this promise once it is resolved.
 
+See also the [`resolve()` function](#resolve).
+
 #### Deferred::reject()
 
 ```php
@@ -136,6 +138,8 @@ computation failed.
 All consumers are notified by having `$onRejected` (which they registered via
 `$promise->then()`) called with `$reason`.
 
+See also the [`reject()` function](#reject).
+
 ### PromiseInterface
 
 The promise interface provides the common interface for all promise
@@ -361,6 +365,19 @@ a trusted promise that follows the state of the thenable is returned.
 
 If `$promiseOrValue` is a promise, it will be returned as is.
 
+The resulting `$promise` implements the [`PromiseInterface`](#promiseinterface)
+and can be consumed like any other promise:
+
+```php
+$promise = React\Promise\resolve(42);
+
+$promise->then(function (int $result): void {
+    var_dump($result);
+}, function (\Throwable $e): void {
+    echo 'Error: ' . $e->getMessage() . PHP_EOL;
+});
+```
+
 #### reject()
 
 ```php
@@ -374,6 +391,52 @@ both user land [`\Exception`](https://www.php.net/manual/en/class.exception.php)
 [`\Error`](https://www.php.net/manual/en/class.error.php) internal PHP errors. By enforcing `\Throwable` as reason to 
 reject a promise, any language error or user land exception can be used to reject a promise.
 
+The resulting `$promise` implements the [`PromiseInterface`](#promiseinterface)
+and can be consumed like any other promise:
+
+```php
+$promise = React\Promise\reject(new RuntimeException('Request failed'));
+
+$promise->then(function (int $result): void {
+    var_dump($result);
+}, function (\Throwable $e): void {
+    echo 'Error: ' . $e->getMessage() . PHP_EOL;
+});
+```
+
+Note that rejected promises should always be handled similar to how any
+exceptions should always be caught in a `try` + `catch` block. If you remove the
+last reference to a rejected promise that has not been handled, it will
+report an unhandled promise rejection:
+
+```php
+function incorrect(): int
+{
+     $promise = React\Promise\reject(new RuntimeException('Request failed'));
+
+     // Commented out: No rejection handler registered here.
+     // $promise->then(null, function (\Throwable $e): void { /* ignore */ });
+
+     // Returning from a function will remove all local variable references, hence why
+     // this will report an unhandled promise rejection here.
+     return 42;
+}
+
+// Calling this function will log an error message plus its stack trace:
+// Unhandled promise rejection with RuntimeException: Request failed in example.php:10
+incorrect();
+```
+
+A rejected promise will be considered "handled" if you catch the rejection
+reason with either the [`then()` method](#promiseinterfacethen), the
+[`catch()` method](#promiseinterfacecatch), or the
+[`finally()` method](#promiseinterfacefinally). Note that each of these methods
+return a new promise that may again be rejected if you re-throw an exception.
+
+A rejected promise will also be considered "handled" if you abort the operation
+with the [`cancel()` method](#promiseinterfacecancel) (which in turn would
+usually reject the promise if it is still pending).
+
 #### all()
 
 ```php

phpstan.neon.dist

@@ -4,3 +4,7 @@ parameters:
     paths:
         - src/
         - tests/
+
+    fileExtensions:
+        - php
+        - phpt

phpunit.xml.dist

@@ -10,6 +10,7 @@
     <testsuites>
         <testsuite name="Promise Test Suite">
             <directory>./tests/</directory>
+            <directory suffix=".phpt">./tests/</directory>
         </testsuite>
     </testsuites>
     <coverage>

phpunit.xml.legacy

@@ -8,6 +8,7 @@
     <testsuites>
         <testsuite name="Promise Test Suite">
             <directory>./tests/</directory>
+            <directory suffix=".phpt">./tests/</directory>
         </testsuite>
     </testsuites>
     <filter>

src/Internal/RejectedPromise.php

@@ -14,6 +14,9 @@ final class RejectedPromise implements PromiseInterface
     /** @var \Throwable */
     private $reason;
 
+    /** @var bool */
+    private $handled = false;
+
     /**
      * @param \Throwable $reason
      */
@@ -22,12 +25,26 @@ public function __construct(\Throwable $reason)
         $this->reason = $reason;
     }
 
+    public function __destruct()
+    {
+        if ($this->handled) {
+            return;
+        }
+
+        $message = 'Unhandled promise rejection with ' . \get_class($this->reason) . ': ' . $this->reason->getMessage() . ' in ' . $this->reason->getFile() . ':' . $this->reason->getLine() . PHP_EOL;
+        $message .= 'Stack trace:' . PHP_EOL . $this->reason->getTraceAsString();
+
+        \error_log($message);
+    }
+
     public function then(callable $onFulfilled = null, callable $onRejected = null): PromiseInterface
     {
         if (null === $onRejected) {
             return $this;
         }
 
+        $this->handled = true;
+
         try {
             return resolve($onRejected($this->reason));
         } catch (\Throwable $exception) {
@@ -55,6 +72,7 @@ public function finally(callable $onFulfilledOrRejected): PromiseInterface
 
     public function cancel(): void
     {
+        $this->handled = true;
     }
 
     /**

src/Promise.php

@@ -18,6 +18,9 @@ final class Promise implements PromiseInterface
     /** @var int */
     private $requiredCancelRequests = 0;
 
+    /** @var bool */
+    private $cancelled = false;
+
     public function __construct(callable $resolver, callable $canceller = null)
     {
         $this->canceller = $canceller;
@@ -89,12 +92,18 @@ public function finally(callable $onFulfilledOrRejected): PromiseInterface
 
     public function cancel(): void
     {
+        $this->cancelled = true;
         $canceller = $this->canceller;
         $this->canceller = null;
 
         $parentCanceller = null;
 
         if (null !== $this->result) {
+            // Forward cancellation to rejected promise to avoid reporting unhandled rejection
+            if ($this->result instanceof RejectedPromise) {
+                $this->result->cancel();
+            }
+
             // Go up the promise chain and reach the top most promise which is
             // itself not following another promise
             $root = $this->unwrap($this->result);
@@ -191,6 +200,11 @@ private function settle(PromiseInterface $result): void
         foreach ($handlers as $handler) {
             $handler($result);
         }
+
+        // Forward cancellation to rejected promise to avoid reporting unhandled rejection
+        if ($this->cancelled && $result instanceof RejectedPromise) {
+            $result->cancel();
+        }
     }
 
     private function unwrap(PromiseInterface $promise): PromiseInterface

src/functions.php

@@ -100,7 +100,7 @@ function (\Throwable $reason) use (&$continue, $reject): void {
                 }
             );
 
-            if (!$continue) {
+            if (!$continue && !\is_array($promisesOrValues)) {
                 break;
             }
         }
@@ -136,7 +136,7 @@ function race(iterable $promisesOrValues): PromiseInterface
                 $continue = false;
             });
 
-            if (!$continue) {
+            if (!$continue && !\is_array($promisesOrValues)) {
                 break;
             }
         }
@@ -187,7 +187,7 @@ function (\Throwable $reason) use ($i, &$reasons, &$toReject, $reject, &$continu
                 }
             );
 
-            if (!$continue) {
+            if (!$continue && !\is_array($promisesOrValues)) {
                 break;
             }
         }

tests/DeferredTest.php

@@ -54,9 +54,13 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerHoldsReferenc
         gc_collect_cycles();
         gc_collect_cycles(); // clear twice to avoid leftovers in PHP 7.4 with ext-xdebug and code coverage turned on
 
+        /** @var Deferred $deferred */
         $deferred = new Deferred(function () use (&$deferred) {
             assert($deferred instanceof Deferred);
         });
+
+        $deferred->promise()->then(null, $this->expectCallableOnce()); // avoid reporting unhandled rejection
+
         $deferred->reject(new \Exception('foo'));
         unset($deferred);
 

tests/DeferredTestCancelNoopThenRejectShouldNotReportUnhandled.phpt

@@ -0,0 +1,21 @@
+--TEST--
+Calling cancel() and then reject() should not report unhandled rejection
+--INI--
+# suppress legacy PHPUnit 7 warning for Xdebug 3
+xdebug.default_enable=
+--FILE--
+<?php
+
+use React\Promise\Deferred;
+
+require __DIR__ . '/../vendor/autoload.php';
+
+$deferred = new Deferred();
+$deferred->promise()->cancel();
+$deferred->reject(new RuntimeException('foo'));
+
+echo 'void' . PHP_EOL;
+
+?>
+--EXPECT--
+void

tests/DeferredTestCancelThatRejectsShouldNotReportUnhandled.phpt

@@ -0,0 +1,20 @@
+--TEST--
+Calling cancel() that rejects should not report unhandled rejection
+--INI--
+# suppress legacy PHPUnit 7 warning for Xdebug 3
+xdebug.default_enable=
+--FILE--
+<?php
+
+use React\Promise\Deferred;
+
+require __DIR__ . '/../vendor/autoload.php';
+
+$deferred = new Deferred(function () { throw new \RuntimeException('Cancelled'); });
+$deferred->promise()->cancel();
+
+echo 'void' . PHP_EOL;
+
+?>
+--EXPECT--
+void

tests/DeferredTestRejectShouldReportUnhandled.phpt

@@ -0,0 +1,20 @@
+--TEST--
+Calling reject() without any handlers should report unhandled rejection
+--INI--
+# suppress legacy PHPUnit 7 warning for Xdebug 3
+xdebug.default_enable=
+--FILE--
+<?php
+
+use React\Promise\Deferred;
+
+require __DIR__ . '/../vendor/autoload.php';
+
+$deferred = new Deferred();
+$deferred->reject(new RuntimeException('foo'));
+
+?>
+--EXPECTF--
+Unhandled promise rejection with RuntimeException: foo in %s:%d
+Stack trace:
+#0 %A{main}

tests/DeferredTestRejectThenCancelShouldNotReportUnhandled.phpt

@@ -0,0 +1,21 @@
+--TEST--
+Calling reject() and then cancel() should not report unhandled rejection
+--INI--
+# suppress legacy PHPUnit 7 warning for Xdebug 3
+xdebug.default_enable=
+--FILE--
+<?php
+
+use React\Promise\Deferred;
+
+require __DIR__ . '/../vendor/autoload.php';
+
+$deferred = new Deferred();
+$deferred->reject(new RuntimeException('foo'));
+$deferred->promise()->cancel();
+
+echo 'void' . PHP_EOL;
+
+?>
+--EXPECT--
+void

tests/FunctionAllTest.php

@@ -106,7 +106,7 @@ public function shouldRejectIfAnyInputPromiseRejects(): void
             ->method('__invoke')
             ->with(self::identicalTo($exception2));
 
-        all([resolve(1), reject($exception2), resolve($exception3)])
+        all([resolve(1), reject($exception2), reject($exception3)])
             ->then($this->expectCallableNever(), $mock);
     }
 

tests/FunctionAllTestRejectedShouldReportUnhandled.phpt

@@ -0,0 +1,23 @@
+--TEST--
+Calling all() with rejected promises should report unhandled rejection
+--INI--
+# suppress legacy PHPUnit 7 warning for Xdebug 3
+xdebug.default_enable=
+--FILE--
+<?php
+
+use function React\Promise\all;
+use function React\Promise\reject;
+
+require __DIR__ . '/../vendor/autoload.php';
+
+all([
+    reject(new RuntimeException('foo')),
+    reject(new RuntimeException('bar'))
+]);
+
+?>
+--EXPECTF--
+Unhandled promise rejection with RuntimeException: foo in %s:%d
+Stack trace:
+#0 %A{main}

tests/FunctionAllTestRejectedThenMatchingThatReturnsShouldNotReportUnhandled.phpt

@@ -0,0 +1,23 @@
+--TEST--
+Calling all() and then then() should not report unhandled rejection
+--INI--
+# suppress legacy PHPUnit 7 warning for Xdebug 3
+xdebug.default_enable=
+--FILE--
+<?php
+
+use function React\Promise\all;
+use function React\Promise\reject;
+
+require __DIR__ . '/../vendor/autoload.php';
+
+all([
+    reject(new RuntimeException('foo')),
+    reject(new RuntimeException('bar'))
+])->then(null, function (\Throwable $e) {
+    echo 'Handled ' . get_class($e) . ': ' . $e->getMessage() . PHP_EOL;
+});
+
+?>
+--EXPECT--
+Handled RuntimeException: foo