Unexpected behavior using a catch-all Blueprint exception handler

[simonwahlgren]

Describe the bug
Using a catch-all exception handler in a Blueprint might lead to unexpected behavior. For example:

from sanic import Sanic, Blueprint, response
from sanic.exceptions import NotFound

error_handlers = Blueprint(__name__)


@error_handlers.exception(NotFound)
def not_found(request, exception):
    return response.text("Not found", status=404)


@error_handlers.exception(Exception)
def unhandled_exceptions(request, exception):
    return response.text("Unhandled exception", status=500)


app = Sanic("My Hello, world app")
app.blueprint(error_handlers)


@app.route("/")
async def test(request):
    return json({"hello": "world"})


if __name__ == '__main__':
    app.run(debug=True)

One might think that the not_found would handle all 404's, but that's not always the case, sometimes the unhandled_exceptions handler is being used instead, restarting the application will give "random" results.

From what I can see the underlying problem is this line: https://github.com/sanic-org/sanic/blob/main/sanic/handlers.py#L67.

Since all exceptions derive from Exception they will return True here when compared to the unhandled_exceptions exception Exception. So it's basically the order of the self.handlers that will determine which error handler to be used (if there are multiple handlers registered for the same derived exception) since it returns early on the first match.

Also, the reason for "random" results between restarts seems to be that a set (undefined order) is used as the data structure for storing the registered exception handlers: https://github.com/sanic-org/sanic/blob/main/sanic/mixins/exceptions.py#L8 when using a Blueprint.

Previously in versions <21.x this used to be a list and the problem above could be "circumvented" by registering the catch-all exception handler last. This is also how the app.error_handler seems to be working and the workaround still works for normal application routes.

Expected behavior
The explicitly registered exception handler should primarily be used even thou a catch-all handler is registered, the order when the handler was registered shouldn't matter. I would also expect the same behavior for both Blueprint and normal application routes.

Environment

• Version: 21.3.2

[ahopkins]

Looks like the simplest solution would be to precache the handler:

self.cached_handlers[exception] = handler

At first glance, I am not entirely sure why this was not done.

---

Digging deeper, it looks to me like a better solution would be either, at run time loop thru mro to find the nearest ancestor (and cache that).

[ahopkins]

PR #2128 should be the solution here. But, I think it is only part of a greater solution needed.

1. You should not be able to register the same exception more than once, or at least not on the same App/Blueprint.
2. Handlers should only be fetched in relation to the BP or App context of the matched route. This effectively means that some exceptions (NotFound could only be registered app level).