-
Notifications
You must be signed in to change notification settings - Fork 3
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 PromiseErrorHandler to decouple the specification #19
Merged
Merged
Changes from all commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
60e0768
Add PromiseErrorHandler to decouple the specification from the event-…
kelunik 909f386
Trigger a fatal error on last chance error handler failure, fail if n…
kelunik 17008a6
Move ErrorHandler to subnamespace, add tests
kelunik 765a6f6
Remove ::class usage, as it's not available in PHP 5.4
kelunik 83e3b81
Use %s instead of exception name, because PHP 5 vs. PHP 7
kelunik 131253d
Use more %s because of PHP 5 vs. PHP 7
kelunik fcc5a3f
Add tests for order and remove, fix tests for PHP 7+
kelunik 10e0168
Don't use PHPDBG for now, as it changes backtraces
kelunik 13e5aea
Fix pattern for HHVM, which doesn't print arguments in backtraces
kelunik dc85e8d
Adjust tests to stricter matching
kelunik 80f375f
Show dependency tree on install on Travis
kelunik d47e280
Use parallel lint
kelunik be876a0
Remove clover coverage output
kelunik 341ac65
Change error handler to a single callback
kelunik 4058691
Use call_user_func instead of direct callback invocation
kelunik 8fc83b7
Return previous error handler callback upon set
kelunik File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
/composer.lock | ||
/vendor/ |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
language: php | ||
|
||
php: | ||
- 5.4 | ||
- 5.5 | ||
- 5.6 | ||
- 7.0 | ||
- nightly | ||
- hhvm | ||
|
||
matrix: | ||
allow_failures: | ||
- php: nightly | ||
fast_finish: true | ||
|
||
cache: | ||
directories: | ||
- $HOME/.composer/cache | ||
|
||
install: | ||
- composer install | ||
- composer show -t | ||
|
||
script: | ||
- php vendor/bin/parallel-lint --exclude vendor . | ||
- php vendor/bin/phpunit --coverage-text |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,14 +1,24 @@ | ||
{ | ||
"name": "async-interop/promise", | ||
"description": "Promise interface for implementing async operations", | ||
"description": "A promise interface for interoperability in async operations.", | ||
"keywords": ["promise", "future", "awaitable", "async", "interop"], | ||
"license": "MIT", | ||
"require": { | ||
"php": ">=5.4" | ||
"php": ">=5.4.0" | ||
}, | ||
"require-dev": { | ||
"phpunit/phpunit": "^4|^5", | ||
"jakub-onderka/php-parallel-lint": "^0.9.2", | ||
"jakub-onderka/php-console-highlighter": "^0.3.2" | ||
}, | ||
"autoload": { | ||
"psr-4": { | ||
"Interop\\Async\\": "src/" | ||
"Interop\\Async\\": "src" | ||
} | ||
}, | ||
"autoload-dev": { | ||
"psr-4": { | ||
"Interop\\Async\\Promise\\Test\\": "test" | ||
} | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
<phpunit bootstrap="./vendor/autoload.php" colors="true"> | ||
<testsuites> | ||
<testsuite name="Main Tests"> | ||
<directory>./test</directory> | ||
</testsuite> | ||
<testsuite name="PHPT Tests"> | ||
<directory suffix=".phpt">./test/phpt</directory> | ||
</testsuite> | ||
</testsuites> | ||
<filter> | ||
<whitelist addUncoveredFilesFromWhitelist="true"> | ||
<directory>./src</directory> | ||
</whitelist> | ||
</filter> | ||
</phpunit> |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,97 @@ | ||
<?php | ||
|
||
namespace Interop\Async\Promise; | ||
|
||
use Interop\Async\Promise; | ||
|
||
/** | ||
* Global error handler for promises. | ||
* | ||
* Callbacks passed to `Promise::when()` should never throw, but they might. Such errors have to be passed to this | ||
* global error handler to make them easily loggable. These can't be handled gracefully in any way, so we just enable | ||
* logging with this handler and ignore them otherwise. | ||
* | ||
* If handler is set or that handler rethrows, it will fail hard by triggering an E_USER_ERROR leading to script | ||
* abortion. | ||
*/ | ||
final class ErrorHandler | ||
{ | ||
/** @var callable|null */ | ||
private static $callback = null; | ||
|
||
private function __construct() | ||
{ | ||
// disable construction, only static helper | ||
} | ||
|
||
/** | ||
* Set a new handler that will be notified on uncaught errors during promise resolution callback invocations. | ||
* | ||
* This callback can attempt to log the error or exit the execution of the script if it sees need. It receives the | ||
* exception as first and only parameter. | ||
* | ||
* As it's already a last chance handler, the script will be aborted using E_USER_ERROR if the handler throws. Thus | ||
* it's suggested to always wrap the body of your callback in a generic `try` / `catch` block, if you want to avoid | ||
* that. | ||
* | ||
* @param callable|null $onError Callback to invoke on errors or `null` to reset. | ||
* | ||
* @return callable|null Previous callback. | ||
*/ | ||
public static function set(callable $onError = null) | ||
{ | ||
$previous = self::$callback; | ||
self::$callback = $onError; | ||
return $previous; | ||
} | ||
|
||
/** | ||
* Notifies the registered handler, that an exception occurred. | ||
* | ||
* This method MUST be called by every promise implementation if a callback passed to `Promise::when()` throws upon | ||
* invocation. It MUST NOT be called otherwise. | ||
*/ | ||
public static function notify($error) | ||
{ | ||
// No type declaration, because of PHP 5 + PHP 7 support. | ||
if (!$error instanceof \Exception && !$error instanceof \Throwable) { | ||
// We have this error handler specifically so we never throw from Promise::when, so it doesn't make sense to | ||
// throw here. We just forward a generic exception to the registered handlers. | ||
$error = new \Exception(sprintf( | ||
"Promise implementation called %s with an invalid argument of type '%s'", | ||
__METHOD__, | ||
is_object($error) ? get_class($error) : gettype($error) | ||
)); | ||
} | ||
|
||
if (self::$callback === null) { | ||
trigger_error( | ||
"An exception has been thrown from an Interop\\Async\\Promise::when handler, but no handler has been" | ||
. " registered via Interop\\Async\\Promise\\ErrorHandler::set. A handler has to be registered to" | ||
. " prevent exceptions from going unnoticed. Do NOT install an empty handler that just" | ||
. " does nothing. If the handler is called, there is ALWAYS something wrong.\n\n" . (string) $error, | ||
E_USER_ERROR | ||
); | ||
|
||
return; | ||
} | ||
|
||
try { | ||
\call_user_func(self::$callback, $error); | ||
} catch (\Exception $e) { | ||
// We're already a last chance handler, throwing doesn't make sense, so use a real fatal | ||
trigger_error(sprintf( | ||
"An exception has been thrown from the promise error handler registered to %s::set().\n\n%s", | ||
__CLASS__, | ||
(string) $e | ||
), E_USER_ERROR); | ||
} catch (\Throwable $e) { | ||
// We're already a last chance handler, throwing doesn't make sense, so use a real fatal | ||
trigger_error(sprintf( | ||
"An exception has been thrown from the promise error handler registered to %s::set().\n\n%s", | ||
__CLASS__, | ||
(string) $e | ||
), E_USER_ERROR); | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
--TEST-- | ||
ErrorHandler::notify() fatals without a handler | ||
--FILE-- | ||
<?php | ||
|
||
require __DIR__ . "/../../vendor/autoload.php"; | ||
|
||
Interop\Async\Promise\ErrorHandler::notify(new Exception); | ||
|
||
?> | ||
--EXPECTF-- | ||
Fatal error: An exception has been thrown from an Interop\Async\Promise::when handler, but no handler has been registered via Interop\Async\Promise\ErrorHandler::set. A handler has to be registered to prevent exceptions from going unnoticed. Do NOT install an empty handler that just does nothing. If the handler is called, there is ALWAYS something wrong. | ||
|
||
%s in %s:%d | ||
Stack trace: | ||
#0 {main} in %s on line %d |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
--TEST-- | ||
ErrorHandler::notify() does not fatal with a handler | ||
--FILE-- | ||
<?php | ||
|
||
require __DIR__ . "/../../vendor/autoload.php"; | ||
|
||
Interop\Async\Promise\ErrorHandler::set(function () { print "1"; }); | ||
Interop\Async\Promise\ErrorHandler::notify(new Exception); | ||
|
||
?> | ||
--EXPECT-- | ||
1 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,18 @@ | ||
--TEST-- | ||
ErrorHandler::notify() fatals after handlers have been removed with ErrorHandler::set(null) | ||
--FILE-- | ||
<?php | ||
|
||
require __DIR__ . "/../../vendor/autoload.php"; | ||
|
||
Interop\Async\Promise\ErrorHandler::set(function () { print "1"; }); | ||
Interop\Async\Promise\ErrorHandler::set(null); | ||
Interop\Async\Promise\ErrorHandler::notify(new Exception); | ||
|
||
?> | ||
--EXPECTF-- | ||
Fatal error: An exception has been thrown from an Interop\Async\Promise::when handler, but no handler has been registered via Interop\Async\Promise\ErrorHandler::set. A handler has to be registered to prevent exceptions from going unnoticed. Do NOT install an empty handler that just does nothing. If the handler is called, there is ALWAYS something wrong. | ||
|
||
%s in %s:%d | ||
Stack trace: | ||
#0 {main} in %s on line %d |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
--TEST-- | ||
ErrorHandler::notify() converts non-exception to exception | ||
--FILE-- | ||
<?php | ||
|
||
require __DIR__ . "/../../vendor/autoload.php"; | ||
|
||
Interop\Async\Promise\ErrorHandler::notify(42); | ||
|
||
?> | ||
--EXPECTF-- | ||
Fatal error: An exception has been thrown from an Interop\Async\Promise::when handler, but no handler has been registered via Interop\Async\Promise\ErrorHandler::set. A handler has to be registered to prevent exceptions from going unnoticed. Do NOT install an empty handler that just does nothing. If the handler is called, there is ALWAYS something wrong. | ||
|
||
%SException%SPromise implementation called Interop\Async\Promise\ErrorHandler::notify with an invalid argument of type 'integer'%S in %s:%d | ||
Stack trace: | ||
#0 %s(%d): Interop\Async\Promise\ErrorHandler::notify(%S) | ||
#1 {main} in %s on line %d |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
--TEST-- | ||
ErrorHandler::set() replaces the current handler | ||
--FILE-- | ||
<?php | ||
|
||
require __DIR__ . "/../../vendor/autoload.php"; | ||
|
||
Interop\Async\Promise\ErrorHandler::set(function () { print "1"; }); | ||
Interop\Async\Promise\ErrorHandler::set(function () { print "2"; }); | ||
Interop\Async\Promise\ErrorHandler::set(function () { print "3"; }); | ||
Interop\Async\Promise\ErrorHandler::notify(new Exception); | ||
|
||
?> | ||
--EXPECT-- | ||
3 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
--TEST-- | ||
ErrorHandler::notify() fatals if handler throws | ||
--FILE-- | ||
<?php | ||
|
||
require __DIR__ . "/../../vendor/autoload.php"; | ||
|
||
Interop\Async\Promise\ErrorHandler::set(function ($e) { throw $e; }); | ||
Interop\Async\Promise\ErrorHandler::notify(new Exception); | ||
|
||
?> | ||
--EXPECTF-- | ||
Fatal error: An exception has been thrown from the promise error handler registered to Interop\Async\Promise\ErrorHandler::set(). | ||
|
||
%s in %s:%d | ||
Stack trace: | ||
#0 {main} in %s on line %d |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we please return the old error handler here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need that? There should be only one handler per application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To temporarily overwrite it, e.g. in a separate loop context.
Is there any harm in providing the possibility at all?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do so, we should do the same for the loop error handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, we should do that there too. [I honestly thought we already were doing that.]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.