Custom error handler not called by ServerErrorMiddleware in debug mode

[alex-oleshkevich]

ServerErrorMiddleware ignores handler argument when app is running in debug mode. Instead, it always calls the default one.

from starlette.applications import Starlette
from starlette.responses import Response

def my_handler(request, exc): 
    if request.app.debug:
        return Response('Fail in debug mode')
    return Response('Fail in production mode')

app = Starlette(
    debug=True,
    exception_handlers={Exception: my_handler}
)

This causes us to do workarounds like reimplementing ServerErrorMiddleware on our own just to use an alternate exception handler in debug mode. Though intuitively, you expect exception_handlers for that purpose.

Would it make sense to always call a configured handler when available, and ignore the default one?
A possible solution can look like this:

try:
    await self.app(scope, receive, _send)
except Exception as exc:
    request = Request(scope)
    if self.handler is not None:
        if is_async_callable(self.handler):
            response = await self.handler(request, exc)
        else:
            response = await run_in_threadpool(self.handler, request, exc)
    elif self.debug:
        # In debug mode, return traceback responses.
        response = self.debug_response(request, exc)
    else:
        # Use our default 500 error handler.
        response = self.error_response(request, exc)

Let me know if I can PR it.

[euri10]

Let me know if I can PR it.

yes, it would be very much appreciated ;)

[Kludex]

• This behavior is wanted, as explained on Always call custom error handler. #1803 (comment).

[Kludex]

It doesn't make sense to me for debug to lose priority. Like, usually you have your handlers, and to debug, you set the parameter.

[tomchristie]

It doesn't make sense to me for debug to lose priority. Like, usually you have your handlers, and to debug, you set the parameter.

Right, that was my rationale here too.
For comparison, we've got equivalent behaviour here to Django's 500 views and debug behaviour...

https://docs.djangoproject.com/en/4.0/ref/views/#the-500-server-error-view

[Kludex]

@alex-oleshkevich Are you strong on this, or can we close this issue and associated PR?

@tomchristie Would you mind inviting @alex-oleshkevich to the organization?

[alex-oleshkevich]

@Kludex I think you can close it. However, I still believe that this is a good add-on to the library. The current implementation steals too much control from the developer.

[euri10]

that would be nice to have yes

[Kludex]

It doesn't make sense to me for debug to lose priority. Like, usually you have your handlers, and to debug, you set the parameter.

Right, that was my rationale here too. For comparison, we've got equivalent behaviour here to Django's 500 views and debug behaviour...

docs.djangoproject.com/en/4.0/ref/views/#the-500-server-error-view

Flask has the same behavior as well: https://flask.palletsprojects.com/en/2.2.x/errorhandling/#unhandled-exceptions

Why would Starlette do something different than those two?

[euri10]

well 2 reasons, one is that Starlette debugger is very "minimal" compared to what Django and Flask ships by default.

Starception is definitely nicer in every possible way than this default, even superior imho to what Flask provides.

There would be no case if Starlette's debugger would be on par with it.

Starlette wants to keep the maintained codebase small, so "plugins" like this should be usable I think

[Kludex]

Starlette wants to keep the maintained codebase small, so "plugins" like this should be usable I think

Correct me if I'm wrong, but... It's usable, and @alex-oleshkevich is not blocked. The package can document for users to do something like:

if debug:
    middleware.append([DebugMiddleware])

Or... Change the DebugMiddleware API to something like: DebugMiddleware(debug=debug) - this doesn't look that good, but there are alternatives.

well 2 reasons, one is that Starlette debugger is very "minimal" compared to what Django and Flask ships by default.

If this gets in, then it will be even minimal, as I don't see why would someone use it anymore. I think the discussion should be different in this case: "is the debug flag useful?".

[euri10]

yes it's usable, but (don't quote me on this I may remember the stuff badly) it is not the outer layer of the middleware stack as the exception layer should be iirc, so exceptions raised after will be rendered using the starlette debugger template and not the starception one.

#1386 dismissed it, the current debugger in starlette serves no purpose to me in its current form.

[Kludex]

yes it's usable, but (don't quote me on this I may remember the stuff badly) it is not the outer layer of the middleware stack as the exception layer should be iirc, so exceptions raised after will be rendered using the starlette debugger template and not the starception one.

Can't the package recommend something like:

middleware = []
if os.getenv("DEBUG"):
    middleware.append([DebugMiddleware])

app = Starlette(middleware=middleware) # Do not use the `debug` flag!

Instead of:

middleware = [DebugMiddleware]

app = Starlette(middleware=middleware, debug=True) 

[alex-oleshkevich]

Can't the package recommend something like:

Sure, but this looks like a hack

IMO, the perfect solution is to re-use existing functionality of exception handlers or add a new constructor argument to Starlette to override exception handler in ServerErrorMiddleware. Currently, it looks for Exception or 500 error handler.

app = Starlette(middleware=middleware) # Do not use the debug flag!

This is bad advice as application may depend on that flag in different ways.
Also, passing the debug flag into multiple places is a cause of errors and confusion. It is already available via app.debug.