Gracefully handle websocket errors on python side#2157
Merged
Conversation
timkpaine
force-pushed
the
tkp/fastapidisconnect
branch
from
5cfe709 to
292a965
Compare
texodus
approved these changes
Apr 4, 2023
Member
texodus
left a comment
texodus
left a comment
There was a problem hiding this comment.
Thanks for the PR! Looks good!
| await self._ws.send_bytes(message) | ||
| else: | ||
| await self._ws.send_str(message) | ||
| except WebSocketError: |
Member
There was a problem hiding this comment.
I don't know enough about the ecosystem of possible exceptions here, but I have spent countless hours of my life chasing bugs that would've been immediately obvious had they not been swallowed by a blanket catch block. At minimum, some comments about what to expect in these cases and why would be super helpful - but I propose we generally try to handle specific exceptions instead, which would also communicate intent here.
Member
There was a problem hiding this comment.
Maybe also in this case merits a debug log, now that I think about it.
texodus
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Disconnect-related errors were handled for
aiohttpandstarlettewhen waiting for the client to send a message, this PR adds that same support to tornado but also handles disconnect when trying to send outbound messages.Note While testing, I noticed the route to the themes is incorrect, this is particularly noticeable after the recent theme changes