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

Add WebSocketException and support for WS handlers #527

Closed
wants to merge 3 commits into from

Conversation

tiangolo
Copy link
Sponsor Member

Add WebSocketException and support for WS handlers.

Currently, if an exception is raised inside of a WebSocket endpoint, it bubbles, until a 500 HTTP error is thrown.

This PR implements support for raising and handling exceptions inside of WebSocket endpoints.

It has default handlers using WebSocket codes from the specification.

It includes a generic WebSocketException comparable to HTTPException.

This would allow raising inside WebSocket endpoints, sub-functions, etc. And handling those errors in a custom manner. It might be used to, e.g. log WebSocket errors, customize a WebSocket closing code, etc.

Related issues: #483, #494


In FastAPI, this would allow using dependencies that can raise early in WebSocket routes (e.g. if the correct headers are not present) and other scenarios.

@tomchristie
Copy link
Member

Ah cool - interesting yup!

One thing for us to think about - are there currently any codepaths we're not thinking of where we're raising HTTPExceptions in a websocket context? Eg. Routing? Middleware?

@tiangolo
Copy link
Sponsor Member Author

I just checked all the uses of HTTPException and nope, at every point that it is raised it is exclusively for HTTP (e.g. inside of Route) or the scope is checked and the WebSocket is closed manually before (instead of) raising the HTTPException.


Nevertheless, I guess it's possible that in some scenarios people get confused and end up using HTTPExceptions in WebSocket routes.

I could update the default HTTPException handler to check if it's receiving a Request or a WebSocket and close the WebSocket in that case. What do you think?

@chbndrhnns
Copy link

Is there anything that prevents this PR from being merged?

Copy link
Member

@tomchristie tomchristie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great stuff here - thanks!

I think my take here would be:

  • Let's defo include the WebSocketException handling. That's an obviously great addition.
  • Let's defo ensure that uvicorn issues 1011 websocket closes if it gets an exception during the websocket task and the connection is open. It needs to function gracefully in that case whatever. (There's an equivelent plain text 500 page for the http exception case.)
  • There's more complicated things to consider wrt. the websocket error handling. Perhaps let's remove that from this PR for now, so that we can discuss it in isolation from the handled-exceptions, which is far clearer.

@@ -16,13 +18,31 @@ def __init__(self, status_code: int, detail: str = None) -> None:
self.detail = detail


class WebSocketException(Exception):
def __init__(self, code: int = status.WS_1008_POLICY_VIOLATION) -> None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reckon we should probably have code as a mandatory argument here, in line with HTTPException.

# 1011 indicates that a server is terminating the connection because
# it encountered an unexpected condition that prevented it from
# fulfilling the request.
await websocket.close(code=status.WS_1011_INTERNAL_ERROR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting...

We shouldn't really have to do this, because the webserver's behavior ought to be in line with this anyway.
Ie. A higher priority task for us should be to ensure that uvicorn has equivelent handling, and will close the websocker with a 1011 code if it gets and exception, and the socket is still open.

We'd also want to check in this case that the websocket is in an open state. (The exception could have been raised after the websocket close.) I think that means we'd want a websocket_complete = False alongside the existing response_started = False, and track its state in the _send() function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also not really clear if we want/need this in any case - In the HTTP case, we can use the handler to issue an application-custom 500 page. In the websocket case there's really nothing available for us to do other than close the the connection, which the server ought to take care of anyways.

@@ -99,7 +101,7 @@ def __init__(
try:
await self.app(scope, receive, _send)
except Exception as exc:
if not response_started:
if not response_started and scope["type"] == "http":
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use if scope["type"] == "http" and not response_started

@tomchristie
Copy link
Member

Related, this PR makes me notice that we could choose to alter our error handling, so that we always run the http error handler (even if a response has already been started), it's just that we only return that response if one has not already been started.

if scope["type"] == "http":
    request = Request(scope)
    if self.debug:
        # In debug mode, return traceback responses.
        response = self.debug_response(request, exc)
    elif self.handler is None:
        # Use our default 500 error handler.
        response = self.error_response(request, exc)
    else:
        # Use an installed 500 error handler.
        if asyncio.iscoroutinefunction(self.handler):
            response = await self.handler(request, exc)
        else:
            response = await run_in_threadpool(self.handler, request, exc)

    if not response_started:
        await response(scope, receive, send)

@luckydonald
Copy link

Hey guys. How's the status on this?

@Catastropha
Copy link

any good news?

@jkamdjou
Copy link

Would also love to see this merged.

@Catastropha
Copy link

Please merge, makes the code more beautiful

@tiangolo
Copy link
Sponsor Member Author

I have to implement the code review, and that probably includes a PR to Uvicorn that I have to do carefully, I just haven't had time yet. But that's the reason. Hopefully I'll finish it soon.

@JayH5 JayH5 added the websocket WebSocket-related label Sep 11, 2020
@remi-debette
Copy link

Hi! Is there any news on this?

@lewoudar
Copy link
Contributor

another ping to know it this pull request is still relevant

@zljubisic
Copy link

Is there any news?

@Kludex
Copy link
Sponsor Member

Kludex commented Dec 6, 2021

Yeah, I talked to @tiangolo about taking the lead on this. I've rebased on #1263. Now I need to work on the uvicorn side.

There's no ETA for this. If someone wants this, implementing the uvicorn part is welcome (info about it on this PR).

@Kludex
Copy link
Sponsor Member

Kludex commented Feb 1, 2022

I'm closing this in favor of #1263.

@Kludex Kludex closed this Feb 1, 2022
@tiangolo tiangolo deleted the websocket-exceptions branch August 18, 2022 15:23
@urbanonymous
Copy link

Docs related to https://fastapi.tiangolo.com/advanced/websockets/ should be updated.

In the future, there will be a WebSocketException that you will be able to raise from anywhere, and add exception handlers for it. It depends on the [PR #527](https://github.com/encode/starlette/pull/527) in Starlette.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
websocket WebSocket-related
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet