feat: Add pinging to local client #627
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds a keep-alive mechanism to the local client to prevent disconnections. Testing showed a significant improvement: 9 disconnects in 5 minutes without pinging versus 0 disconnects with pinging enabled.
Key Changes
- Added periodic ping functionality to maintain connection stability
- Implemented keep-alive task lifecycle management (creation on connect, cancellation on disconnect)
- Added exception handling for ping failures to ensure loop continues
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async def _keep_alive_loop(self) -> None: | ||
| while self._is_connected: | ||
| try: | ||
| await asyncio.sleep(_PING_INTERVAL) | ||
| if self._is_connected: | ||
| await self._ping() | ||
| except asyncio.CancelledError: | ||
| break | ||
| except Exception: | ||
| _LOGGER.debug("Keep-alive ping failed", exc_info=True) | ||
| # Retry next interval |
There was a problem hiding this comment.
The new keep-alive functionality lacks test coverage. Consider adding tests to verify:
- That
_keep_alive_taskis created whenconnect()is called - That the task is properly canceled when
close()or_connection_lost()is called - That the ping loop continues to execute periodically while connected
- That exceptions in the ping loop are handled gracefully without stopping the loop
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
* Initial plan * feat: Add comprehensive test coverage for keep-alive functionality Co-authored-by: Lash-L <20257911+Lash-L@users.noreply.github.com> * refactor: Address code review feedback on keep-alive tests Co-authored-by: Lash-L <20257911+Lash-L@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: Lash-L <20257911+Lash-L@users.noreply.github.com>
allenporter
left a comment
allenporter
left a comment
There was a problem hiding this comment.
I have requested changes for a follow up PR -- approving now so we can merge.
| # Perform protocol negotiation | ||
| try: | ||
| await self._hello() | ||
| self._keep_alive_task = asyncio.create_task(self._keep_alive_loop()) |
There was a problem hiding this comment.
Move this outside the scope of the try/catch. It won't throw RoborockException
| await self._ping() | ||
| except asyncio.CancelledError: | ||
| break | ||
| except Exception: |
There was a problem hiding this comment.
Let's catch RoborockException (expected) separate from uncaught exceptions so we can log them separately e.g. "ping failed" vs "Uncaught exception" implying a bug/shouldn't happen case we need to fix. Similar to _background_reconnect in v1 channel?
| _LOGGER = logging.getLogger(__name__) | ||
| _PORT = 58867 | ||
| _TIMEOUT = 5.0 | ||
| _PING_INTERVAL = 10 |
There was a problem hiding this comment.
Can you use a timedelta here to be more explicit on the unit?
| _PING_INTERVAL = 10 | |
| _PING_INTERVAL = datetime.timedelta(seconds=10) |
| async def _keep_alive_loop(self) -> None: | ||
| while self._is_connected: | ||
| try: | ||
| await asyncio.sleep(_PING_INTERVAL) |
There was a problem hiding this comment.
Assuming you accept the above suggestion
| await asyncio.sleep(_PING_INTERVAL) | |
| await asyncio.sleep(_PING_INTERVAL.total_seconds()) |
| assert channel._is_connected is True | ||
|
|
||
|
|
||
| async def test_keep_alive_task_created_on_connect(local_channel: LocalChannel, mock_loop: Mock) -> None: |
There was a problem hiding this comment.
We should avoid these tests of internal state like this generally, i'm not sure its worhtwhile to keep?
Tested twice:
Without pinging:
9 disconnects in 5 minutes
With pinging:
0 disconnects in 5 minutes