Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.1k
Comments
Add WebSocketException and support for WS handlers#527
Add WebSocketException and support for WS handlers#527tiangolo wants to merge 3 commits intoKludex:masterfrom
Conversation
lovelydinosaur commentedMay 24, 2019
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 commentedMay 24, 2019
I just checked all the uses of Nevertheless, I guess it's possible that in some scenarios people get confused and end up using I could update the default |
chbndrhnns commentedNov 7, 2019
Is there anything that prevents this PR from being merged? |
lovelydinosaur left a comment
There was a problem hiding this 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
WebSocketExceptionhandling. 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.
| class WebSocketException(Exception): | ||
| def __init__(self, code: int = status.WS_1008_POLICY_VIOLATION) -> None: |
There was a problem hiding this comment.
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 havecode as a mandatory argument here, in line withHTTPException.
| # 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) |
lovelydinosaurNov 7, 2019 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
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 reallyhave to do this, because the webserver's behaviorought 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 exceptioncould have been raised after the websocket close.) I think that means we'd want awebsocket_complete = False alongside the existingresponse_started = False, and track its state in the_send() function.
There was a problem hiding this comment.
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 thisin 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 serverought to take care of anyways.
| await self.app(scope, receive, _send) | ||
| except Exception as exc: | ||
| if not response_started: | ||
| if not response_started and scope["type"] == "http": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Let's useif scope["type"] == "http" and not response_started
lovelydinosaur commentedNov 7, 2019
Related, this PR makes me notice that wecould choose to alter our error handling, so that wealways run the http error handler (even if a response has already been started), it's just that weonly return that response if one has not already been started. ifscope["type"]=="http":request=Request(scope)ifself.debug:# In debug mode, return traceback responses.response=self.debug_response(request,exc)elifself.handlerisNone:# Use our default 500 error handler.response=self.error_response(request,exc)else:# Use an installed 500 error handler.ifasyncio.iscoroutinefunction(self.handler):response=awaitself.handler(request,exc)else:response=awaitrun_in_threadpool(self.handler,request,exc)ifnotresponse_started:awaitresponse(scope,receive,send) |
luckydonald commentedDec 24, 2019
Hey guys. How's the status on this? |
Catastropha commentedJan 5, 2020
any good news? |
jkamdjou commentedJan 22, 2020
Would also love to see this merged. |
Catastropha commentedFeb 12, 2020
Please merge, makes the code more beautiful |
tiangolo commentedFeb 12, 2020
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. |
remi-debette commentedJan 11, 2021
Hi! Is there any news on this? |
lewoudar commentedJul 17, 2021
another ping to know it this pull request is still relevant |
zljubisic commentedDec 6, 2021
Is there any news? |
Kludex commentedDec 6, 2021
Kludex commentedFeb 1, 2022
I'm closing this in favor of#1263. |
urbanonymous commentedOct 3, 2022
Docs related tohttps://fastapi.tiangolo.com/advanced/websockets/ should be updated.
|
Add WebSocketException and support for WS handlers.
Currently, if an exception is raised inside of a WebSocket endpoint, it bubbles, until a
500HTTP 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
WebSocketExceptioncomparable toHTTPException.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.