Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Why define an exception handler needed async? #1804

Closed
matianjun1 opened this issue Mar 11, 2020 · 8 comments · Fixed by #1876
Closed

Why define an exception handler needed async? #1804

matianjun1 opened this issue Mar 11, 2020 · 8 comments · Fixed by #1876

Comments

@matianjun1
Copy link

matianjun1 commented Mar 11, 2020

with tests/test_exceptions_handler.py define an exception handler like this:

@exception_handler_app.exception(NotFound, ServerError)
def handler_exception(request, exception):
return text("OK")

but in docs https://sanic.readthedocs.io/en/latest/sanic/exceptions.html

@app.exception(NotFound)
async def ignore_404s(request, exception):
return text("Yep, I totally found the page: {}".format(request.url))

what is the difference? And is async def needed?

@matianjun1 matianjun1 changed the title Why defnine a exception handler needed async? Why defnine an exception handler needed async? Mar 11, 2020
@matianjun1 matianjun1 changed the title Why defnine an exception handler needed async? Why define an exception handler needed async? Mar 11, 2020
@Tronic
Copy link
Member

Tronic commented Mar 11, 2020

In general all handlers in Sanic can be async or normal functions. You are free to use either even though in documentation async def is generally preferred even if the function doesn't need to be async.

In the current version there is a limitation that async error handlers might not work in some contexts (protocol errors), so a normal function is actually a safer choice there. This is due to be fixed in a future version.

@matianjun1
Copy link
Author

thx~I do meet unexcepted exception with async def in exceptions handler. And it works good with def only.

@ashleysommer
Copy link
Member

ashleysommer commented Mar 16, 2020

@matianjun1
Yes, this issue has come up a few times. The documentation needs to be changed.

The confusion comes from the fact Sanic request handler does support async exception handlers, see here: https://github.com/huge-success/sanic/blob/60b4efad67ba3b22dfaf83820c772cd7f5ff137c/sanic/app.py#L1015-L1017

But the sanic asyncio server implementation does not support async exception handlers.
https://github.com/huge-success/sanic/blob/60b4efad67ba3b22dfaf83820c772cd7f5ff137c/sanic/server.py#L576-L578

So I think for now we should remove any mention of async exception handlers in the documentation, and look at how we can get async exception handlers working at the asyncio server level.

@Tronic
Copy link
Member

Tronic commented Mar 16, 2020

Agreed, it would be best not to mention async exception handlers in documentation for now. This is one of the problems fixed in #1791 but it will take a few more months until that can ship, and there is no quick and easy way to fix this otherwise.

@ashleysommer
Copy link
Member

Is is just the docs at https://sanic.readthedocs.io/en/latest/sanic/exceptions.html that show an async error handler? Are there any other mentions anywhere else that need changing?

@Tronic Tronic mentioned this issue Mar 18, 2020
@stale
Copy link

stale bot commented Jun 14, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

@stale stale bot added the stale label Jun 14, 2020
@ahopkins ahopkins removed the stale label Jun 17, 2020
@stale
Copy link

stale bot commented Sep 17, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

@stale stale bot added the stale label Sep 17, 2020
@Tronic Tronic removed the stale label Sep 17, 2020
@stale
Copy link

stale bot commented Dec 19, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

@stale stale bot added the stale label Dec 19, 2020
@ahopkins ahopkins added necessary and removed stale labels Dec 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants