Skip to content

Replace RuntimeError with WebSocketDisconnected #2767

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

ruitcatarino
Copy link

@ruitcatarino ruitcatarino commented Nov 25, 2024

Summary

Change the exception raised if we attempt to use a disconnected WebSocket from RuntimeError to WebSocketDisconnected.
Discussed in #2762
Issue #2766

Checklist

  • I understand that this PR may be closed in case there was no previous discussion. (This doesn't apply to typos!)
  • I've added a test for each change that was introduced, and I tried as much as possible to make a single atomic change.
  • I've updated the documentation accordingly.

I haven't changed the documentation, if there is a need for it I will gladly change it.

Comment on lines +25 to +30
class WebSocketDisconnected(RuntimeError):
"""
Raised when attempting to use a disconnected WebSocket.
"""

pass
Copy link
Member

Choose a reason for hiding this comment

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

That's not really what I meant... The exception already exists.

Copy link
Author

Choose a reason for hiding this comment

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

Well, reading the code, WebSocketDisconnect seems to be called when you want to disconnect a socket, in these cases the socket is already disconnected. In my opinion, it would make more sense to create a new exception (WebSocketDisconnected). Tell me if you still want to use WebSocketDisconnect and if so I will make such changes.

@dym-ok
Copy link

dym-ok commented Mar 19, 2025

The change suggested by @ruitcatarino looks meaningful to me.
It would allow precise handling of situations like the one mentioned in #2543.

What stands in the way of accepting this change, can we somehow help?

@ruitcatarino
Copy link
Author

After the revival of this MR, which, to be honest, I have revisited sporadically in the hopes of getting an update, since my team and I at work are still affected by the original issue that led me to create this pull request, I realized that this newly created exception could also be used in the following cases: raise RuntimeError('WebSocket is not connected. Need to call "accept" first.')raise WebSocketDisconnected('WebSocket is not connected. Need to call "accept" first.') in the methods receive_bytes, receive_text and receive_json.

@ruitcatarino

This comment was marked as resolved.

@ruitcatarino ruitcatarino requested a review from Kludex March 20, 2025 19:08
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.

Replace RuntimeError with WebSocketDisconnected once the socket has been disconnected
3 participants