Errors and exceptions handling improvement #43
Conversation
|
Currently testing this version on my HA instance. I have tested by forcing a returned status to 500 at random, and it effectively resends the same request after 0.5s repeatedly, until the return status is 200. If the 5 tries all failed with 500, then it aborts and raise a |
|
And of course, since I started testing this fix, the 500 error status no longer appear for me ... Making it ready for review anyway, still testing |
|
|
||
| _LOGGER.debug(f"Call service {url} OK") | ||
| return response_in_json | ||
|
|
||
| def filtered_request(self, method: str, path: str, **kwargs) -> Any: |
There was a problem hiding this comment.
@MrXANA91 I don't like to have unused code, what about the request api? Is it still used?
In case not, change request api instead of add a new one, and add a parameter for "retry" that could be MAX_TRRIES as default and with 0 you should have the same behavior of the old request implementation.
There was a problem hiding this comment.
If you check line 73, you can see a call to the request API. filtered_request just wraps the current request method to catch any 500 status that could potentially appear.
First request is sent (considered first try). If a 500 status is caught and MAX_TRIES is greater than 1, the request is resent after waiting 500ms (or the value of DELAY_BETWEEN_TRIES_MS). If every requests throw a 500 error code, a total number of MAX_TRIES (here is 5) requests will be sent before an error pops up in Home Assistant.
If you set the value of MAX_TRIES to 1, this code will behave exactly like it currently does in the release.
There was a problem hiding this comment.
mmmm ok, good point. Anyway, I don't like the name, why it is filtered?
Could you use for:else (https://www.w3schools.com/python/gloss_python_for_else.asp) returning the result instead of breaking the loop?
There was a problem hiding this comment.
Anyway, I prefer to make request "private" -> so you could name your method request and I would like to accept the retry as parameter that per default it is equals to MAX_RETRIES
There was a problem hiding this comment.
Got it, I'll make the changes.
Do you want the delay value as a parameter as well?
There was a problem hiding this comment.
Yup, that's great.
Thanks a lot for your help and passion!
- setting original 'request' method as private - renamed 'filtered_request' to 'request'
- maxNumberOfTrails defaults to MAX_TRIES - delayMSBetweenTrials defaults to DELAY_BETWEEN_TRIES_MS
| for tryNumber in range(maxNumberOfTrials): | ||
| try: | ||
| result = self.__request(method, path, **kwargs) | ||
| break |
There was a problem hiding this comment.
@KiraPC
I have to keep the break statement at this location in order to get out of the loop if the request succeeds.
I added the else statement which is better than checking the tryNumber but I don't think that I can get rid of the break statement unless I completely rethink the way the trials are handled.
There was a problem hiding this comment.
@MrXANA91 As I suggested, you should use a return statement in case of success directly inside the try block, and the else statement is executed just when the loop end without result
|
Going back to draft during testing Not tested yet, I'll let you know |
|
A few minutes ago, when trying to send a
So the fix is actually working properly. Marking it as ready! |
|
Great, merging |
This PR aims to fix #36 and to generally improve the exceptions potentially raised by the integration using the Home Assistant exception classes
If the Switchbot servers return a 500 status, the integration will send a warning to the Home Assistant logs.
It will wait 0.5s and will then try to resend the same command.
After 5 consecutive 500 status failures, it will (should ?) abort and raise an error to Home Assistant.