fix(device)!: rollback and improve send/recv socket exception #304
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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 (
|
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #304 +/- ##
==========================================
+ Coverage 39.93% 40.00% +0.07%
==========================================
Files 84 84
Lines 7535 7524 -11
==========================================
+ Hits 3009 3010 +1
+ Misses 4526 4514 -12 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Outside diff range and nitpick comments (1)
midealocal/device.py (1)
248-254: Redundant logging before raisingSocketExceptionIn the
authenticatemethod, whenself._socketisNone, you log a debug message and then raise aSocketException. Since the calling method (connect) already logs the exception, this logging might be redundant and could be removed to reduce clutter.Consider removing the debug log before raising the exception:
if not self._socket: - _LOGGER.debug( - "[%s] authenticate failure, device socket is none", - self._device_id, - ) # Raise exception to connect loop raise SocketException
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- midealocal/device.py (8 hunks)
Additional comments not posted (2)
midealocal/device.py (2)
645-654: Review exception handling forNoSupportedProtocolIn the
runmethod, when aNoSupportedProtocolexception is caught, the loop continues without settingreconnect = True, unlike other exceptions. If this exception indicates a critical issue affecting connectivity, you may need to handle it similarly to other exceptions.Please verify whether
NoSupportedProtocolshould trigger a reconnection attempt or if additional handling is required.
572-572: Ensure socket is closed when IP address changesIn the
set_ip_addressmethod, after updating the IP address, you callself.close_socket(init=True). Ensure that the socket is properly closed before attempting to reconnect to the new IP address to prevent any lingering connections.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
midealocal/device.py (4)
Line range hint
200-233: Improved connection handling and error managementThe changes to the
connectmethod significantly enhance error handling and provide more detailed logging for various exception types. The addition of theinitparameter allows for a more comprehensive initial connection setup, including protocol checking and capabilities retrieval.Consider adding a brief comment explaining the purpose of the
initparameter for better code readability:- def connect(self, init: bool = False) -> bool: + def connect(self, init: bool = False) -> bool: + """ + Connect to the device. + + :param init: If True, perform initial setup including protocol check and capabilities retrieval. + :return: True if connection is successful, False otherwise. + """
284-327: Enhanced error handling and timeout management in send_message_v2The improvements to
send_message_v2method are commendable. The addition of a socket check, more specific exception handling, and query-specific timeout settings enhance the robustness of the method.Consider extracting the common logging pattern into a separate method to reduce code duplication:
def _log_send_error(self, error_type: str, error: Exception): _LOGGER.debug( "[%s] send_message_v2 %s: %s", self._device_id, error_type, error, )Then, you can use it in the exception handlers:
except TimeoutError as e: self._log_send_error("timed out", e) raise
Line range hint
360-419: Improved message handling and error management in refresh_statusThe changes to the
refresh_statusmethod significantly enhance its robustness. The addition of a loop for message reception and parsing ensures that all messages are properly handled. The improved exception handling provides better control over different error scenarios.Consider adding a maximum retry count to prevent potential infinite loops in case of persistent errors:
MAX_RETRIES = 5 retry_count = 0 while True: if retry_count >= MAX_RETRIES: _LOGGER.warning("[%s] Max retries reached in refresh_status", self._device_id) break # ... existing code ... retry_count += 1
636-699: Greatly improved run loop with robust error handling and connection managementThe changes to the
runmethod significantly enhance its robustness and efficiency. The addition of a connection retry loop with exponential backoff is an excellent practice for handling network issues. The more specific exception handling provides better control over different error scenarios, and the added sleep prevents high CPU usage.Consider extracting the exponential backoff logic into a separate method for better readability and potential reuse:
def _get_backoff_time(self, retries: int) -> int: return min(5 * (2 ** (retries - 1)), 600) # In the run method: sleep_time = self._get_backoff_time(connection_retries)This would make the code more maintainable and easier to adjust if needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- midealocal/device.py (8 hunks)
🔇 Additional comments not posted (3)
midealocal/device.py (3)
248-253: Improved socket validation in authenticationThe addition of a check for
self._socketbeing None before attempting authentication is a good defensive programming practice. Raising aSocketExceptionin this case ensures that the error is properly propagated and can be handled in the calling code.
579-583: Improved socket closure with initialization optionThe addition of the
initparameter toclose_socketmethod allows for resetting the unsupported protocol list during initialization. This is useful for re-establishing device capabilities after reconnection.Regarding the previous thread safety concern, it's important to ensure that access to
self._unsupported_protocolis thread-safe if multiple threads can interact with the device. Consider using a threading lock when modifying this shared resource:#!/bin/bash # Check for potential thread safety issues rg -n "self._unsupported_protocol" midealocal/device.py rg -n "threading.Lock()" midealocal/device.pyIf no lock is found, consider adding one to ensure thread safety.
Line range hint
1-724: Overall significant improvements in error handling and connection managementThe changes made to this file have greatly enhanced its robustness and reliability. The improvements in error handling, connection management, and the addition of exponential backoff for retries are particularly noteworthy. These changes should make the device communication more resilient to network issues and unexpected errors.
To further improve the code:
- Consider creating a custom logger for this class to reduce the repetition of
[%s]andself._device_idin log messages.- Look for opportunities to extract common error handling patterns into separate methods to reduce code duplication.
- Consider implementing a state machine pattern to manage the different states of the device (connecting, connected, disconnected, etc.) which could make the code more maintainable and easier to reason about.
To ensure that all error cases are properly handled, run the following script to check for any
except:clauses without specific exception types:If any are found, consider replacing them with more specific exception handling.
wuwentao
force-pushed
the
rollback_socket
branch
from
5c23ea3 to
f0b25dc
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
midealocal/device.py (5)
Line range hint
200-243: Improved connection handling and error managementThe changes to the
connectmethod significantly enhance the connection process by introducing different behaviors for initial connections and reconnections. The added error handling for various exceptions improves the robustness of the connection process.Consider adding more detailed logging for the different connection scenarios (init, reconnect) to aid in debugging. For example:
if reconnect or init: + _LOGGER.debug("[%s] Refreshing status (init: %s, reconnect: %s)", self._device_id, init, reconnect) self.refresh_status(check_protocol=init) if init: + _LOGGER.debug("[%s] Getting capabilities during init", self._device_id) self.get_capabilities()
287-339: Enhanced error handling and logging in send_message_v2The improvements to the
send_message_v2method, including the check forself._socketbeing None and the more detailed exception handling, significantly enhance the robustness of the message sending process. The additional logging will aid in debugging.Consider grouping the exception handling blocks together to improve code readability. For example:
try: # ... existing code ... -except TimeoutError: - # ... existing code ... -except ConnectionResetError as e: - # ... existing code ... -except OSError as e: - # ... existing code ... -except Exception as e: - # ... existing code ... +except (TimeoutError, ConnectionResetError, OSError) as e: + _LOGGER.debug( + "[%s] send_message_v2 error: %s", + self._device_id, + str(e), + ) + raise +except Exception as e: + _LOGGER.exception( + "[%s] send_message_v2 Unexpected socket error", + self._device_id, + exc_info=e, + ) + raiseThis change would reduce code duplication while maintaining the specific logging for each exception type.
Line range hint
376-446: Significantly improved refresh_status methodThe
refresh_statusmethod has been greatly enhanced with more robust error handling, a loop for receiving and parsing messages, and improved handling for protocol checking and unsupported protocols. These changes should make the status refresh process more reliable and resilient to network issues.Consider extracting the message receiving and parsing loop into a separate method to improve code readability. For example:
def _receive_and_parse_messages(self): while True: if not self._socket: raise SocketException("Device socket is None") msg = self._socket.recv(512) if len(msg) == 0: raise OSError("Empty message received.") result = self.parse_message(msg) if result == MessageResult.SUCCESS: break elif result == MessageResult.PADDING: continue else: raise ResponseExceptionThis would make the
refresh_statusmethod cleaner and easier to understand.
Line range hint
663-730: Greatly improved run method with robust error handlingThe changes to the
runmethod significantly enhance its robustness and efficiency. The addition of a connection retry loop with exponential backoff is an excellent practice for handling network issues. The improved exception handling and the small sleep time to prevent high CPU usage are also valuable improvements.Consider extracting the connection retry logic into a separate method to improve code readability. For example:
def _retry_connection(self): connection_retries = 0 while self._socket is None: _LOGGER.debug("[%s] Socket is None, try to connect", self._device_id) if self.connect(init=True) is False: self.close_socket(init=True) connection_retries += 1 sleep_time = min(5 * (2 ** (connection_retries - 1)), 600) _LOGGER.warning( "[%s] Unable to connect, sleep %s seconds and retry", self._device_id, sleep_time, ) time.sleep(sleep_time) return connection_retriesThis would make the
runmethod cleaner and easier to understand.
Line range hint
1-730: Overall significant improvements to device communicationThe changes made to
midealocal/device.pyrepresent a substantial improvement in the reliability, robustness, and maintainability of the device communication code. Key improvements include:
- Enhanced error handling across multiple methods.
- Improved connection management with retry mechanisms.
- More detailed logging for better debugging.
- Refined status refresh and message parsing processes.
These changes should result in a more stable and resilient device communication system.
Consider the following architectural improvements for future iterations:
- Implement a state machine to manage device connection states more explicitly.
- Use asyncio for non-blocking I/O operations, which could improve overall performance.
- Implement a more comprehensive logging strategy, possibly using structured logging for easier analysis.
- Consider breaking down this large class into smaller, more focused classes to improve maintainability and testability.
These suggestions could further enhance the scalability and maintainability of the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- midealocal/device.py (8 hunks)
🔇 Additional comments (3)
midealocal/device.py (3)
246-257: Improved error handling in authenticate methodThe addition of a check for
self._socketbeing None before attempting to send data is a good defensive programming practice. This prevents potentialNoneTypeerrors and raises aSocketExceptionto be handled by the calling method.
352-365: Improved logging and refresh timing in build_sendThe addition of logging for force refreshing after setting status enhances the visibility of the device's behavior. Updating the
_previous_refreshtime ensures that the next status refresh will occur at the appropriate interval after sending a set command.
Line range hint
606-621: Improved close_socket method with initialization optionThe addition of the
initparameter to reset the_unsupported_protocollist during initialization is a good improvement. This allows for re-establishing device capabilities after a reconnection.Regarding the thread safety concern raised in a previous review, it's important to ensure that access to
self._unsupported_protocolis thread-safe if multiple threads can interact with the device. Consider using a threading lock when modifying this shared resource. Here's a script to check for potential thread safety issues:If the results show that
_unsupported_protocolis accessed from multiple methods and threading is used, consider implementing a lock mechanism to ensure thread safety.
rokam
changed the title
github-actions
bot
added
the
breaking change
changes detail:
Summary by CodeRabbit
New Features
Bug Fixes
Refactor