Add ExceptionHandler interface and middleware (#375)

src/DefaultExceptionHandler.php

@@ -0,0 +1,54 @@
+<?php declare(strict_types=1);
+
+namespace Amp\Http\Server;
+
+use Amp\Http\HttpStatus;
+use Psr\Log\LoggerInterface as PsrLogger;
+
+/**
+ * Simple exception handler that writes a message to the logger and returns an error page generated by the provided
+ * {@see ErrorHandler}.
+ */
+final class DefaultExceptionHandler implements ExceptionHandler
+{
+    public function __construct(
+        private readonly ErrorHandler $errorHandler,
+        private readonly PsrLogger $logger,
+    ) {
+    }
+
+    public function handleException(Request $request, \Throwable $exception): Response
+    {
+        $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->errorHandler->handleError(HttpStatus::INTERNAL_SERVER_ERROR, request: $request);
+    }
+}

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);
-        }
-    }
 }

src/Driver/Internal/HttpDriverErrorHandler.php

@@ -0,0 +1,46 @@
+<?php declare(strict_types=1);
+
+namespace Amp\Http\Server\Driver\Internal;
+
+use Amp\Http\Server\DefaultErrorHandler;
+use Amp\Http\Server\ErrorHandler;
+use Amp\Http\Server\Request;
+use Amp\Http\Server\Response;
+use Psr\Log\LoggerInterface as PsrLogger;
+
+/** @internal */
+final class HttpDriverErrorHandler implements ErrorHandler
+{
+    private static ?DefaultErrorHandler $defaultErrorHandler = null;
+
+    private static function getDefaultErrorHandler(): ErrorHandler
+    {
+        return self::$defaultErrorHandler ??= new DefaultErrorHandler();
+    }
+
+    public function __construct(
+        private readonly ErrorHandler $errorHandler,
+        private readonly PsrLogger $logger,
+    ) {
+    }
+
+    public function handleError(int $status, ?string $reason = null, ?Request $request = null): 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);
+        }
+    }
+}

src/ExceptionHandler.php

@@ -0,0 +1,13 @@
+<?php declare(strict_types=1);
+
+namespace Amp\Http\Server;
+
+use Amp\Http\Server\Middleware\ExceptionHandlerMiddleware;
+
+interface ExceptionHandler
+{
+    /**
+     * Handles an uncaught exception from the {@see RequestHandler} wrapped with {@see ExceptionHandlerMiddleware}.
+     */
+    public function handleException(Request $request, \Throwable $exception): Response;
+}

src/Middleware/ExceptionHandlerMiddleware.php

@@ -0,0 +1,37 @@
+<?php declare(strict_types=1);
+
+namespace Amp\Http\Server\Middleware;
+
+use Amp\Http\Server\ClientException;
+use Amp\Http\Server\ExceptionHandler;
+use Amp\Http\Server\HttpErrorException;
+use Amp\Http\Server\Middleware;
+use Amp\Http\Server\Request;
+use Amp\Http\Server\RequestHandler;
+use Amp\Http\Server\Response;
+
+/**
+ * This middleware catches exceptions from the wrapped {@see RequestHandler}, delegating handling of the exception to
+ * the provided instance of {@see ExceptionHandler}. Generally it is recommended that this middleware be first in the
+ * middleware stack so it is able to catch any exception from another middleware or request handler.
+ */
+final class ExceptionHandlerMiddleware implements Middleware
+{
+    public function __construct(private readonly ExceptionHandler $exceptionHandler)
+    {
+    }
+
+    public function handleRequest(Request $request, RequestHandler $requestHandler): Response
+    {
+        try {
+            return $requestHandler->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);
+        }
+    }
+}

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);
         }

test/Middleware/ExceptionHandlerMiddlewareTest.php

@@ -0,0 +1,59 @@
+<?php declare(strict_types=1);
+
+namespace Amp\Http\Server\Test\Middleware;
+
+use Amp\Http\HttpStatus;
+use Amp\Http\Server\Driver\Client;
+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 Amp\PHPUnit\AsyncTestCase;
+use Amp\PHPUnit\TestException;
+use League\Uri\Http;
+
+class ExceptionHandlerMiddlewareTest extends AsyncTestCase
+{
+    private function setupAndInvokeMiddleware(ExceptionHandler $exceptionHandler, \Throwable $exception): Response
+    {
+        $request = new Request($this->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);
+    }
+}