From f27cc60eb113e996b9f412bfa067c7dd6c71f73b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Michael=20Vo=C5=99=C3=AD=C5=A1ek?= Date: Mon, 25 Dec 2023 18:49:54 +0100 Subject: [PATCH] Handle fatal error as exception (#2136) --- composer.json | 2 +- demos/_unit-test/fatal-error.php | 58 +++++++++++++++++++++ phpstan.neon.dist | 6 +++ src/App.php | 88 +++++++++++++++++++++++--------- tests/CallbackTest.php | 13 +++-- tests/DemosHttpTest.php | 31 +++++++++++ tests/DemosTest.php | 65 +++++++++++------------ 7 files changed, 197 insertions(+), 66 deletions(-) create mode 100644 demos/_unit-test/fatal-error.php diff --git a/composer.json b/composer.json index 470797856a..0d1d9f6e95 100644 --- a/composer.json +++ b/composer.json @@ -68,9 +68,9 @@ }, "require-dev": { "atk4/behat-mink-selenium2-driver": "^1.6.2", - "atk4/ergebnis-phpunit-slow-test-detector": "^2.4", "behat/mink-extension": "^2.3.1", "ergebnis/composer-normalize": "^2.13", + "ergebnis/phpunit-slow-test-detector": "^2.9", "friendsofphp/php-cs-fixer": "^3.0", "fzaninotto/faker": "^1.6", "guzzlehttp/guzzle": "^7.3", diff --git a/demos/_unit-test/fatal-error.php b/demos/_unit-test/fatal-error.php new file mode 100644 index 0000000000..e60d7214f4 --- /dev/null +++ b/demos/_unit-test/fatal-error.php @@ -0,0 +1,58 @@ +onHook(App::HOOK_BEFORE_RENDER, static function () use (&$fx) { + if ($fx !== null) { + try { + $fx(); + } finally { + $fx = null; + } + } + }); +}; + +$type = $app->tryGetRequestQueryParam('type'); +if ($type === 'oom') { + ini_set('memory_limit', '16M'); + + $str = ''; + for ($i = 0; $i < 1024; ++$i) { + $str .= random_bytes(16 * 1024); + } +} elseif ($type === 'time-limit') { + set_time_limit(1); + + $str = ''; + $t = microtime(true); + while (microtime(true) - $t < 1.5) { + $str = md5($str); + } +} elseif ($type === 'compile-error') { + // E_COMPILE_ERROR + // https://github.com/php/php-src/blob/php-8.3.0/Zend/zend_compile.c#L7459 + // https://github.com/php/php-src/issues/9333 + eval('class Cl { function foo(); }'); +} elseif ($type === 'compile-warning') { + // E_COMPILE_WARNING + // https://github.com/php/php-src/blob/php-8.3.0/Zend/zend_compile.c#L6366 + eval('declare(x=1);'); +} elseif ($type === 'exception-in-shutdown') { + $runOnShutdownFx(static function () { + throw new \Exception('Exception from shutdown'); + }); +} elseif ($type === 'warning-in-shutdown') { + $runOnShutdownFx(static function () { + trigger_error('Warning from shutdown'); + }); +} diff --git a/phpstan.neon.dist b/phpstan.neon.dist index 8ff5c6b53b..a6f871e665 100644 --- a/phpstan.neon.dist +++ b/phpstan.neon.dist @@ -18,6 +18,12 @@ parameters: - '~^Only booleans are allowed in .+, .+ given( on the (left|right) side)?\.~' - '~^Variable (static )?(property access|method call) on .+\.~' + # https://github.com/phpstan/phpstan/issues/10337 + - + path: 'src/App.php' + message: '~^Call to method Atk4\\Ui\\App::callExit\(\) with true will always evaluate to true\.$~' + count: 2 + # TODO these rules are generated, this ignores should be fixed in the code # for level = 2 - diff --git a/src/App.php b/src/App.php index d65fa8d824..06ad5a537b 100644 --- a/src/App.php +++ b/src/App.php @@ -36,11 +36,15 @@ class App use DynamicMethodTrait; use HookTrait; - private const UNSUPPRESSEABLE_ERROR_LEVELS = \PHP_MAJOR_VERSION >= 8 ? (\E_ERROR | \E_PARSE | \E_CORE_ERROR | \E_COMPILE_ERROR | \E_USER_ERROR | \E_RECOVERABLE_ERROR) : 0; + private const UNHANDLEABLE_ERROR_LEVELS = \E_ERROR | \E_PARSE | \E_CORE_ERROR | \E_CORE_WARNING | \E_COMPILE_ERROR | \E_COMPILE_WARNING; + private const UNSUPPRESSIBLE_ERROR_LEVELS = \PHP_MAJOR_VERSION >= 8 ? (\E_ERROR | \E_PARSE | \E_CORE_ERROR | \E_COMPILE_ERROR | \E_USER_ERROR | \E_RECOVERABLE_ERROR) : 0; public const HOOK_BEFORE_EXIT = self::class . '@beforeExit'; public const HOOK_BEFORE_RENDER = self::class . '@beforeRender'; + private static ?string $shutdownReservedMemory; // @phpstan-ignore-line + private static ?int $errorReportingLevel = null; + /** @var array|false Location where to load JS/CSS files */ public $cdn = [ 'atk' => '/public', @@ -183,8 +187,13 @@ public function __construct(array $defaults = []) // set our exception handler if ($this->catchExceptions) { set_exception_handler(\Closure::fromCallable([$this, 'caughtException'])); - set_error_handler(static function (int $severity, string $msg, string $file, int $line): bool { - if ((error_reporting() & ~self::UNSUPPRESSEABLE_ERROR_LEVELS) === 0) { + + $createErrorExceptionArgsFx = static function (int $severity, string $msg, string $file, int $line) { + return [$msg, 0, $severity, $file, $line]; + }; + + set_error_handler(function (int $severity, string $msg, string $file, int $line) use ($createErrorExceptionArgsFx): bool { + if ((error_reporting() & ~self::UNSUPPRESSIBLE_ERROR_LEVELS) === 0) { $isFirstFrame = true; foreach (array_slice(debug_backtrace(\DEBUG_BACKTRACE_IGNORE_ARGS, 10), 1) as $frame) { // allow to suppress any warning outside Atk4 @@ -203,8 +212,17 @@ public function __construct(array $defaults = []) } } - throw new \ErrorException($msg, 0, $severity, $file, $line); + $this->throwOrCaughtExceptionIfInShutdown(new \ErrorException(...$createErrorExceptionArgsFx($severity, $msg, $file, $line))); + }); + + register_shutdown_function(function () use ($createErrorExceptionArgsFx): void { + $error = error_get_last(); + if ($error !== null && ($error['type'] & self::UNHANDLEABLE_ERROR_LEVELS) !== 0) { + $this->throwOrCaughtExceptionIfInShutdown(new \ErrorException(...$createErrorExceptionArgsFx($error['type'], $error['message'], $error['file'], $error['line']))); + } }); + self::$errorReportingLevel = error_reporting(); + error_reporting(self::$errorReportingLevel & ~self::UNHANDLEABLE_ERROR_LEVELS); http_response_code(500); header('Content-Type: text/plain'); @@ -254,33 +272,46 @@ protected function setupTemplateDirs(): void $this->templateDir[] = dirname(__DIR__) . '/template'; } - protected function callBeforeExit(): void + /** + * @return ($calledFromShutdownHandler is true ? void : never) + */ + public function callExit(bool $calledFromShutdownHandler = false): void { if (!$this->exitCalled) { $this->exitCalled = true; $this->hook(self::HOOK_BEFORE_EXIT); } + + if (!$calledFromShutdownHandler) { + if (!$this->callExit) { + // case process is not in shutdown mode + // App as already done everything + // App need to stop output + // set_handler to catch/trap any exception + set_exception_handler(static function (\Throwable $t): void {}); + + // raise exception to be trapped and stop execution + throw new ExitApplicationError(); + } + + exit; + } } /** * @return never */ - public function callExit(): void + private function throwOrCaughtExceptionIfInShutdown(\Throwable $exception): void { - $this->callBeforeExit(); - - if (!$this->callExit) { - // case process is not in shutdown mode - // App as already done everything - // App need to stop output - // set_handler to catch/trap any exception - set_exception_handler(static function (\Throwable $t): void {}); + if (\PHP_VERSION_ID < 80300 && self::$shutdownReservedMemory === null) { + // remove method once PHP 8.2 support is dropped + // https://github.com/php/php-src/issues/10695 + $this->caughtException($exception); - // raise exception to be trapped and stop execution - throw new ExitApplicationError(); + exit(1); } - exit; + throw $exception; } protected function caughtException(\Throwable $exception): void @@ -318,8 +349,7 @@ protected function caughtException(\Throwable $exception): void } // process is already in shutdown because of uncaught exception - // no need of call exit function - $this->callBeforeExit(); + $this->callExit(true); } public function getRequest(): ServerRequestInterface @@ -1098,7 +1128,7 @@ public function encodeJson($data, bool $forceObject = false): string * $app->initLayout([Layout\Centered::class]); * $app->layout->template->dangerouslySetHtml('Content', $e->getHtml()); * $app->run(); - * $app->callBeforeExit(); + * $app->callExit(); */ public function renderExceptionHtml(\Throwable $exception): string { @@ -1111,15 +1141,14 @@ protected function setupAlwaysRun(): void if (!$this->runCalled) { try { $this->run(); + $this->callExit(true); } catch (ExitApplicationError $e) { - // let the process continue and terminate using self::callExit() below + // continue shutdown } catch (\Throwable $e) { // set_exception_handler does not work in shutdown // https://github.com/php/php-src/issues/10695 $this->caughtException($e); } - - $this->callBeforeExit(); } }); } @@ -1232,3 +1261,16 @@ private function outputResponseJson($data): void $this->outputResponse($data); } } + +\Closure::bind(static function (): void { + // reserve 1 MB of memory for shutdown + App::$shutdownReservedMemory = str_repeat(str_repeat('shutdownReserved', 256), 256); + + register_shutdown_function(static function (): void { + App::$shutdownReservedMemory = null; + + if (App::$errorReportingLevel !== null) { + error_reporting(error_reporting() | (App::$errorReportingLevel & App::UNHANDLEABLE_ERROR_LEVELS)); + } + }); +}, null, App::class)(); diff --git a/tests/CallbackTest.php b/tests/CallbackTest.php index fa34491653..10835170dc 100644 --- a/tests/CallbackTest.php +++ b/tests/CallbackTest.php @@ -31,8 +31,7 @@ class CallbackTest extends TestCase triggerCallback as private _triggerCallback; } - /** @var string */ - private $regexHtmlDoctype = '~^\s*expectOutputRegex($this->regexHtmlDoctype); + $this->expectOutputRegex(self::$regexHtml); $cb->getApp()->run(); self::assertSame(34, $var); } @@ -195,7 +194,7 @@ public function testCallbackLaterNested(): void self::assertNull($var); - $this->expectOutputRegex($this->regexHtmlDoctype); + $this->expectOutputRegex(self::$regexHtml); $cb->getApp()->run(); self::assertSame(134, $var); } @@ -214,7 +213,7 @@ public function testCallbackLaterNotFiring(): void self::assertNull($var); - $this->expectOutputRegex($this->regexHtmlDoctype); + $this->expectOutputRegex(self::$regexHtml); $app->run(); self::assertNull($var); // @phpstan-ignore-line } @@ -235,7 +234,7 @@ public function testVirtualPage(): void $var = 25; }); - $this->expectOutputRegex($this->regexHtmlDoctype); + $this->expectOutputRegex(self::$regexHtml); $vp->getApp()->run(); self::assertSame(25, $var); } @@ -256,7 +255,7 @@ public function testVirtualPageCustomUrlTrigger(): void $var = 25; }); - $this->expectOutputRegex($this->regexHtmlDoctype); + $this->expectOutputRegex(self::$regexHtml); $vp->getApp()->run(); self::assertSame(25, $var); } diff --git a/tests/DemosHttpTest.php b/tests/DemosHttpTest.php index 7709c440d8..79c8411e52 100644 --- a/tests/DemosHttpTest.php +++ b/tests/DemosHttpTest.php @@ -123,6 +123,37 @@ protected function getPathWithAppVars(string $path): string return parent::getPathWithAppVars($path); } + #[\Override] + public static function provideDemoCallbackErrorCases(): iterable + { + yield from parent::provideDemoCallbackErrorCases(); + + yield [ + '_unit-test/fatal-error.php?type=oom', + 'Allowed memory size of 16777216 bytes exhausted', + ]; + yield [ + '_unit-test/fatal-error.php?type=time-limit', + 'Maximum execution time of 1 second exceeded', + ]; + yield [ + '_unit-test/fatal-error.php?type=compile-error', + 'Non-abstract method Cl::foo() must contain body', + ]; + yield [ + '_unit-test/fatal-error.php?type=compile-warning', + 'Unsupported declare \'x\'', + ]; + yield [ + '_unit-test/fatal-error.php?type=exception-in-shutdown', + 'Exception from shutdown', + ]; + yield [ + '_unit-test/fatal-error.php?type=warning-in-shutdown', + 'Warning from shutdown', + ]; + } + /** * @dataProvider provideDemoLateOutputErrorCases */ diff --git a/tests/DemosTest.php b/tests/DemosTest.php index b99d94d325..dc5bf32864 100644 --- a/tests/DemosTest.php +++ b/tests/DemosTest.php @@ -31,6 +31,16 @@ class DemosTest extends TestCase private static ?Persistence $_db = null; + protected static string $regexHtml = '~^\s*$~s'; + protected static string $regexJson = '~^(?\s*(?: + (?-?(?=[1-9]|0(?!\d))\d+(\.\d+)?(E[+-]?\d+)?) + |(?true|false|null) + |(?"([^"\\\\]*|\\\\["\\\\bfnrt/]|\\\\u[0-9a-f]{4})*") + |(?\[(?:(?&json)(?:,(?&json))*|\s*)\]) + |(?\{(?:(?\s*(?&string)\s*:(?&json))(?:,(?&pair))*|\s*)\}) + )\s*)$~six'; + protected static string $regexSseLine = '~^(id|event|data).*$~s'; + #[\Override] public static function setUpBeforeClass(): void { @@ -120,7 +130,7 @@ protected function createTestingApp(): App { $app = new class(['callExit' => false, 'catchExceptions' => false, 'alwaysRun' => false]) extends App { #[\Override] - public function callExit(): void + public function callExit(bool $calledFromShutdownHandler = false): void { throw new DemosTestExitError(); } @@ -230,24 +240,6 @@ protected function getPathWithAppVars(string $path): string return 'demos/' . $path; } - /** @var string */ - protected $regexHtml = '~^\s*getResponseFromRequest($path); self::assertSame(200, $response->getStatusCode()); - self::assertMatchesRegularExpression($this->regexHtml, $response->getBody()->getContents()); + self::assertMatchesRegularExpression(self::$regexHtml, $response->getBody()->getContents()); } public function testDemoResponseError(): void @@ -332,7 +324,7 @@ public function testDemoGet(string $path): void $response = $this->getResponseFromRequest($path); self::assertSame(200, $response->getStatusCode()); self::assertSame('text/html', preg_replace('~;\s*charset=.+$~', '', $response->getHeaderLine('Content-Type'))); - self::assertMatchesRegularExpression($this->regexHtml, $response->getBody()->getContents()); + self::assertMatchesRegularExpression(self::$regexHtml, $response->getBody()->getContents()); } public function testHugeOutputStream(): void @@ -380,14 +372,14 @@ public function testWizard(): void ); self::assertSame(200, $response->getStatusCode()); - self::assertMatchesRegularExpression($this->regexJson, $response->getBody()->getContents()); + self::assertMatchesRegularExpression(self::$regexJson, $response->getBody()->getContents()); $response = $this->getResponseFromRequest('interactive/wizard.php?atk_admin_wizard=2&name=Country'); self::assertSame(200, $response->getStatusCode()); - self::assertMatchesRegularExpression($this->regexHtml, $response->getBody()->getContents()); + self::assertMatchesRegularExpression(self::$regexHtml, $response->getBody()->getContents()); } - public static function provideDemoAssertJsonResponseCases(): iterable + public static function provideDemoJsonResponseCases(): iterable { // simple reload yield ['_unit-test/reload.php?__atk_reload=reload']; @@ -401,9 +393,9 @@ public static function provideDemoAssertJsonResponseCases(): iterable /** * Test reload and loader callback. * - * @dataProvider provideDemoAssertJsonResponseCases + * @dataProvider provideDemoJsonResponseCases */ - public function testDemoAssertJsonResponse(string $path, string $expectedExceptionMessage = null): void + public function testDemoJsonResponse(string $path, string $expectedExceptionMessage = null): void { if (static::class === self::class) { if ($expectedExceptionMessage !== null) { @@ -421,14 +413,14 @@ public function testDemoAssertJsonResponse(string $path, string $expectedExcepti self::assertSame(200, $response->getStatusCode()); self::assertSame('application/json', preg_replace('~;\s*charset=.+$~', '', $response->getHeaderLine('Content-Type'))); $responseBodyStr = $response->getBody()->getContents(); - self::assertMatchesRegularExpression($this->regexJson, $responseBodyStr); + self::assertMatchesRegularExpression(self::$regexJson, $responseBodyStr); self::assertStringNotContainsString(preg_replace('~.+\\\\~', '', UnhandledCallbackExceptionError::class), $responseBodyStr); if ($expectedExceptionMessage !== null) { self::assertStringContainsString($expectedExceptionMessage, $responseBodyStr); } } - public static function provideDemoAssertSseResponseCases(): iterable + public static function provideDemoSseResponseCases(): iterable { yield ['_unit-test/sse.php?' . Callback::URL_QUERY_TRIGGER_PREFIX . 'see_test=ajax&' . Callback::URL_QUERY_TARGET . '=1&__atk_sse=1']; yield ['_unit-test/console.php?' . Callback::URL_QUERY_TRIGGER_PREFIX . 'console_test=ajax&' . Callback::URL_QUERY_TARGET . '=1&__atk_sse=1']; @@ -439,9 +431,9 @@ public static function provideDemoAssertSseResponseCases(): iterable /** * Test JsSse and Console. * - * @dataProvider provideDemoAssertSseResponseCases + * @dataProvider provideDemoSseResponseCases */ - public function testDemoAssertSseResponse(string $path): void + public function testDemoSseResponse(string $path): void { // this test requires SessionTrait, more precisely session_start() which we do not support in non-HTTP testing if (static::class === self::class) { @@ -458,7 +450,7 @@ public function testDemoAssertSseResponse(string $path): void // check SSE Syntax self::assertGreaterThan(0, count($outputLines)); foreach ($outputLines as $index => $line) { - preg_match_all($this->regexSse, $line, $matchesAll); + preg_match_all(self::$regexSseLine, $line, $matchesAll); self::assertSame( $line, implode('', $matchesAll[0] ?? ['error']), @@ -467,7 +459,7 @@ public function testDemoAssertSseResponse(string $path): void } } - public static function provideDemoAssertJsonResponsePostCases(): iterable + public static function provideDemoJsonResponsePostCases(): iterable { yield [ '_unit-test/post.php?' . Callback::URL_QUERY_TRIGGER_PREFIX . 'test_submit=ajax&' . Callback::URL_QUERY_TARGET . '=test_submit', @@ -476,17 +468,19 @@ public static function provideDemoAssertJsonResponsePostCases(): iterable } /** - * @dataProvider provideDemoAssertJsonResponsePostCases + * @dataProvider provideDemoJsonResponsePostCases */ - public function testDemoAssertJsonResponsePost(string $path, array $postData): void + public function testDemoJsonResponsePost(string $path, array $postData): void { $response = $this->getResponseFromRequest($path, ['form_params' => $postData]); self::assertSame(200, $response->getStatusCode()); - self::assertMatchesRegularExpression($this->regexJson, $response->getBody()->getContents()); + self::assertMatchesRegularExpression(self::$regexJson, $response->getBody()->getContents()); } /** * @dataProvider provideDemoCallbackErrorCases + * + * @slowThreshold 1500 */ public function testDemoCallbackError(string $path, string $expectedExceptionMessage, array $options = []): void { @@ -500,6 +494,7 @@ public function testDemoCallbackError(string $path, string $expectedExceptionMes self::assertSame('text/html', preg_replace('~;\s*charset=.+$~', '', $response->getHeaderLine('Content-Type'))); self::assertSame('no-store', $response->getHeaderLine('Cache-Control')); $responseBodyStr = $response->getBody()->getContents(); + self::assertMatchesRegularExpression(self::$regexHtml, $responseBodyStr); self::assertStringNotContainsString(preg_replace('~.+\\\\~', '', UnhandledCallbackExceptionError::class), $responseBodyStr); self::assertStringContainsString($expectedExceptionMessage, $responseBodyStr); }