Error handler call order is not deterministic

[avilaton]

Code

Here is sample code where the issue is seen

class GenericError(Exception):
    pass


class SomeSpecificError(GenericError):
    pass


ns = Namespace("some_route")

@ns.errorhandler(SomeSpecificError)
def handle_specific_error(error):
    return {"message": "something specific happened"}, 401

@ns.errorhandler(GenericError)
def handle_generic_error(error):
    return {"message": "generic error"}, 401

Repro Steps (if applicable)

1. raise SomeSpecificError from any route handler
2. run this test repeatedly
3. Broken!

Expected Behavior

the order in which the error handlers are registered should be taken into account when those are looked up for a given exception

Actual Behavior

Handlers are called in non deterministic order

Environment

• Python version 3.5
• Flask version 1.0.3
• Flask-RESTX version 0.14

[jhampson-dbre]

What's the use case for having 2 error handler functions with the same name (i.e. handle_specific_error)? Or is that a typo?

[avilaton]

What's the use case for having 2 error handler functions with the same name (i.e. handle_specific_error)? Or is that a typo?

It was a typo, I just corrected it.

[jhampson-dbre]

In that case, I don't see why raising SomeSpecificError would ever trigger GenericError, regardless of the order they were registered in. Can you post a code snippet for how you are raisingSomeSpecificError inside a route?

[avilaton]

Registering an error handler for an exception will also catch child exceptions, see

flask-restx/flask_restx/api.py

Line 674 in 66d884f

if isinstance(e, typecheck):

Where the exception being raised is validated with isinstance . I'll update the snippet of that isn't clear, thanks for your response.

[jhampson-dbre]

In your example, both classes only are subclass of Exception. However, I can see where there would be a problem if SomeSpecificError is a subclass GenericError:

class GenericError(Exception):
    pass


class SomeSpecificError(GenericError):
    pass


ns = Namespace("some_route")

@ns.errorhandler(SomeSpecificError)
def handle_specific_error(error):
    return {"message": "something specific happened"}, 401

@ns.errorhandler(GenericError)
def handle_generic_error(error):
    return {"message": "generic error"}, 401

Based on

flask-restx/flask_restx/api.py

Line 673 in 66d884f

for typecheck, handler in six.iteritems(self._own_and_child_error_handlers):

I see that given SpecificError, when typecheck is GenericError and handler is handle_generic_error(), then
if isinstance(SpecificError, GenericError) would evaluate to True and call handle_generic_error(e). If GenericError is encountered before SpecificError, then it would break from the loop with GenericError incorrectly being chosen to handle the error.

The proposed solution of checking the handlers in the order they were registered would work in scenarios where more specific handlers are registered before less specific handlers:

@ns.errorhandler(SomeSpecificError)
def handle_specific_error(error):
    return {"message": "something specific happened"}, 401

@ns.errorhandler(GenericError)
def handle_generic_error(error):
    return {"message": "generic error"}, 401

But if we reverse the order the handlers are registered in, the bug would still be encountered:

@ns.errorhandler(GenericError)
def handle_generic_error(error):
    return {"message": "generic error"}, 401

@ns.errorhandler(SomeSpecificError)
def handle_specific_error(error):
    return {"message": "something specific happened"}, 401

What do you think about adding "exact match" type check? Something like

        for typecheck, handler in six.iteritems(self._own_and_child_error_handlers):
            if type(e, typecheck):
                # Found exact match to handle error
                result = handler(e)
                default_data, code, headers = unpack(
                    result, HTTPStatus.INTERNAL_SERVER_ERROR
                )
                break
            elif isinstance(e, typecheck):
                # Found subclass to handle error,
                # keep looking for an exact match or a superseding subclass
                candidate_handler = handler

            # No exact match found. If subclass match was found, use it
            if candidate_handler:
                result = candidate_handler(e)
                default_data, code, headers = unpack(
                    result, HTTPStatus.INTERNAL_SERVER_ERROR
                )

The OrderedDict would still ensure consistent behavior when there is no exact match registered for the exception and multiple subclasses match. In this case, the last handler registered that is a match would be chosen to handle the error.

[avilaton]

I corrected the code sample in the original issue which probably caused the confussion, I'm very sorry. Yes, I meant to state that SomeSpecificError is a child exception of GenericError and the behavior you describe is exactly what happens in our case.

I only want to correct an error we did see that is clearly present, the fact that the order of error handlers is not predictable and we are seeing intermittent errors in our tests due to this. My proposed fix is minimal and is in any case, required to make the order predictable. I don't think I would want to alter the behavior much more.