-
Notifications
You must be signed in to change notification settings - Fork 29
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.
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
catch camera url update timeout #506
catch camera url update timeout #506
Conversation
Reviewer's Guide by SourceryThis pull request addresses a timeout issue in the NOC module by extending the exception handling in the File-Level Changes
Tips
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @adriencog - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
@@ -502,7 +502,7 @@ async def async_update_camera_urls(self) -> None: | |||
self.local_url = await self._async_check_url( | |||
temp_local_url, | |||
) | |||
except ClientConnectorError as exc: | |||
except (TimeoutError, ClientConnectorError) as exc: | |||
LOG.debug("Cannot connect to %s - reason: %s", temp_local_url, exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Consider modifying the logging to distinguish between timeout and connection errors.
The current logging doesn't differentiate between TimeoutError and ClientConnectorError. Consider updating the log message to provide more specific information about the type of error encountered. This could aid in debugging and monitoring.
LOG.debug("Cannot connect to %s - reason: %s", temp_local_url, exc) | |
if isinstance(exc, TimeoutError): | |
LOG.debug("Timeout connecting to %s", temp_local_url) | |
elif isinstance(exc, ClientConnectorError): | |
LOG.debug("Connection error to %s - reason: %s", temp_local_url, exc) | |
else: | |
LOG.debug("Cannot connect to %s - reason: %s", temp_local_url, exc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rather use separate exceptions instead of handling within a separate if statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Certainly, @cgtobi! Using separate except
blocks for each exception type is a great approach for clarity and maintainability. It allows for more specific handling and logging of each error type. Here's how you could implement it:
try:
self.local_url = await self._async_check_url(temp_local_url)
except TimeoutError:
LOG.debug("Timeout connecting to %s", temp_local_url)
except ClientConnectorError as exc:
LOG.debug("Connection error to %s - reason: %s", temp_local_url, exc)
This way, each exception is handled individually, making the code easier to read and the logs more informative. Let me know if you need further assistance!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for that fix. But please use separate exceptions than if.
hi @cgtobi do you think it could be possible to create a 8.10.1 release with this fix ? 🙏 |
On my NOC module, I'm experiencing timeout for local url. It makes whole home update throw.
Client already catches connectivity error, but timeout wasn't part of them.
Summary by Sourcery
Catch timeout errors in the camera URL update process to improve error handling and prevent disruptions in the home update process.
Bug Fixes: