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 exception handler for WebSocketRequestValidationError (which also allows to override it) #6030

Merged
merged 10 commits into from Jun 11, 2023

Conversation

kristjanvalur
Copy link
Contributor

@kristjanvalur kristjanvalur commented Feb 22, 2023

Previously, when a WebSocket dependency failed, the connection was closed (via close()) and a WebSocketRequestValidationError was raised.

While the close() was typically handled, and would result in a server returning 403 to the originator (as per ASGI spec https://asgi.readthedocs.io/en/latest/specs/www.html?highlight=403#close-send-event), a subsequently raised exception was not handled and would typically result in an internal 500 error. This error could pollute logs and generally be a nuisance.

Starlette has, since version 0.21.0, supported error handlers for WebSocket endpoints. This PR installs an error handler for
WebSocketRequestValidationError which performs the correct handling of calling close().

@github-actions
Copy link
Contributor

📝 Docs preview for commit 0d755c2 at: https://63f616c20ad18a14fc03c9eb--fastapi.netlify.app

@lclbm
Copy link

lclbm commented Feb 26, 2023

btw, Is there a way to make the fastapi exception_handler support websocket connection?

It seems that when the websocket route is processing Depends, if an exception occurs in the Depends func, the exception will be handed over to the default ServerErrorMiddleware

app = FastAPI()
@app.exception_handler(Exception)
async def exception_handler(request: Request | WebSocket, exc: Exception | ExceptionBase):
    ...

venv/Lib/site-packages/starlette/middleware/errors.py:148

class ServerErrorMiddleware:
    def __init__(
        self,
        app: ASGIApp,
        handler: typing.Optional[typing.Callable] = None,
        debug: bool = False,
    ) -> None:
        ...

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

@kristjanvalur
Copy link
Contributor Author

btw, Is there a way to make the fastapi exception_handler support websocket connection?

I've been looking at that, over in the starlette repo. Basically, ServerErrorMiddleware does not handle errors happing in a websocket scope.

There may be a few reasons for this:

  1. Backwards compatibility. Existing registered 500 or Exception handlers would then possibly get called for a handler needs to be aware of the scope and behave differently.
  2. Less need for it than for http. This error handler really catches only if the whole endpoint fails. And a webserver typically catches this also and closes the connection
  3. the debug functionality might be marginally useful here.
  4. The ExceptionMiddleware is probably good enough. It already has a handler for WebSocketsException

That being said, I'm in the process of investigating how standard servers deal with an unhandled exception in the websocket ASGI scope. Maybe Starlette could do with a separate webockets handler inside ServerErrorMiddleware in this case.

@kristjanvalur
Copy link
Contributor Author

kristjanvalur commented Feb 27, 2023

Oh, and by the way, the @app.exception_handler works such that all classes except Exception actually do get passed to ExceptionMiddleware. Exception (and 500) get sent to the ServerErrorMiddleware which as explained above does not do anything with websockets connections.

@github-actions
Copy link
Contributor

📝 Docs preview for commit 72d8584 at: https://641c15416cec180f2f6f008d--fastapi.netlify.app

@github-actions
Copy link
Contributor

github-actions bot commented May 7, 2023

📝 Docs preview for commit 37e9c97 at: https://6457867e077e6b51a52b19cd--fastapi.netlify.app

@tiangolo tiangolo changed the title Handle WebSocketRequestValidationError ✨ Add exception handler for WebSocketRequestValidationError (which also allows to override it) Jun 11, 2023
@tiangolo
Copy link
Owner

Awesome, thanks @kristjanvalur! 🚀

This will be available in the next release, in the next hours (or tomorrow), 0.97.0 🎉

@tiangolo tiangolo merged commit ab03f22 into tiangolo:master Jun 11, 2023
10 checks passed
@kristjanvalur kristjanvalur deleted the kristjan/ws-err branch June 11, 2023 20:40
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

3 participants