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

Add new set_rejection_handler() function for unhandled rejections #249

Merged
merged 1 commit into from
Jul 11, 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
43 changes: 43 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ Table of Contents
* [all()](#all)
* [race()](#race)
* [any()](#any)
* [set_rejection_handler()](#set-rejection-handler)
4. [Examples](#examples)
* [How to use Deferred](#how-to-use-deferred)
* [How promise forwarding works](#how-promise-forwarding-works)
Expand Down Expand Up @@ -437,6 +438,8 @@ 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).

See also the [`set_rejection_handler()` function](#set-rejection-handler).

#### all()

```php
Expand Down Expand Up @@ -478,6 +481,46 @@ which holds all rejection reasons. The rejection reasons can be obtained with
The returned promise will also reject with a `React\Promise\Exception\LengthException`
if `$promisesOrValues` contains 0 items.

#### set_rejection_handler()

```php
React\Promise\set_rejection_handler(?callable $callback): ?callable;
```

Sets the global rejection handler for unhandled promise rejections.

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. See also the [`reject()` function](#reject)
for more details.

The `?callable $callback` argument MUST be a valid callback function that
accepts a single `Throwable` argument or a `null` value to restore the
default promise rejection handler. The return value of the callback function
will be ignored and has no effect, so you SHOULD return a `void` value. The
callback function MUST NOT throw or the program will be terminated with a
fatal error.

The function returns the previous rejection handler or `null` if using the
default promise rejection handler.

The default promise rejection handler will log an error message plus its stack
trace:

```php
// Unhandled promise rejection with RuntimeException: Unhandled in example.php:2
React\Promise\reject(new RuntimeException('Unhandled'));
```

The promise rejection handler may be used to use customize the log message or
write to custom log targets. As a rule of thumb, this function should only be
used as a last resort and promise rejections are best handled with either the
[`then()` method](#promiseinterfacethen), the
[`catch()` method](#promiseinterfacecatch), or the
[`finally()` method](#promiseinterfacefinally).
See also the [`reject()` function](#reject) for more details.

Examples
--------

Expand Down
22 changes: 19 additions & 3 deletions src/Internal/RejectedPromise.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use React\Promise\PromiseInterface;
use function React\Promise\_checkTypehint;
use function React\Promise\resolve;
use function React\Promise\set_rejection_handler;

/**
* @internal
Expand All @@ -25,16 +26,31 @@ public function __construct(\Throwable $reason)
$this->reason = $reason;
}

/** @throws void */
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();
$handler = set_rejection_handler(null);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello.
Why did you make this handler one-time use? In this case, it's hard to even guess on which promise it will be used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, it's one time use because this is inside the promise that is not handled when rejected. It will only ever run once. It might run again for another promise, but not this specific one.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But there is no connection between the handler and the promise here. I can't explicitly specify which promise this handler is for. And if the user completes another promise without handling the exception, they will not be notified about it, while the promise for which I would like to define a handler will complete without a handler.

Are you considering the possibility of adding a function to set a global persistent handler for all promises? Or add the ability to specify a chain of promises for the handler?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @roxblnfk, took a closer look at this with @clue and it seems like following exceptions are currently not being handled correctly. We'll look into this, create some additional tests for multiple exceptions to define our expected behavior and implement a fix.

Thanks for taking the time and bringing this up 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I think I misunderstood your initial comment. You are right, this shouldn't be a one use only handler but a persistent one instead. We're looking into adding additional tests and a fix for this.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

if ($handler === null) {
$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);
\error_log($message);
return;
}

try {
$handler($this->reason);
} catch (\Throwable $e) {
$message = 'Fatal error: Uncaught ' . \get_class($e) . ' from unhandled promise rejection handler: ' . $e->getMessage() . ' in ' . $e->getFile() . ':' . $e->getLine() . PHP_EOL;
$message .= 'Stack trace:' . PHP_EOL . $e->getTraceAsString();

\error_log($message);
exit(255);
}
}

public function then(callable $onFulfilled = null, callable $onRejected = null): PromiseInterface
Expand Down
47 changes: 47 additions & 0 deletions src/functions.php
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,53 @@ function (\Throwable $reason) use ($i, &$reasons, &$toReject, $reject, &$continu
}, $cancellationQueue);
}

/**
* Sets the global rejection handler for unhandled promise rejections.
*
* 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. See also the [`reject()` function](#reject)
* for more details.
*
* The `?callable $callback` argument MUST be a valid callback function that
* accepts a single `Throwable` argument or a `null` value to restore the
* default promise rejection handler. The return value of the callback function
* will be ignored and has no effect, so you SHOULD return a `void` value. The
* callback function MUST NOT throw or the program will be terminated with a
* fatal error.
*
* The function returns the previous rejection handler or `null` if using the
* default promise rejection handler.
*
* The default promise rejection handler will log an error message plus its
* stack trace:
*
* ```php
* // Unhandled promise rejection with RuntimeException: Unhandled in example.php:2
* React\Promise\reject(new RuntimeException('Unhandled'));
* ```
*
* The promise rejection handler may be used to use customize the log message or
* write to custom log targets. As a rule of thumb, this function should only be
* used as a last resort and promise rejections are best handled with either the
* [`then()` method](#promiseinterfacethen), the
* [`catch()` method](#promiseinterfacecatch), or the
* [`finally()` method](#promiseinterfacefinally).
* See also the [`reject()` function](#reject) for more details.
*
* @param callable(\Throwable):void|null $callback
* @return callable(\Throwable):void|null
*/
function set_rejection_handler(?callable $callback): ?callable
{
static $current = null;
$previous = $current;
$current = $callback;

return $previous;
}

/**
* @internal
*/
Expand Down
25 changes: 25 additions & 0 deletions tests/FunctionSetRejectionHandlerShouldBeInvokedForUnhandled.phpt
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
--TEST--
The callback given to set_rejection_handler() should be invoked for unhandled rejection
--INI--
# suppress legacy PHPUnit 7 warning for Xdebug 3
xdebug.default_enable=
--FILE--
<?php

use function React\Promise\reject;
use function React\Promise\set_rejection_handler;

require __DIR__ . '/../vendor/autoload.php';

set_rejection_handler(function (Throwable $e): void {
echo 'Unhandled ' . get_class($e) . ': ' . $e->getMessage() . PHP_EOL;
});

reject(new RuntimeException('foo'));

echo 'done' . PHP_EOL;

?>
--EXPECT--
Unhandled RuntimeException: foo
done
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
--TEST--
The callback given to the last set_rejection_handler() should be invoked for unhandled rejection
--INI--
# suppress legacy PHPUnit 7 warning for Xdebug 3
xdebug.default_enable=
--FILE--
<?php

use function React\Promise\reject;
use function React\Promise\set_rejection_handler;

require __DIR__ . '/../vendor/autoload.php';

$ret = set_rejection_handler($first = function (Throwable $e): void {
echo 'THIS WILL NEVER BE CALLED' . PHP_EOL;
});

// previous should be null
var_dump($ret === null);

$ret = set_rejection_handler(function (Throwable $e): void {
echo 'Unhandled ' . get_class($e) . ': ' . $e->getMessage() . PHP_EOL;
});

// previous rejection handler should be first rejection handler callback
var_dump($ret === $first);

reject(new RuntimeException('foo'));

echo 'done' . PHP_EOL;

?>
--EXPECT--
bool(true)
bool(true)
Unhandled RuntimeException: foo
done
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
--TEST--
The callback given to set_rejection_handler() should be invoked for outer unhandled rejection but should use default rejection handler for unhandled rejection
--INI--
# suppress legacy PHPUnit 7 warning for Xdebug 3
xdebug.default_enable=
--FILE--
<?php

use function React\Promise\reject;
use function React\Promise\set_rejection_handler;

require __DIR__ . '/../vendor/autoload.php';

set_rejection_handler(function (Throwable $e): void {
reject(new \UnexpectedValueException('bar'));

echo 'Unhandled ' . get_class($e) . ': ' . $e->getMessage() . PHP_EOL;
});

reject(new RuntimeException('foo'));

echo 'done' . PHP_EOL;

?>
--EXPECTF--
Unhandled promise rejection with UnexpectedValueException: bar in %s:%d
Stack trace:
#0 %A{main}
Unhandled RuntimeException: foo
done
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
--TEST--
The callback given to set_rejection_handler() should not throw an exception or the program should terminate for unhandled rejection
--INI--
# suppress legacy PHPUnit 7 warning for Xdebug 3
xdebug.default_enable=
--FILE--
<?php

use function React\Promise\reject;
use function React\Promise\set_rejection_handler;

require __DIR__ . '/../vendor/autoload.php';

set_rejection_handler(function (Throwable $e): void {
throw new \UnexpectedValueException('This function should never throw');
});

reject(new RuntimeException('foo'));

echo 'NEVER';

?>
--EXPECTF--
Fatal error: Uncaught UnexpectedValueException from unhandled promise rejection handler: This function should never throw in %s:%d
Stack trace:
#0 %A{main}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
--TEST--
The callback given to set_rejection_handler() may trigger a fatal error for unhandled rejection
--INI--
# suppress legacy PHPUnit 7 warning for Xdebug 3
xdebug.default_enable=
--FILE--
<?php

use function React\Promise\reject;
use function React\Promise\set_rejection_handler;

require __DIR__ . '/../vendor/autoload.php';

set_rejection_handler(function (Throwable $e): void {
trigger_error('Unexpected ' . get_class($e) . ': ' .$e->getMessage(), E_USER_ERROR);
});

reject(new RuntimeException('foo'));

echo 'NEVER';

?>
--EXPECTF--
Fatal error: Unexpected RuntimeException: foo in %s line %d
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
--TEST--
The callback given to set_rejection_handler() may trigger a fatal error which in turn throws an exception which will terminate the program for unhandled rejection
--INI--
# suppress legacy PHPUnit 7 warning for Xdebug 3
xdebug.default_enable=
--FILE--
<?php

use function React\Promise\reject;
use function React\Promise\set_rejection_handler;

require __DIR__ . '/../vendor/autoload.php';

set_error_handler(function (int $_, string $errstr): void {
throw new \OverflowException('This function should never throw');
});

set_rejection_handler(function (Throwable $e): void {
trigger_error($e->getMessage(), E_USER_ERROR);
});

reject(new RuntimeException('foo'));

echo 'NEVER';

?>
--EXPECTF--
Fatal error: Uncaught OverflowException from unhandled promise rejection handler: This function should never throw in %s:%d
Stack trace:
#0 [internal function]: {closure}(%S)
#1 %s(%d): trigger_error(%S)
#2 %s/src/Internal/RejectedPromise.php(%d): {closure}(%S)
#3 %s/src/functions.php(%d): React\Promise\Internal\RejectedPromise->__destruct()
#4 %s(%d): React\Promise\reject(%S)
#5 %A{main}
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
--TEST--
The callback given to set_rejection_handler() should be invoked for outer unhandled rejection and may set new rejection handler for inner unhandled rejection
--INI--
# suppress legacy PHPUnit 7 warning for Xdebug 3
xdebug.default_enable=
--FILE--
<?php

use function React\Promise\reject;
use function React\Promise\set_rejection_handler;

require __DIR__ . '/../vendor/autoload.php';

set_rejection_handler(function (Throwable $e): void {
$ret = set_rejection_handler(function (Throwable $e): void {
echo 'Unhandled inner ' . get_class($e) . ': ' . $e->getMessage() . PHP_EOL;
});

// previous rejection handler should be unset while handling a rejection
var_dump($ret === null);

reject(new \UnexpectedValueException('bar'));

echo 'Unhandled outer ' . get_class($e) . ': ' . $e->getMessage() . PHP_EOL;
});

reject(new RuntimeException('foo'));

echo 'done' . PHP_EOL;

?>
--EXPECT--
bool(true)
Unhandled inner UnexpectedValueException: bar
Unhandled outer RuntimeException: foo
done