Skip to content

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
Owner

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.

Copy link
Owner

Choose a reason for hiding this comment

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

Sorry, you are right.

Copy link
Owner

Choose a reason for hiding this comment

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

No, actually, I understand what I had in mind: my idea was to raise WebSocketDisconnect to avoid the user from seeing the exception. The purpose of the RuntimeError here is that it's an issue with the code.

Copy link
Author

Choose a reason for hiding this comment

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

I'm confused, raising WebSocketDisconnect instead of RuntimeError or WebSocketDisconnected when attempting to explicitly call close() on a closed socket would still raise the exception WebSocketDisconnect and it would not be handled automatically. The user would have to handle it.
My thought process is by explicitly calling close() on a closed socket it will eventually call send() that would currently raise a RuntimeError. With my changes it would raise a specific exception WebSocketDisconnected. If we changed it to WebSocketDisconnect that exception would be raised from send() and close() never handles it.

And even if it was handled automatically and avoided the user from seeing the exception, the idea that I got from our discussion was that making close() idempotent was not the approach to follow (#2762 (reply in thread)).

Maybe I'm not understanding your request.

@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.

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