fix: Fix bugs in the subscription idle timeout#665
fix: Fix bugs in the subscription idle timeout#665allenporter merged 1 commit intoPython-roborock:mainfrom
Conversation
We need to actually track which subscriptionsare active and don't add duplicate subscriptions in those cases. We keep a separate object to track the subscription state since it is different than the callback logic (e.g. subscribe callback is removed from the list when unsubscribe happens)
There was a problem hiding this comment.
Pull request overview
This PR fixes bugs in the MQTT subscription idle timeout mechanism by introducing proper tracking of active subscriptions to prevent duplicates and ensure correct unsubscription behavior.
Key Changes:
- Added
_client_subscribed_topicsset to track which topics are actively subscribed at the MQTT client level - Enhanced the idle timeout logic with double-check locking to prevent race conditions during subscription/unsubscription
- Added comprehensive test coverage for subscription reuse scenarios
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| roborock/mqtt/roborock_session.py | Implements subscription state tracking with _client_subscribed_topics set and updates subscribe/unsubscribe logic to prevent duplicate subscriptions |
| tests/mqtt/test_roborock_session.py | Adds test case verifying subscription reuse behavior and idle timeout handling with multiple callbacks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _LOGGER.debug("Skipping unsubscribe for %s, new listeners added", topic) | ||
| return | ||
|
|
||
| self._idle_timers.pop(topic, None) |
There was a problem hiding this comment.
The timer is being removed from _idle_timers after the double-check for listeners, but this creates a potential inconsistency. If this code path is reached, it means the timer already completed its sleep and was never cancelled by a new subscription. However, the timer should have already been removed from the dict when schedule_unsubscribe() was called (which adds it), or when it was cancelled. Consider removing this line since the timer should naturally be removed when the task completes or move it outside the lock to avoid holding the lock longer than necessary.
| self._idle_timers.pop(topic, None) |
2 participants
We need to actually track which subscriptions are active and don't add duplicate subscriptions in those cases. We keep a separate object to track the subscription state since it is different than the callback logic (e.g. subscribe callback is removed from the list when unsubscribe happens)