Sanic: Wrong status response when raise exception from catch_404 method

[rsc1975]

When I raise an exception from the catch method (favicon.ico request), the raised exception is not correctly processed, I get a 200 status code in the browser, maybe I'm doing something wrong, this is my first time with Sanic.

I've tested it in v0.6 and @master branch

My code:

from sanic import response
from sanic.exceptions import NotFound
from os.path import dirname, abspath, join
from sanic.app import Sanic

_CURDIR = dirname(abspath(__file__))

if __name__ == '__main__':

    app = Sanic(__name__) 
    app.static('', join(_CURDIR, 'static' ))

    @app.route('/')
    async def index(_):
        return await response.file_stream(join(_CURDIR, 'static', 'index.html'))

    @app.exception(NotFound)
    async def catch_404(request, exception):
        if '.' not in request.path:
            return await index(request) #Used for angular routes
        raise exception

    app.run('0.0.0.0', 5000, debug = True)

The server log trace:

[2017-12-01 16:26:30 +0100] [14367] [INFO] Goin' Fast @ http://0.0.0.0:5000
[2017-12-01 16:26:30 +0100] [14367] [INFO] Starting worker [14367]
[2017-12-01 16:26:34 +0100] - (sanic.access)[INFO][1:2]: GET http://127.0.0.1:5000/  200 -1
[2017-12-01 16:26:34 +0100] - (sanic.access)[INFO][1:2]: GET http://127.0.0.1:5000/favicon.ico  200 1712
[2017-12-01 16:26:39 +0100] [14367] [INFO] KeepAlive Timeout. Closing connection.

From browser I get a 200 status, in plain/text, with the response content:

Error while handling error: File not found
Stack: Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/sanic/static.py", line 76, in _handler
    stats = await stat(file_path)
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/coroutines.py", line 213, in coro
    res = yield from res
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/concurrent/futures/thread.py", line 56, in run
    result = self.fn(*self.args, **self.kwargs)
FileNotFoundError: [Errno 2] No such file or directory: '/Users/rob/git/GITHUB/zzzz-server/scripts/static/favicon.ico'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/sanic/app.py", line 565, in handle_request
    response = await response
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/coroutines.py", line 109, in __next__
    return self.gen.send(None)
  File "/Users/rob/git/GITHUB/zzzz-server/scripts/dummy.py", line 21, in catch_all
    raise exception
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/sanic/app.py", line 556, in handle_request
    response = await response
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/asyncio/coroutines.py", line 128, in throw
    return self.gen.throw(type, value, traceback)
  File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/site-packages/sanic/static.py", line 120, in _handler
    relative_url=file_uri)
sanic.exceptions.FileNotFound: File not found

I think that the expected behavior should be a 404 response, Am I wrong ?

[rsc1975]

I think I've found the problem, but I'm not sure what would be the best solution, I see 2 problems:

• The original exception (with the proper http status) is lost in the error management
• The resulting response shouldn't return a 200 status code (We are dealing with an exception)

In app.py, the code to manage exceptions in responses is this (line 557):

        except Exception as e:
            # -------------------------------------------- #
            # Response Generation Failed
            # -------------------------------------------- #
            try:
                response = self.error_handler.response(request, e)
                if isawaitable(response):
                    response = await response
            except Exception as e:
                if self.debug:
                    response = HTTPResponse("Error while handling error: {}\nStack: {}".format(e, format_exc()))
                else:
                    response = HTTPResponse("An error occurred while handling an error")

I would change it by something like this:

        except Exception as e:
            # -------------------------------------------- #
            # Response Generation Failed
            # -------------------------------------------- #
            try:
                response = self.error_handler.response(request, e)
                if isawaitable(response):
                    response = await response
            except Exception as ex:
                if ex == e: # <- If It's the original exception, IMHO we should return it
                    response = self.error_handler.default(request=request, exception=ex)
                elif self.debug:
                    response = HTTPResponse(
                        "Error while handling error: {}\nStack: {}".format(
                            ex, format_exc()), status=500)
                else:
                    response = HTTPResponse(
                        "An error occurred while handling an error", status=500)

If ex == e we are not dealing an error during the original exception management, we are dealing with the original exception.
Instead of status==500, maybe we can get it from the exception if it contains status_code, besides, if the ex object is a SanicException, maybe we should return it, instead of a generic "An error occurred while handling an error", I mean, if in the catch method the user raise a custom exception (different to the original one), that exception is lost and replaced by a generic response, I don't think this is a desired behavior for most users.

So, I'd change the line:

 if ex == e: 

By

 if isinstance(ex, SanicException): 

With this change we assume that the raised exception is a "controlled" case by the user or the original exception (i.e: missing routes, 404)