Merge pull request #43 from async-interop/error-handler

Ensure error handler does NEVER bubble up

Author: bwoebi

README.md

@@ -67,10 +67,14 @@ interface Promise
 
 All callbacks registered before the `Promise` is resolved MUST be executed in the order they were registered after the `Promise` has been resolved. Callbacks registered after the resolution MUST be executed immediately.
 
-The invocation of `Promise::when()` MUST NOT throw exceptions bubbling up from a `$onResolved` invocation. If one of the callbacks throws an `Exception` or `Throwable`, it MUST be forwarded to `AsyncInterop\Promise\ErrorHandler::notify`. The `Promise` implementation MUST then continue to call the remaining callbacks with the original parameters.
+The invocation of `Promise::when()` MUST NOT throw exceptions bubbling up from an `$onResolved` invocation. If one of the callbacks throws an `Exception` or `Throwable`, the `Promise` implementation MUST catch it and call `AsyncInterop\Promise\ErrorHandler::notify()` with the `Exception` or `Throwable` as first argument. The `Promise` implementation MUST then continue to call the remaining callbacks with the original parameters.
 
 Registered callbacks MUST NOT be called from a file with strict types enabled (`declare(strict_types=1)`).
 
+## Error Handling
+
+Uncaught exceptions thrown from callbacks registered to `Promise::when()` are forwarded to the `ErrorHandler` by `Promise` implementations. `ErrorHandler::set()` can be used to register a callable to handle these exceptions gracefully, e.g. by logging them. In case the handler throws again or is not set, an `E_USER_ERROR` is triggered. If a PHP error handler is set using `set_error_handler` and it throws, a short message is written to STDERR and the program exits with code `255`. Thus, it's RECOMMENDED to set an error handler and ensure it doesn't throw, especially if the PHP error handler is set up to convert errors to exceptions.
+
 ## Contributors
 
 * [Aaron Piotrowski](https://github.com/trowski)

composer.json

@@ -7,7 +7,7 @@
         "php": ">=5.4.0"
     },
     "require-dev": {
-        "phpunit/phpunit": "^4|^5",
+        "phpunit/phpunit": "^4.8.25|^5.3.3",
         "jakub-onderka/php-parallel-lint": "^0.9.2",
         "jakub-onderka/php-console-highlighter": "^0.3.2"
     },

src/Promise/ErrorHandler.php

@@ -55,22 +55,22 @@ 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'",
+            // 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)
+                \is_object($error) ? \get_class($error) : \gettype($error)
             ));
         }
 
         if (self::$callback === null) {
-            trigger_error(
-                "An exception has been thrown from an AsyncInterop\\Promise::when handler, but no handler has been"
-                . " registered via AsyncInterop\\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
+            self::triggerErrorHandler(
+                "An exception has been thrown from an AsyncInterop\\Promise::when() handler, but no handler has been"
+                . " registered via AsyncInterop\\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.",
+                $error
             );
 
             return;
@@ -79,19 +79,61 @@ public static function notify($error)
         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);
+            self::triggerErrorHandler(
+                "An exception has been thrown from the promise error handler registered to"
+                . " AsyncInterop\\Promise\\ErrorHandler::set().",
+                $e
+            );
+        } catch (\Throwable $e) {
+            self::triggerErrorHandler(
+                "An exception has been thrown from the promise error handler registered to"
+                . " AsyncInterop\\Promise\\ErrorHandler::set().",
+                $e
+            );
+        }
+    }
+
+    private static function triggerErrorHandler($message, $error) {
+        // We're already a last chance handler, throwing doesn't make sense, so use E_USER_ERROR.
+        // E_USER_ERROR is recoverable by a handler set via set_error_handler, which might throw, too.
+
+        try {
+            \trigger_error(
+                $message . "\n\n" . (string) $error,
+                E_USER_ERROR
+            );
+        } catch (\Exception $e) {
+            self::panic($e);
+        } catch (\Throwable $e) {
+            self::panic($e);
+        }
+    }
+
+    private static function panic($error) {
+        // The set error handler did throw or not exist, PHP's error handler threw, no chance to handle the error
+        // gracefully at this time. PANIC!
+
+        // Print error information to STDERR so the reason for the program abortion can be found, but do not expose
+        // exception message and trace, as they might contain sensitive information and we do not know whether STDERR
+        // might be available to an untrusted user.
+
+        // Exit with the same code as if PHP exits because of an uncaught exception.
+
+        try {
+            // fputs might fail due to a closed pipe
+            // no STDERR, because it doesn't exist on piped STDIN
+            // no finally, because PHP 5.4
+            \fputs(\fopen("php://stderr", "w"), \sprintf(
+                "Fatal error: Uncaught exception '%s' while trying to report a throwing AsyncInterop\\Promise::when()"
+                . " handler gracefully." . \PHP_EOL,
+                \get_class($error)
+            ));
+
+            exit(255);
+        } catch (\Exception $e) {
+            exit(255);
         } 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);
