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

Update exception handler signature to reflect HTTPConnection #1440

Closed
wants to merge 6 commits into from
Closed

Update exception handler signature to reflect HTTPConnection #1440

wants to merge 6 commits into from

Conversation

aminalaee
Copy link
Member

Right now the docs and examples show the following signature for handlers:

async def not_found(request: Request, exc):
    return Response(content=HTML_404_PAGE, status_code=exc.status_code)

Even though this should be possible too:

async def handler(conn: HTTPConnection, exc):
    return Response(content=HTML_404_PAGE, status_code=exc.status_code)

Still in progress to find all occurrences that should be update.

@aminalaee
Copy link
Member Author

I was expecting that the ExceptionMiddleware should reflect this too, but I think because of this line the WebSocket exceptions are not processed?

async def __call__(self, scope: Scope, receive: Receive, send: Send) -> None:
if scope["type"] != "http":
await self.app(scope, receive, send)
return

@adriangb
Copy link
Member

What is the point of accepting an HTTPConnection? Error handlers don't work with WebSockets (unless #527 or similar gets merged), so in practice it's always going to be a Request, which is an HTTPConnection with additional info, right?

@aminalaee
Copy link
Member Author

aminalaee commented Jan 27, 2022

Error handlers don't work with WebSockets

Well I know they don't work. That's exactly what my last comment is pointing to. I didn't see @Kludex had taken the lead on that in #1263

@aminalaee aminalaee closed this Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants