fix: reconnect when send message fails #295
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR addresses a reconnection issue when sending messages fails by updating the connection attribute usage and error handling. Key changes include:
- Switching from the public ws_connection attribute to the internal _ws_connection in both connection logic and tests.
- Introducing a new test case to validate reconnection behavior after a simulated connection failure.
- Updating the pyproject.toml dev dependency configuration.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/test_connection.py | Updated tests to reference _ws_connection and added reconnection test case |
| realtime/_async/client.py | Replaced ws_connection with _ws_connection and adjusted error handling |
| pyproject.toml | Reorganized dev dependency configuration under a new dependency group |
Comments suppressed due to low confidence (1)
tests/test_connection.py:262
- [nitpick] The test is directly accessing the protected attribute '_ws_connection'. Consider using a public interface (e.g., is_connected) or a dedicated getter to reduce coupling to internal implementation details.
initial_ws = socket._ws_connection
| ) | ||
|
|
||
| try: | ||
| await self._ws_connection.send(message) |
There was a problem hiding this comment.
In the send_message function, errors during message sending are caught and handled with _on_connect_error, but the caller is not informed of the failure. Consider propagating the exception after handling so that the failure is not silently suppressed.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| @@ -17,18 +17,15 @@ python-dateutil = "^2.8.1" | |||
| typing-extensions = "^4.12.2" | |||
| aiohttp = "^3.11.14" | |||
|
|
|||
| [tool.poetry.dev-dependencies] | |||
| [tool.poetry.group.dev.dependencies] | |||
There was a problem hiding this comment.
The new dev dependency group only includes a subset of the dependencies previously defined. Please verify if the removal of dependencies like pytest, python-dotenv, pytest-asyncio, and coveralls is intentional.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
Pull Request Test Coverage Report for Build 14177115261Details
💛 - Coveralls |
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
When an error is raised when sending a message through the socket it just gets logged
What is the new behavior?
Handle the raised error when sending a message and initiate the reconnect flow.
Additional context
Add any other context or screenshots.