+            exit(255);
         }
     }
 }

test/phpt/error_handler_001.phpt

@@ -9,7 +9,7 @@ AsyncInterop\Promise\ErrorHandler::notify(new Exception);
 
 ?>
 --EXPECTF--
-Fatal error: An exception has been thrown from an AsyncInterop\Promise::when handler, but no handler has been registered via AsyncInterop\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.
+Fatal error: An exception has been thrown from an AsyncInterop\Promise::when() handler, but no handler has been registered via AsyncInterop\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:

test/phpt/error_handler_003.phpt

@@ -11,7 +11,7 @@ AsyncInterop\Promise\ErrorHandler::notify(new Exception);
 
 ?>
 --EXPECTF--
-Fatal error: An exception has been thrown from an AsyncInterop\Promise::when handler, but no handler has been registered via AsyncInterop\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.
+Fatal error: An exception has been thrown from an AsyncInterop\Promise::when() handler, but no handler has been registered via AsyncInterop\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:

test/phpt/error_handler_004.phpt

@@ -9,9 +9,9 @@ AsyncInterop\Promise\ErrorHandler::notify(42);
 
 ?>
 --EXPECTF--
-Fatal error: An exception has been thrown from an AsyncInterop\Promise::when handler, but no handler has been registered via AsyncInterop\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.
+Fatal error: An exception has been thrown from an AsyncInterop\Promise::when() handler, but no handler has been registered via AsyncInterop\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 AsyncInterop\Promise\ErrorHandler::notify with an invalid argument of type 'integer'%S in %s:%d
+%SException%SPromise implementation called AsyncInterop\Promise\ErrorHandler::notify() with an invalid argument of type 'integer'%S in %s:%d
 Stack trace:
 #0 %s(%d): AsyncInterop\Promise\ErrorHandler::notify(%S)
 #1 {main} in %s on line %d

test/phpt/error_handler_007.phpt

@@ -0,0 +1,34 @@
+--TEST--
+ErrorHandler::notify() fatals with a throwing error handler
+--SKIPIF--
+<?php
+
+if (!file_exists(__DIR__ . "/vendor/autoload.php")) {
+    die("Skipped: Cannot detect PHPUnit version, run tests from project root.");
+}
+
+// If the directory is not the root of the project, this will silently fail and execute the test below.
+require __DIR__ . "/vendor/autoload.php";
+
+$series = PHPUnit_Runner_Version::series();
+list($major, $minor) = explode(".", $series);
+
+if ($major < 5 || $major == 5 && $minor < 1) {
+    die("Skipped: PHPUnit 5.1 required to test for STDERR output.");
+}
+
+?>
+--FILE--
+<?php
+
+require __DIR__ . "/../../vendor/autoload.php";
+
+set_error_handler(function () {
+    throw new RuntimeException;
+});
+
+AsyncInterop\Promise\ErrorHandler::notify(new Exception);
+
+?>
+--EXPECT--
+Fatal error: Uncaught exception 'RuntimeException' while trying to report a throwing AsyncInterop\Promise::when() handler gracefully.