From c5bad9bc1ace2552b8a03bb1766593489411dbfb Mon Sep 17 00:00:00 2001 From: Aaron Piotrowski Date: Fri, 22 Nov 2024 17:47:31 -0600 Subject: [PATCH] Add ExceptionHandler interface and middleware (#375) --- src/DefaultExceptionHandler.php | 54 +++++++++++ src/Driver/Internal/AbstractHttpDriver.php | 92 ++++--------------- .../Internal/HttpDriverErrorHandler.php | 46 ++++++++++ src/ExceptionHandler.php | 13 +++ src/Middleware/ExceptionHandlerMiddleware.php | 37 ++++++++ src/SocketHttpServer.php | 11 +++ .../ExceptionHandlerMiddlewareTest.php | 59 ++++++++++++ 7 files changed, 239 insertions(+), 73 deletions(-) create mode 100644 src/DefaultExceptionHandler.php create mode 100644 src/Driver/Internal/HttpDriverErrorHandler.php create mode 100644 src/ExceptionHandler.php create mode 100644 src/Middleware/ExceptionHandlerMiddleware.php create mode 100644 test/Middleware/ExceptionHandlerMiddlewareTest.php diff --git a/src/DefaultExceptionHandler.php b/src/DefaultExceptionHandler.php new file mode 100644 index 00000000..4255313b --- /dev/null +++ b/src/DefaultExceptionHandler.php @@ -0,0 +1,54 @@ +getClient(); + $method = $request->getMethod(); + $uri = (string) $request->getUri(); + $protocolVersion = $request->getProtocolVersion(); + $local = $client->getLocalAddress()->toString(); + $remote = $client->getRemoteAddress()->toString(); + + $this->logger->error( + \sprintf( + "Unexpected %s with message '%s' thrown from %s:%d when handling request: %s %s HTTP/%s %s on %s", + $exception::class, + $exception->getMessage(), + $exception->getFile(), + $exception->getLine(), + $method, + $uri, + $protocolVersion, + $remote, + $local, + ), + [ + 'exception' => $exception, + 'method' => $method, + 'uri' => $uri, + 'protocolVersion' => $protocolVersion, + 'local' => $local, + 'remote' => $remote, + ], + ); + + return $this->errorHandler->handleError(HttpStatus::INTERNAL_SERVER_ERROR, request: $request); + } +} diff --git a/src/Driver/Internal/AbstractHttpDriver.php b/src/Driver/Internal/AbstractHttpDriver.php index ee4f274c..15749ad0 100644 --- a/src/Driver/Internal/AbstractHttpDriver.php +++ b/src/Driver/Internal/AbstractHttpDriver.php @@ -2,41 +2,42 @@ namespace Amp\Http\Server\Driver\Internal; -use Amp\Http\HttpStatus; use Amp\Http\Server\ClientException; -use Amp\Http\Server\DefaultErrorHandler; +use Amp\Http\Server\DefaultExceptionHandler; use Amp\Http\Server\Driver\HttpDriver; use Amp\Http\Server\ErrorHandler; +use Amp\Http\Server\ExceptionHandler; use Amp\Http\Server\HttpErrorException; +use Amp\Http\Server\Middleware\ExceptionHandlerMiddleware; use Amp\Http\Server\Request; use Amp\Http\Server\RequestHandler; use Amp\Http\Server\Response; -use Psr\Log\LoggerInterface; +use Psr\Log\LoggerInterface as PsrLogger; /** @internal */ abstract class AbstractHttpDriver implements HttpDriver { private static ?TimeoutQueue $timeoutQueue = null; - private static ?ErrorHandler $defaultErrorHandler = null; final protected static function getTimeoutQueue(): TimeoutQueue { return self::$timeoutQueue ??= new TimeoutQueue(); } - private static function getDefaultErrorHandler(): ErrorHandler - { - return self::$defaultErrorHandler ??= new DefaultErrorHandler(); - } + private readonly DefaultExceptionHandler $exceptionHandler; private int $pendingRequestHandlerCount = 0; private int $pendingResponseCount = 0; + protected readonly ErrorHandler $errorHandler; + protected function __construct( protected readonly RequestHandler $requestHandler, - protected readonly ErrorHandler $errorHandler, - protected readonly LoggerInterface $logger, + ErrorHandler $errorHandler, + protected readonly PsrLogger $logger, ) { + $this->errorHandler = new HttpDriverErrorHandler($errorHandler, $this->logger); + $this->exceptionHandler = new DefaultExceptionHandler($this->errorHandler, $this->logger); } /** @@ -55,9 +56,14 @@ final protected function handleRequest(Request $request): void } catch (ClientException $exception) { throw $exception; } catch (HttpErrorException $exception) { - $response = $this->handleError($exception->getStatus(), $exception->getReason(), $request); + $response = $this->errorHandler->handleError($exception->getStatus(), $exception->getReason(), $request); } catch (\Throwable $exception) { - $response = $this->handleInternalServerError($request, $exception); + /** + * This catch is not designed to be a general-purpose exception handler, rather a last-resort to write to + * the logger if the application has failed to handle an exception thrown from a {@see RequestHandler}. + * Instead of relying on this handler, use {@see ExceptionHandler} and {@see ExceptionHandlerMiddleware}. + */ + $response = $this->exceptionHandler->handleException($request, $exception); } finally { $this->pendingRequestHandlerCount--; } @@ -80,67 +86,7 @@ final protected function handleRequest(Request $request): void } /** - * Write the given response to the client using the write callback provided to `setup()`. + * Write the given response to the client. */ abstract protected function write(Request $request, Response $response): void; - - /** - * Used if an exception is thrown from a request handler. - */ - private function handleInternalServerError(Request $request, \Throwable $exception): Response - { - $status = HttpStatus::INTERNAL_SERVER_ERROR; - - $client = $request->getClient(); - $method = $request->getMethod(); - $uri = (string) $request->getUri(); - $protocolVersion = $request->getProtocolVersion(); - $local = $client->getLocalAddress()->toString(); - $remote = $client->getRemoteAddress()->toString(); - - $this->logger->error( - \sprintf( - "Unexpected %s with message '%s' thrown from %s:%d when handling request: %s %s HTTP/%s %s on %s", - $exception::class, - $exception->getMessage(), - $exception->getFile(), - $exception->getLine(), - $method, - $uri, - $protocolVersion, - $remote, - $local, - ), - [ - 'exception' => $exception, - 'method' => $method, - 'uri' => $uri, - 'protocolVersion' => $protocolVersion, - 'local' => $local, - 'remote' => $remote, - ], - ); - - return $this->handleError($status, null, $request); - } - - private function handleError(int $status, ?string $reason, Request $request): Response - { - try { - return $this->errorHandler->handleError($status, $reason, $request); - } catch (\Throwable $exception) { - // If the error handler throws, fallback to returning the default error page. - $this->logger->error( - \sprintf( - "Unexpected %s thrown from %s::handleError(), falling back to default error handler.", - $exception::class, - $this->errorHandler::class, - ), - ['exception' => $exception], - ); - - // The default error handler will never throw, otherwise there's a bug - return self::getDefaultErrorHandler()->handleError($status, null, $request); - } - } } diff --git a/src/Driver/Internal/HttpDriverErrorHandler.php b/src/Driver/Internal/HttpDriverErrorHandler.php new file mode 100644 index 00000000..672c3053 --- /dev/null +++ b/src/Driver/Internal/HttpDriverErrorHandler.php @@ -0,0 +1,46 @@ +errorHandler->handleError($status, $reason, $request); + } catch (\Throwable $exception) { + // If the error handler throws, fallback to returning the default error page. + $this->logger->error( + \sprintf( + "Unexpected %s thrown from %s::handleError(), falling back to default error handler.", + $exception::class, + $this->errorHandler::class, + ), + ['exception' => $exception], + ); + + // The default error handler will never throw, otherwise there's a bug + return self::getDefaultErrorHandler()->handleError($status, null, $request); + } + } +} diff --git a/src/ExceptionHandler.php b/src/ExceptionHandler.php new file mode 100644 index 00000000..3c4a1a86 --- /dev/null +++ b/src/ExceptionHandler.php @@ -0,0 +1,13 @@ +handleRequest($request); + } catch (ClientException|HttpErrorException $exception) { + // Rethrow our special client exception or HTTP error exception. These exceptions have special meaning + // to the HTTP driver, so will be handled differently from other uncaught exceptions from the request + // handler. + throw $exception; + } catch (\Throwable $exception) { + return $this->exceptionHandler->handleException($request, $exception); + } + } +} diff --git a/src/SocketHttpServer.php b/src/SocketHttpServer.php index ea6f57ff..03aa7741 100644 --- a/src/SocketHttpServer.php +++ b/src/SocketHttpServer.php @@ -14,6 +14,7 @@ use Amp\Http\Server\Middleware\AllowedMethodsMiddleware; use Amp\Http\Server\Middleware\CompressionMiddleware; use Amp\Http\Server\Middleware\ConcurrencyLimitingMiddleware; +use Amp\Http\Server\Middleware\ExceptionHandlerMiddleware; use Amp\Http\Server\Middleware\ForwardedHeaderType; use Amp\Http\Server\Middleware\ForwardedMiddleware; use Amp\Socket\BindContext; @@ -70,6 +71,7 @@ public static function createForDirectAccess( ?int $concurrencyLimit = self::DEFAULT_CONCURRENCY_LIMIT, ?array $allowedMethods = AllowedMethodsMiddleware::DEFAULT_ALLOWED_METHODS, ?HttpDriverFactory $httpDriverFactory = null, + ?ExceptionHandler $exceptionHandler = null, ): self { $serverSocketFactory = new ConnectionLimitingServerSocketFactory(new LocalSemaphore($connectionLimit)); @@ -88,6 +90,10 @@ public static function createForDirectAccess( $middleware = []; + if ($exceptionHandler) { + $middleware[] = new ExceptionHandlerMiddleware($exceptionHandler); + } + if ($concurrencyLimit !== null) { $logger->notice(\sprintf("Request concurrency limited to %s simultaneous requests", $concurrencyLimit)); $middleware[] = new ConcurrencyLimitingMiddleware($concurrencyLimit); @@ -126,9 +132,14 @@ public static function createForBehindProxy( ?int $concurrencyLimit = self::DEFAULT_CONCURRENCY_LIMIT, ?array $allowedMethods = AllowedMethodsMiddleware::DEFAULT_ALLOWED_METHODS, ?HttpDriverFactory $httpDriverFactory = null, + ?ExceptionHandler $exceptionHandler = null, ): self { $middleware = []; + if ($exceptionHandler) { + $middleware[] = new ExceptionHandlerMiddleware($exceptionHandler); + } + if ($concurrencyLimit !== null) { $middleware[] = new ConcurrencyLimitingMiddleware($concurrencyLimit); } diff --git a/test/Middleware/ExceptionHandlerMiddlewareTest.php b/test/Middleware/ExceptionHandlerMiddlewareTest.php new file mode 100644 index 00000000..f9ca4f3a --- /dev/null +++ b/test/Middleware/ExceptionHandlerMiddlewareTest.php @@ -0,0 +1,59 @@ +createMock(Client::class), 'GET', Http::createFromString('/')); + + $requestHandler = $this->createMock(RequestHandler::class); + $requestHandler->expects(self::once()) + ->method('handleRequest') + ->with($request) + ->willThrowException($exception); + + $middleware = new ExceptionHandlerMiddleware($exceptionHandler); + + return $middleware->handleRequest($request, $requestHandler); + } + + public function testUncaughtException(): void + { + $exception = new TestException(); + + $exceptionHandler = $this->createMock(ExceptionHandler::class); + $exceptionHandler->expects(self::once()) + ->method('handleException') + ->with(self::isInstanceOf(Request::class), $exception) + ->willReturn(new Response(HttpStatus::INTERNAL_SERVER_ERROR)); + + $this->setupAndInvokeMiddleware($exceptionHandler, $exception); + } + + public function testHttpErrorException(): void + { + $exception = new HttpErrorException(HttpStatus::BAD_REQUEST); + + $exceptionHandler = $this->createMock(ExceptionHandler::class); + $exceptionHandler->expects(self::never()) + ->method('handleException'); + + $this->expectExceptionObject($exception); + + $this->setupAndInvokeMiddleware($exceptionHandler, $exception); + } +}