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.

Sign up for GitHub

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

Trigger E_USER_ERROR on unhandled exceptions #170

Closed
wants to merge 1 commit into from
Closed

Trigger E_USER_ERROR on unhandled exceptions #170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/Internal/FulfilledPromise.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
use function React\Promise\enqueue;
use function React\Promise\fatalError;
use function React\Promise\resolve;
use function React\Promise\unhandledException;

/**
* @internal
Expand Down Expand Up @@ -51,7 +52,7 @@ public function done(callable $onFulfilled = null, callable $onRejected = null):
try {
$result = $onFulfilled($this->value);
} catch (\Throwable $exception) {
return fatalError($exception);
unhandledException($exception);
}

if ($result instanceof PromiseInterface) {
Expand Down
24 changes: 20 additions & 4 deletions src/Internal/RejectedPromise.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,29 +4,43 @@

use React\Promise\Promise;
use React\Promise\PromiseInterface;
use Throwable;
use function React\Promise\_checkTypehint;
use function React\Promise\enqueue;
use function React\Promise\fatalError;
use function React\Promise\resolve;
use function React\Promise\unhandledException;

/**
* @internal
*/
final class RejectedPromise implements PromiseInterface
{
private $reason;
private $handled = false;

public function __construct(\Throwable $reason)
public function __construct(Throwable $reason)
{
$this->reason = $reason;
}

public function __destruct()
{
if ($this->handled) {
return;
}

unhandledException($this->reason);
}

public function then(callable $onFulfilled = null, callable $onRejected = null): PromiseInterface
{
if (null === $onRejected) {
return $this;
}

$this->handled = true;

return new Promise(function (callable $resolve, callable $reject) use ($onRejected): void {
enqueue(function () use ($resolve, $reject, $onRejected): void {
try {
Expand All @@ -41,18 +55,20 @@ public function then(callable $onFulfilled = null, callable $onRejected = null):
public function done(callable $onFulfilled = null, callable $onRejected = null): void
{
enqueue(function () use ($onRejected) {
$this->handled = true;

if (null === $onRejected) {
return fatalError($this->reason);
unhandledException($this->reason);
}

try {
$result = $onRejected($this->reason);
} catch (\Throwable $exception) {
return fatalError($exception);
unhandledException($exception);
}

if ($result instanceof self) {
return fatalError($result->reason);
unhandledException($result->reason);
}

if ($result instanceof PromiseInterface) {
Expand Down
22 changes: 16 additions & 6 deletions src/functions.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
use React\Promise\Exception\CompositeException;
use React\Promise\Internal\FulfilledPromise;
use React\Promise\Internal\RejectedPromise;
use Throwable;

/**
* Creates a promise for the supplied `$promiseOrValue`.
Expand Down Expand Up @@ -313,14 +314,23 @@ function enqueue(callable $task): void
/**
* @internal
*/
function fatalError($error): void
function unhandledException(Throwable $throwable): void
{
try {
\trigger_error($error, E_USER_ERROR);
} catch (\Throwable $e) {
\set_error_handler(null);
\trigger_error($error, E_USER_ERROR);
// set dummy handler and return previous handler
$handler = set_exception_handler(static function (Throwable $throwable) {
throw $throwable;
});

// use dummy handler when no original handler is available
if ($handler === null) {
$handler = set_exception_handler(null);
}

set_exception_handler(null);
Comment on lines +319 to +329
Copy link

@Seldaek Seldaek Jan 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

IMO you need to restore the original handler there, not wipe it.

Suggested change
// set dummy handler and return previous handler
$handler = set_exception_handler(static function (Throwable $throwable) {
throw $throwable;
});
// use dummy handler when no original handler is available
if ($handler === null) {
$handler = set_exception_handler(null);
}
set_exception_handler(null);
// set dummy handler and return previous handler
$handler = set_exception_handler(static function (Throwable $throwable) {
throw $throwable;
});
// use dummy handler when no original handler is available
if ($handler === null) {
$handler = set_exception_handler(null);
} else {
// restore original handler
set_exception_handler($handler);
}


$handler($throwable);

die(255);
}

/**
Expand Down
4 changes: 3 additions & 1 deletion tests/DeferredTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerRejectsWithEx
$deferred = new Deferred(function ($resolve, $reject) {
$reject(new \Exception('foo'));
});
$deferred->promise()->then(null, function () { });
$deferred->promise()->cancel();
unset($deferred);

Expand All @@ -42,7 +43,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfParentCancellerRejects
$deferred = new Deferred(function ($resolve, $reject) {
$reject(new \Exception('foo'));
});
$deferred->promise()->then()->cancel();
$deferred->promise()->then(null, function () { })->cancel();
unset($deferred);

$this->assertSame(0, gc_collect_cycles());
Expand All @@ -56,6 +57,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerHoldsReferenc

$deferred = new Deferred(function () use (&$deferred) { });
$deferred->reject(new \Exception('foo'));
$deferred->promise()->then(null, function () { });
unset($deferred);

$this->assertSame(0, gc_collect_cycles());
Expand Down
2 changes: 1 addition & 1 deletion tests/FunctionRaceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,6 @@ public function shouldNotCancelOtherPendingInputArrayPromisesIfOnePromiseRejects

$promise2 = new Promise(function () {}, $this->expectCallableNever());

race([$deferred->promise(), $promise2])->cancel();
race([$deferred->promise(), $promise2])->then(null, function () { })->cancel();
}
}
4 changes: 3 additions & 1 deletion tests/FunctionSomeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,8 @@ public function shouldNotCancelOtherPendingInputArrayPromisesIfEnoughPromisesRej

$promise2 = new Promise(function () {}, $this->expectCallableNever());

some([$deferred->promise(), $promise2], 2);
$ret = some([$deferred->promise(), $promise2], 2);

$ret->then(null, function () { });
}
}
11 changes: 11 additions & 0 deletions tests/Internal/RejectedPromiseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use Exception;
use LogicException;
use React\Promise\ErrorCollector;
use React\Promise\PromiseAdapter\CallbackPromiseAdapter;
use React\Promise\PromiseTest\PromiseRejectedTestTrait;
use React\Promise\PromiseTest\PromiseSettledTestTrait;
Expand Down Expand Up @@ -45,4 +46,14 @@ public function getPromiseTestAdapter(callable $canceller = null)
},
]);
}

/** @test */
public function unhandledRejectionShouldTriggerFatalError()
{
self::expectException(Exception::class);
self::expectExceptionMessage('foo');

$promise = new RejectedPromise(new Exception('foo'));
unset($promise);
}
}
12 changes: 10 additions & 2 deletions tests/PromiseTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsExceptio
$promise = new Promise(function () {
throw new \Exception('foo');
});
$promise->then(null, function () { });
unset($promise);

$this->assertSame(0, gc_collect_cycles());
Expand All @@ -78,6 +79,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfResolverRejectsWithExc
$promise = new Promise(function ($resolve, $reject) {
$reject(new \Exception('foo'));
});
$promise->then(null, function () { });
unset($promise);

$this->assertSame(0, gc_collect_cycles());
Expand All @@ -91,6 +93,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerRejectsWithEx
$reject(new \Exception('foo'));
});
$promise->cancel();
$promise->then(null, function () { });
unset($promise);

$this->assertSame(0, gc_collect_cycles());
Expand All @@ -103,7 +106,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfParentCancellerRejects
$promise = new Promise(function ($resolve, $reject) { }, function ($resolve, $reject) {
$reject(new \Exception('foo'));
});
$promise->then()->then()->then()->cancel();
$promise->then()->then()->then(null, function () { })->cancel();
unset($promise);

$this->assertSame(0, gc_collect_cycles());
Expand All @@ -116,6 +119,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfResolverThrowsExceptio
$promise = new Promise(function ($resolve, $reject) {
throw new \Exception('foo');
});
$promise->then(null, function () { });
unset($promise);

$this->assertSame(0, gc_collect_cycles());
Expand All @@ -141,6 +145,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerWithReference
throw new \Exception('foo');
});
$promise->cancel();
$promise->then(null, function () { });
unset($promise);

$this->assertSame(0, gc_collect_cycles());
Expand All @@ -157,6 +162,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfResolverWithReferenceT
$promise = new Promise(function () use (&$promise) {
throw new \Exception('foo');
});
$promise->then(null, function () { });
unset($promise);

$this->assertSame(0, gc_collect_cycles());
Expand All @@ -173,6 +179,7 @@ public function shouldRejectWithoutCreatingGarbageCyclesIfCancellerHoldsReferenc
$promise = new Promise(function () {
throw new \Exception('foo');
}, function () use (&$promise) { });
$promise->then(null, function () { });
unset($promise);

$this->assertSame(0, gc_collect_cycles());
Expand All @@ -186,7 +193,7 @@ public function shouldIgnoreNotifyAfterReject()
$notify(42);
});

$promise->then(null, null, $this->expectCallableNever());
$promise->then(null, function () { }, $this->expectCallableNever());
$promise->cancel();
}

Expand Down Expand Up @@ -263,6 +270,7 @@ public function shouldFulfillIfFullfilledWithSimplePromise()
$promise = new Promise(function () {
throw new Exception('foo');
});
$promise->then(null, function () { });
unset($promise);

self::assertSame(0, gc_collect_cycles());
Expand Down
14 changes: 6 additions & 8 deletions tests/PromiseTest/CancelTestTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,15 +105,13 @@ public function cancelShouldRejectPromiseWithExceptionIfCancellerThrows()
/** @test */
public function cancelShouldCallCancellerOnlyOnceIfCancellerResolves()
{
$mock = $this->createCallableMock();
$mock
->expects($this->once())
->method('__invoke')
->will($this->returnCallback(function ($resolve) {
$resolve();
}));
$once = $this->expectCallableOnce();
$canceller = function ($resolve) use ($once) {
$resolve();
$once();
};

$adapter = $this->getPromiseTestAdapter($mock);
$adapter = $this->getPromiseTestAdapter($canceller);

$adapter->promise()->cancel();
$adapter->promise()->cancel();
Expand Down
18 changes: 4 additions & 14 deletions tests/PromiseTest/PromiseFulfilledTestTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -227,17 +227,12 @@ public function doneShouldTriggerFatalErrorThrownFulfillmentHandlerForFulfilledP

$adapter->resolve(1);

$errorCollector = new ErrorCollector();
$errorCollector->start();
self::expectException(Exception::class);
self::expectExceptionMessage('Unhandled Rejection');

self::assertNull($adapter->promise()->done(function () {
throw new Exception('Unhandled Rejection');
}));

$errors = $errorCollector->stop();

self::assertEquals(E_USER_ERROR, $errors[0]['errno']);
self::assertStringContainsString('Unhandled Rejection', $errors[0]['errstr']);
}

/** @test */
Expand All @@ -247,17 +242,12 @@ public function doneShouldTriggerFatalErrorUnhandledRejectionExceptionWhenFulfil

$adapter->resolve(1);

$errorCollector = new ErrorCollector();
$errorCollector->start();
self::expectException(Exception::class);
self::expectExceptionMessage('Unhandled Rejection');

self::assertNull($adapter->promise()->done(function () {
return reject(new Exception('Unhandled Rejection'));
}));

$errors = $errorCollector->stop();

self::assertEquals(E_USER_ERROR, $errors[0]['errno']);
self::assertStringContainsString('Unhandled Rejection', $errors[0]['errstr']);
}

/** @test */
Expand Down
Loading