Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Report any unhandled promise rejections #248

Merged
merged 1 commit into from
Jul 7, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 63 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
4 changes: 4 additions & 0 deletions phpstan.neon.dist
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,7 @@ parameters:
paths:
- src/
- tests/

fileExtensions:
- php
- phpt
1 change: 1 addition & 0 deletions phpunit.xml.dist
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
<testsuites>
<testsuite name="Promise Test Suite">
<directory>./tests/</directory>
<directory suffix=".phpt">./tests/</directory>
</testsuite>
</testsuites>
<coverage>
Expand Down
1 change: 1 addition & 0 deletions phpunit.xml.legacy
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
<testsuites>
<testsuite name="Promise Test Suite">
<directory>./tests/</directory>
<directory suffix=".phpt">./tests/</directory>
</testsuite>
</testsuites>
<filter>
Expand Down
18 changes: 18 additions & 0 deletions src/Internal/RejectedPromise.php
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,9 @@ final class RejectedPromise implements PromiseInterface
/** @var \Throwable */
private $reason;

/** @var bool */
private $handled = false;

/**
* @param \Throwable $reason
*/
Expand All @@ -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) {
Expand Down Expand Up @@ -55,6 +72,7 @@ public function finally(callable $onFulfilledOrRejected): PromiseInterface

public function cancel(): void
{
$this->handled = true;
}

/**
Expand Down
14 changes: 14 additions & 0 deletions src/Promise.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand Down
6 changes: 3 additions & 3 deletions src/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,7 @@ function (\Throwable $reason) use (&$continue, $reject): void {
}
);

if (!$continue) {
if (!$continue && !\is_array($promisesOrValues)) {
break;
}
}
Expand Down Expand Up @@ -136,7 +136,7 @@ function race(iterable $promisesOrValues): PromiseInterface
$continue = false;
});

if (!$continue) {
if (!$continue && !\is_array($promisesOrValues)) {
break;
}
}
Expand Down Expand Up @@ -187,7 +187,7 @@ function (\Throwable $reason) use ($i, &$reasons, &$toReject, $reject, &$continu
}
);

if (!$continue) {
if (!$continue && !\is_array($promisesOrValues)) {
break;
}
}
Expand Down
4 changes: 4 additions & 0 deletions tests/DeferredTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
Original file line number Diff line number Diff line change
@@ -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
20 changes: 20 additions & 0 deletions tests/DeferredTestCancelThatRejectsShouldNotReportUnhandled.phpt
Original file line number Diff line number Diff line change
@@ -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
20 changes: 20 additions & 0 deletions tests/DeferredTestRejectShouldReportUnhandled.phpt
Original file line number Diff line number Diff line change
@@ -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}
21 changes: 21 additions & 0 deletions tests/DeferredTestRejectThenCancelShouldNotReportUnhandled.phpt
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion tests/FunctionAllTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand Down
23 changes: 23 additions & 0 deletions tests/FunctionAllTestRejectedShouldReportUnhandled.phpt
Original file line number Diff line number Diff line change
@@ -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}
Original file line number Diff line number Diff line change
@@ -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
Loading