Cannot add a custom handler for HTTPExceptions with status_code=500

[clickonben]

I found this through FastAPI but the code causing the problem appears to be in Starlette.

from fastapi import FastAPI, HTTPException

app = FastAPI()

@app.exception_handler(500)
def custom_exception_handler(request, exc):
    print(f"Oops! An HTTP error occurred: {exc}")
    return JSONResponse({"detail":"Internal Server Error"}, status_code=500)


@app.get("/")
def read_root():
    raise HTTPException(status_code=500, detail="Internal Server Error")

This exception handler never gets called. When I dug into it I found the problem was this:

def build_middleware_stack(self) -> ASGIApp:
        debug = self.debug
        error_handler = None
        exception_handlers: dict[
            typing.Any, typing.Callable[[Request, Exception], Response]
        ] = {}

        for key, value in self.exception_handlers.items():
            if key in (500, Exception):
                error_handler = value
            else:
                exception_handlers[key] = value

        middleware = (
            [Middleware(ServerErrorMiddleware, handler=error_handler, debug=debug)]
            + self.user_middleware
            + [
                Middleware(
                    ExceptionMiddleware, handlers=exception_handlers, debug=debug
                )
            ]
        )

        app = self.router
        for cls, args, kwargs in reversed(middleware):
            app = cls(app=app, *args, **kwargs)
        return app

in this file https://github.com/encode/starlette/blob/master/starlette/applications.py

I don't understand why it is doing this:

for key, value in self.exception_handlers.items():
    if key in (500, Exception):
        error_handler = value
    else:
        exception_handlers[key] = value

because if I change it to this:

for key, value in self.exception_handlers.items():
    exception_handlers[key] = value

Everything works as expected and my error handler handles 500 errors the same as any other status code.

[zoola969]

Same for me. Looks strange because it prevents middle middlewares (a.k.a user middlewares) to work properly on server errors. CORS middleware for example doesn't send CORS headers

[ccrims0n]

This is definitely an issue.
What is the reason for having 2 different flows for exception handling?

[clickonben]

Apparently, this is by design. ServerErrorMiddleware (for 500 errors) and ExceptionMiddleware (for everything else) are separate so that the middleware stack can ben in this order

ServerErrorMiddleware
CustomMiddleware1
CustomMiddleware2
ExceptionMiddleware

I.E. Server errors get handled before any custom middleware runs but other HTTP exceptions get handled after any custom middleware runs, which makes sense.

https://www.starlette.io/middleware/#using-middleware

[clickonben]

The way I understand it is as follows.

Say this is your middleware stack which runs top to bottom.

ServerErrorMiddleware
CORSMiddleware
ExceptionMiddleware

The top and bottom are fixed and CORSMiddleware is optional. If the server throws a 500 error it basically means that an unhandled exception in the code has occurred, no other HTTP status means this. 500 errors are unhandled internal exceptions and other status codes (like 404 or 403) are intentionally rasied HTTP errors. In the case of a 500 error, you don't want other middleware to run first because the request/thread is in an exceptional state, you are effectively in the except: block of a try: except:. so this is handled in ServerErrorMiddleware which runs first.

All other status codes are not exceptions in the traditional sense, they are a product of the way that Starlette uses exceptions to return HTTP errors, the request/thread is not in an exceptional state, the user has made an exceptional request. This is a common way to handle HTTP errors in this kind of framework. In this case, you DO want the other middleware to run first, including the ServerErrorMiddleware.

If we take the above middleware stack as an example, say a client makes a request that is a CORS violation, if the server throws a 500 error we likely don't want the CORSMiddleware to run and return a 403 error, we want to return a 500 error. However, if the request would otherwise return a 405 (or anything other than a 500 error) we DO want the CORSMiddleware to run and return a 403 error and we don't want ExceptionMiddleware to run.

[zoola969]

Actually, CORSMiddleware should not decide which status code to return; its sole purpose is to include, or not include, additional headers. Therefore, it should work in all situations, including those with handled and unhandled exceptions. If the server returns a 500 status without the appropriate CORS headers, the browser and JavaScript code won't even be able to detect the status code or body. It’s often useful to include an error ID in the response body or headers.

In my opinion, having an option to catch all exceptions to modify the body of a 500 response, add additional headers, or perform any necessary action, is essential for any web framework.

[ccrims0n]

I agree with @zoola969
Isn't it a middleware anti-pattern to catch the exception and return the response except for the ExceptionMiddleware.

[clickonben]

Guys, you might want to continue this discussion here:

#2723

I am not a Starlette dev, I'm just someone who couldn't get his exception handler to work and the Starl;ette devs don't seem to bother reading these discussion pages.

[Kludex]

I'm reading it.