From 23cd28cd2d42105fb5442c2c2caec81026ddd550 Mon Sep 17 00:00:00 2001 From: Jan Sorgalla Date: Thu, 24 Aug 2017 21:17:10 +0200 Subject: [PATCH] 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. --- README.md | 8 +- src/FulfilledPromise.php | 8 +- src/RejectedPromise.php | 16 ++- src/functions.php | 16 +++ tests/ErrorCollector.php | 29 ++++++ .../PromiseTest/PromiseFulfilledTestTrait.php | 30 ++++-- .../PromiseTest/PromiseRejectedTestTrait.php | 97 ++++++++++++++----- tests/PromiseTest/RejectTestTrait.php | 90 ++++++++++++----- tests/PromiseTest/ResolveTestTrait.php | 26 +++-- 9 files changed, 248 insertions(+), 72 deletions(-) create mode 100644 tests/ErrorCollector.php diff --git a/README.md b/README.md index befa1cd3..70f17363 100644 --- a/README.md +++ b/README.md @@ -201,8 +201,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`. @@ -651,8 +651,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 index caa98336..2bfaf5b3 100644 --- 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 index adea8e2e..7d6bf95f 100644 --- 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 index 97d03e6d..6594de13 100644 --- 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 new file mode 100644 index 00000000..970f8628 --- /dev/null +++ b/tests/ErrorCollector.php @@ -0,0 +1,29 @@ +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 index 428230b9..2f91d3c2 100644 --- 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 index 98d1dcf9..31ab3d55 100644 --- 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 */ diff --git a/tests/PromiseTest/RejectTestTrait.php b/tests/PromiseTest/RejectTestTrait.php index 9de7ca0c..cf0671cc 100644 --- a/tests/PromiseTest/RejectTestTrait.php +++ b/tests/PromiseTest/RejectTestTrait.php @@ -145,61 +145,86 @@ public function doneShouldInvokeRejectionHandler() } /** @test */ - public function doneShouldThrowExceptionThrownByRejectionHandler() + public function doneShouldTriggerFatalErrorExceptionThrownByRejectionHandler() { - $adapter = $this->getPromiseTestAdapter(); + $errorCollector = new Promise\ErrorCollector(); + $errorCollector->start(); - $this->setExpectedException('\Exception', 'UnhandledRejectionException'); + $adapter = $this->getPromiseTestAdapter(); $this->assertNull($adapter->promise()->done(null, function () { - throw new \Exception('UnhandledRejectionException'); + throw new \Exception('Unhandled Rejection'); })); $adapter->reject(1); + + $errors = $errorCollector->stop(); + + $this->assertEquals(E_USER_ERROR, $errors[0]['errno']); + $this->assertContains('Unhandled Rejection', $errors[0]['errstr']); } /** @test */ - public function doneShouldThrowUnhandledRejectionExceptionWhenRejectedWithNonException() + public function doneShouldTriggerFatalErrorUnhandledRejectionExceptionWhenRejectedWithNonException() { - $adapter = $this->getPromiseTestAdapter(); + $errorCollector = new Promise\ErrorCollector(); + $errorCollector->start(); - $this->setExpectedException('React\\Promise\\UnhandledRejectionException'); + $adapter = $this->getPromiseTestAdapter(); $this->assertNull($adapter->promise()->done()); $adapter->reject(1); + + $errors = $errorCollector->stop(); + + $this->assertEquals(E_USER_ERROR, $errors[0]['errno']); + $this->assertContains('Unhandled Rejection: 1', $errors[0]['errstr']); } /** @test */ - public function doneShouldThrowUnhandledRejectionExceptionWhenRejectionHandlerRejects() + public function doneShouldTriggerFatalErrorUnhandledRejectionExceptionWhenRejectionHandlerRejects() { - $adapter = $this->getPromiseTestAdapter(); + $errorCollector = new Promise\ErrorCollector(); + $errorCollector->start(); - $this->setExpectedException('React\\Promise\\UnhandledRejectionException'); + $adapter = $this->getPromiseTestAdapter(); $this->assertNull($adapter->promise()->done(null, function () { return \React\Promise\reject(); })); $adapter->reject(1); + + $errors = $errorCollector->stop(); + + $this->assertEquals(E_USER_ERROR, $errors[0]['errno']); + $this->assertContains('Unhandled Rejection: null', $errors[0]['errstr']); } /** @test */ - public function doneShouldThrowRejectionExceptionWhenRejectionHandlerRejectsWithException() + public function doneShouldTriggerFatalErrorRejectionExceptionWhenRejectionHandlerRejectsWithException() { - $adapter = $this->getPromiseTestAdapter(); + $errorCollector = new Promise\ErrorCollector(); + $errorCollector->start(); - $this->setExpectedException('\Exception', 'UnhandledRejectionException'); + $adapter = $this->getPromiseTestAdapter(); $this->assertNull($adapter->promise()->done(null, function () { - return \React\Promise\reject(new \Exception('UnhandledRejectionException')); + return \React\Promise\reject(new \Exception('Unhandled Rejection')); })); $adapter->reject(1); + + $errors = $errorCollector->stop(); + + $this->assertEquals(E_USER_ERROR, $errors[0]['errno']); + $this->assertContains('Unhandled Rejection', $errors[0]['errstr']); } /** @test */ - public function doneShouldThrowUnhandledRejectionExceptionWhenRejectionHandlerRetunsPendingPromiseWhichRejectsLater() + public function doneShouldTriggerFatalErrorUnhandledRejectionExceptionWhenRejectionHandlerRetunsPendingPromiseWhichRejectsLater() { - $adapter = $this->getPromiseTestAdapter(); + $errorCollector = new Promise\ErrorCollector(); + $errorCollector->start(); - $this->setExpectedException('React\\Promise\\UnhandledRejectionException'); + $adapter = $this->getPromiseTestAdapter(); $d = new Deferred(); $promise = $d->promise(); @@ -209,25 +234,37 @@ public function doneShouldThrowUnhandledRejectionExceptionWhenRejectionHandlerRe })); $adapter->reject(1); $d->reject(1); + + $errors = $errorCollector->stop(); + + $this->assertEquals(E_USER_ERROR, $errors[0]['errno']); + $this->assertContains('Unhandled Rejection: 1', $errors[0]['errstr']); } /** @test */ - public function doneShouldThrowExceptionProvidedAsRejectionValue() + public function doneShouldTriggerFatalErrorExceptionProvidedAsRejectionValue() { - $adapter = $this->getPromiseTestAdapter(); + $errorCollector = new Promise\ErrorCollector(); + $errorCollector->start(); - $this->setExpectedException('\Exception', 'UnhandledRejectionException'); + $adapter = $this->getPromiseTestAdapter(); $this->assertNull($adapter->promise()->done()); - $adapter->reject(new \Exception('UnhandledRejectionException')); + $adapter->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 doneShouldThrowWithDeepNestingPromiseChains() + public function doneShouldTriggerFatalErrorWithDeepNestingPromiseChains() { - $this->setExpectedException('\Exception', 'UnhandledRejectionException'); + $errorCollector = new Promise\ErrorCollector(); + $errorCollector->start(); - $exception = new \Exception('UnhandledRejectionException'); + $exception = new \Exception('Unhandled Rejection'); $d = new Deferred(); @@ -245,6 +282,11 @@ function () use ($exception) { $result->done(); $d->resolve(); + + $errors = $errorCollector->stop(); + + $this->assertEquals(E_USER_ERROR, $errors[0]['errno']); + $this->assertEquals((string) $exception, $errors[0]['errstr']); } /** @test */ diff --git a/tests/PromiseTest/ResolveTestTrait.php b/tests/PromiseTest/ResolveTestTrait.php index 0736d35a..937d6782 100644 --- a/tests/PromiseTest/ResolveTestTrait.php +++ b/tests/PromiseTest/ResolveTestTrait.php @@ -177,29 +177,41 @@ public function doneShouldInvokeFulfillmentHandler() } /** @test */ - public function doneShouldThrowExceptionThrownFulfillmentHandler() + public function doneShouldTriggerFatalErrorExceptionThrownFulfillmentHandler() { - $adapter = $this->getPromiseTestAdapter(); + $errorCollector = new Promise\ErrorCollector(); + $errorCollector->start(); - $this->setExpectedException('\Exception', 'UnhandledRejectionException'); + $adapter = $this->getPromiseTestAdapter(); $this->assertNull($adapter->promise()->done(function () { - throw new \Exception('UnhandledRejectionException'); + throw new \Exception('Unhandled Rejection'); })); $adapter->resolve(1); + + $errors = $errorCollector->stop(); + + $this->assertEquals(E_USER_ERROR, $errors[0]['errno']); + $this->assertContains('Unhandled Rejection', $errors[0]['errstr']); } /** @test */ - public function doneShouldThrowUnhandledRejectionExceptionWhenFulfillmentHandlerRejects() + public function doneShouldTriggerFatalErrorUnhandledRejectionExceptionWhenFulfillmentHandlerRejects() { - $adapter = $this->getPromiseTestAdapter(); + $errorCollector = new Promise\ErrorCollector(); + $errorCollector->start(); - $this->setExpectedException('React\\Promise\\UnhandledRejectionException'); + $adapter = $this->getPromiseTestAdapter(); $this->assertNull($adapter->promise()->done(function () { return \React\Promise\reject(); })); $adapter->resolve(1); + + $errors = $errorCollector->stop(); + + $this->assertEquals(E_USER_ERROR, $errors[0]['errno']); + $this->assertContains('Unhandled Rejection: null', $errors[0]['errstr']); } /** @test */