Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(device): prevent exception on network failure #291

Closed
wants to merge 7 commits into from
Closed

fix(device): prevent exception on network failure #291

Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5e78508
fix(device): prevent exception on network failure
rokam Sep 6, 2024
bd657c5
chore: replaced error by warning
rokam Sep 6, 2024
3e58de3
fix: remove connect loop
rokam Sep 6, 2024
6744d37
fix: max wait time 10 minutes
rokam Sep 6, 2024
f7f797f
chore: update connect check order in run job
wuwentao Sep 9, 2024
7fdf0db
chore: add try/except for all the socket send/recv
wuwentao Sep 12, 2024
9489cd2
chore: update timeout from 1 to 2
wuwentao Sep 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 62 additions & 51 deletions midealocal/device.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@

from typing_extensions import deprecated

from .exceptions import CannotConnect, SocketException
from .exceptions import SocketException
from .message import (
MessageApplianceResponse,
MessageQueryAppliance,
Expand Down Expand Up @@ -201,41 +201,35 @@ def _authenticate_refresh_capabilities(self) -> None:
def connect(self) -> bool:
"""Connect to device."""
connected = False
for _ in range(3):
rokam marked this conversation as resolved.
Show resolved Hide resolved
try:
self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
self._socket.settimeout(10)
_LOGGER.debug(
"[%s] Connecting to %s:%s",
self._device_id,
self._ip_address,
self._port,
)
self._socket.connect((self._ip_address, self._port))
_LOGGER.debug("[%s] Connected", self._device_id)
connected = True
break
except TimeoutError:
_LOGGER.debug("[%s] Connection timed out", self._device_id)
except OSError:
_LOGGER.debug("[%s] Connection error", self._device_id)
except AuthException:
_LOGGER.debug("[%s] Authentication failed", self._device_id)
except RefreshFailed:
_LOGGER.debug("[%s] Refresh status is timed out", self._device_id)
except Exception as e:
file = None
lineno = None
if e.__traceback__:
file = e.__traceback__.tb_frame.f_globals["__file__"] # pylint: disable=E1101
lineno = e.__traceback__.tb_lineno
_LOGGER.exception(
"[%s] Unknown error : %s, %s",
self._device_id,
file,
lineno,
)
self.enable_device(connected)
try:
self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
self._socket.settimeout(10)
_LOGGER.debug(
"[%s] Connecting to %s:%s",
self._device_id,
self._ip_address,
self._port,
)
self._socket.connect((self._ip_address, self._port))
_LOGGER.debug("[%s] Connected", self._device_id)
connected = True
except TimeoutError:
_LOGGER.debug("[%s] Connection timed out", self._device_id)
except OSError:
_LOGGER.debug("[%s] Connection error", self._device_id)
except Exception as e:
file = None
lineno = None
if e.__traceback__:
file = e.__traceback__.tb_frame.f_globals["__file__"] # pylint: disable=E1101
lineno = e.__traceback__.tb_lineno
_LOGGER.exception(
"[%s] Unknown error : %s, %s",
self._device_id,
file,
lineno,
)
self.set_available(connected)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the connect method with suggestions for error handling improvements.

The connect method has been updated to handle various exceptions more gracefully by logging them. However, the handling of generic exceptions could be improved by providing more specific error messages or handling specific cases differently.

Consider refining the exception handling to provide more actionable insights or recover from specific errors when possible. For example, network-related exceptions could trigger a re-attempt or a different recovery mechanism.

Here's a proposed change to enhance the exception handling:

- except Exception as e:
-     file = None
-     lineno = None
-     if e.__traceback__:
-         file = e.__traceback__.tb_frame.f_globals["__file__"]  # pylint: disable=E1101
-         lineno = e.__traceback__.tb_lineno
-     _LOGGER.exception(
-         "[%s] Unknown error : %s, %s",
-         self._device_id,
-         file,
-         lineno,
-     )
+ except Exception as e:
+     _LOGGER.exception("[%s] Unexpected error occurred: %s", self._device_id, str(e))

This change simplifies the logging and focuses on the exception message, which is typically sufficient for debugging unexpected errors.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
try:
self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
self._socket.settimeout(10)
_LOGGER.debug(
"[%s] Connecting to %s:%s",
self._device_id,
self._ip_address,
self._port,
)
self._socket.connect((self._ip_address, self._port))
_LOGGER.debug("[%s] Connected", self._device_id)
connected = True
except TimeoutError:
_LOGGER.debug("[%s] Connection timed out", self._device_id)
except OSError:
_LOGGER.debug("[%s] Connection error", self._device_id)
except Exception as e:
file = None
lineno = None
if e.__traceback__:
file = e.__traceback__.tb_frame.f_globals["__file__"] # pylint: disable=E1101
lineno = e.__traceback__.tb_lineno
_LOGGER.exception(
"[%s] Unknown error : %s, %s",
self._device_id,
file,
lineno,
)
self.set_available(connected)
try:
self._socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
self._socket.settimeout(10)
_LOGGER.debug(
"[%s] Connecting to %s:%s",
self._device_id,
self._ip_address,
self._port,
)
self._socket.connect((self._ip_address, self._port))
_LOGGER.debug("[%s] Connected", self._device_id)
connected = True
except TimeoutError:
_LOGGER.debug("[%s] Connection timed out", self._device_id)
except OSError:
_LOGGER.debug("[%s] Connection error", self._device_id)
except Exception as e:
_LOGGER.exception("[%s] Unexpected error occurred: %s", self._device_id, str(e))
self.set_available(connected)

return connected

def authenticate(self) -> None:
Expand Down Expand Up @@ -525,33 +519,50 @@ def _check_heartbeat(self, now: float) -> None:
self.send_heartbeat()
self._previous_heartbeat = now

def _check_state(self) -> ParseMessageResult:
if not self._socket:
_LOGGER.warning("[%s] Socket closed", self._device_id)
return ParseMessageResult.ERROR
now = time.time()
self._check_refresh(now)
self._check_heartbeat(now)
msg = self._socket.recv(512)
if len(msg) == 0:
if self._is_run:
_LOGGER.warning(
"[%s] Socket error - Connection closed by peer",
self._device_id,
)
self.close_socket()
return ParseMessageResult.ERROR
return self.parse_message(msg)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review of the _check_state method with suggestions for enhanced error handling.

The _check_state method has been significantly modified to handle socket state checks and message parsing more robustly. The method now includes checks for socket closure and empty messages, which are correctly handled by logging warnings and closing the socket if necessary.

However, the method could benefit from additional error handling, particularly in managing exceptions that may occur during message reception or parsing.

Consider adding exception handling around the message reception and parsing to prevent the thread from exiting unexpectedly on exceptions:

- msg = self._socket.recv(512)
- if len(msg) == 0:
+ try:
+     msg = self._socket.recv(512)
+     if len(msg) == 0:
+         if self._is_run:
+             _LOGGER.warning(
+                 "[%s] Socket error - Connection closed by peer",
+                 self._device_id,
+             )
+             self.close_socket()
+         return ParseMessageResult.ERROR
+ except OSError as e:
+     _LOGGER.warning("[%s] Failed to receive message: %s", self._device_id, str(e))
+     self.close_socket()
+     return ParseMessageResult.ERROR
+ except Exception as e:
+     _LOGGER.exception("[%s] Unexpected error during message reception: %s", self._device_id, str(e))
+     self.close_socket()
+     return ParseMessageResult.ERROR

This change ensures that all potential exceptions during message reception are caught and handled appropriately, preventing the thread from terminating unexpectedly and allowing for more graceful error recovery.

Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
def _check_state(self) -> ParseMessageResult:
if not self._socket:
_LOGGER.warning("[%s] Socket closed", self._device_id)
return ParseMessageResult.ERROR
now = time.time()
self._check_refresh(now)
self._check_heartbeat(now)
msg = self._socket.recv(512)
if len(msg) == 0:
if self._is_run:
_LOGGER.warning(
"[%s] Socket error - Connection closed by peer",
self._device_id,
)
self.close_socket()
return ParseMessageResult.ERROR
return self.parse_message(msg)
def _check_state(self) -> ParseMessageResult:
if not self._socket:
_LOGGER.warning("[%s] Socket closed", self._device_id)
return ParseMessageResult.ERROR
now = time.time()
self._check_refresh(now)
self._check_heartbeat(now)
try:
msg = self._socket.recv(512)
if len(msg) == 0:
if self._is_run:
_LOGGER.warning(
"[%s] Socket error - Connection closed by peer",
self._device_id,
)
self.close_socket()
return ParseMessageResult.ERROR
except OSError as e:
_LOGGER.warning("[%s] Failed to receive message: %s", self._device_id, str(e))
self.close_socket()
return ParseMessageResult.ERROR
except Exception as e:
_LOGGER.exception("[%s] Unexpected error during message reception: %s", self._device_id, str(e))
self.close_socket()
return ParseMessageResult.ERROR
return self.parse_message(msg)

def run(self) -> None:
"""Run loop."""
connection_retries = 0
while self._is_run:
if not self.connect():
raise CannotConnect
_LOGGER.warning(
"[%s] Unable to connect with the device",
self._device_id,
)
connection_retries += 1
time.sleep(min(60 * connection_retries, 600))
continue
if not self._socket:
raise SocketException
_LOGGER.warning("[%s] No open socket available", self._device_id)
connection_retries += 1
time.sleep(min(60 * connection_retries, 600))
continue
connection_retries = 0
self._authenticate_refresh_capabilities()
timeout_counter = 0
start = time.time()
self._previous_refresh = self._previous_heartbeat = start
self._socket.settimeout(1)
while True:
try:
now = time.time()
self._check_refresh(now)
self._check_heartbeat(now)
msg = self._socket.recv(512)
if len(msg) == 0:
if self._is_run:
_LOGGER.error(
"[%s] Socket error - Connection closed by peer",
self._device_id,
)
self.close_socket()
break
result = self.parse_message(msg)
result = self._check_state()
if result == ParseMessageResult.ERROR:
_LOGGER.debug("[%s] Message 'ERROR' received", self._device_id)
self.close_socket()
Expand Down