Trigger WSClient Disconnect When WS Connection Closed #480
Conversation
WalkthroughThe changes involve modifications to the Changes
Sequence Diagram(s)sequenceDiagram
participant Client as WebSocket Client
participant Server as WebSocket Server
participant Exit as Exit Handler
Client->>Server: Establish Connection
Client->>Client: Start Listening
alt Connection Closed
Client->>Exit: _signal_exit()
Exit->>Server: Close Connection
Exit->>Client: Cleanup Resources
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
deepgram/clients/common/v1/abstract_sync_websocket.py (1)
268-270: LGTM: Proper cleanup for specific close codesThe addition of
_signal_exit()for close codes 1000 (normal closure) and 1001 (going away) ensures proper cleanup in these specific scenarios. This is consistent with the ConnectionClosedOK handling and properly addresses the case where the connection is hanging on recv().Consider enhancing the code comments to document the meaning of these specific close codes:
if e.code in [1000, 1001]: + # 1000: Normal closure, meaning the purpose for which the connection was established has been fulfilled + # 1001: Going away, meaning the server is going down or the client is navigating away # signal exit and close self._signal_exit()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
deepgram/clients/common/v1/abstract_async_websocket.py(2 hunks)deepgram/clients/common/v1/abstract_sync_websocket.py(1 hunks)
🔇 Additional comments (4)
deepgram/clients/common/v1/abstract_sync_websocket.py (1)
259-261: LGTM: Proper cleanup on normal connection closure
The addition of _signal_exit() ensures proper cleanup of resources when the WebSocket connection is closed normally. This change aligns with the PR objective and follows consistent error handling patterns.
deepgram/clients/common/v1/abstract_async_websocket.py (3)
257-259: Properly signaling exit on ConnectionClosedOK exceptions
Adding await self._signal_exit() in the ConnectionClosedOK exception handler ensures that the WebSocket client gracefully handles normal closure events by signaling the exit and cleaning up resources appropriately.
266-268: Graceful shutdown on ConnectionClosed exceptions with codes 1000 and 1001
Including await self._signal_exit() when handling ConnectionClosed exceptions with codes 1000 and 1001 ensures that the client exits gracefully during expected disconnections, releasing resources as intended.
475-475: Consistent return value in finish method after handling CancelledError
Returning True after handling asyncio.CancelledError provides a consistent success indicator from the finish method, even when tasks are cancelled, aligning with the method's purpose.
Proposed changes
Addresses: #477
Should close websocket when server forces disconnect (when not using keepalive and not sending audio).
Types of changes
What types of changes does your code introduce to the community Python SDK?
Put an
xin the boxes that applyChecklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
NA
Summary by CodeRabbit