fix: handle auth expiring #460
Conversation
|
We will likely need to do something similar in the new mqtt side once everything is finalized. |
|
I am also taking this PR to try to decrease the amount of times we completely reauth in the old system. Right now we are disconnecting and reconnecting, but now we will attempt to unsub and resub 1 time, if that doesn't fix our problem, then we will do the normal reauth. As well, I bumped up keepalive to 70 seconds as our cloud poll time for not cleaning in HA is 60 seconds which matched it, so I could see that causing issues. Ideally this is configurable, but imo it doesn't make sense to do major refactoring as this is going to be a dead part of the code soon enough anyways |
|
Did some simplification. I don't think i specifically want to request re-auth, as waiting is a fine solution. |
roborock/cloud_api.py
Outdated
| raise RoborockException(f"Failed to publish ({mqtt.error_string(info.rc)})") | ||
|
|
||
| async def validate_connection(self) -> None: | ||
| """Override the default validate connection to try to re-subscribe rather than disconnect.""" |
There was a problem hiding this comment.
I'm having trouble following everything going on here because of:
- the new state member variable
- keepalive
- interaction with the parent class, invoking parent calss validate connection 4 different ways
Could this have an opening description of (1) big picture whats supposed to be happening here then (2) at a detailed level what these flows are and how the 3 things interact? Im also unsure how this pare relates to the error handling issue above.
A suggestion for simplification would be to no longer call the parent class validate_connection logic. Instead move that down into the local and cloud clients and inline it here so there are fewer interactions with the parent class.
There was a problem hiding this comment.
Take a look now, i broke things out into some helper functions
Co-authored-by: Allen Porter <allen.porter@gmail.com>
|
Anything outstanding here @allenporter? |
roborock/cloud_api.py
Outdated
| connection_queue.set_result(True) | ||
|
|
||
| def _mqtt_on_message(self, *args, **kwargs): | ||
| self.received_message_since_last_disconnect = False |
There was a problem hiding this comment.
Should this now be True?
Unless i misunderstood the naming of this.
There was a problem hiding this comment.
Yes good call - it did not match the new naming. fixed.
Seemingly, tokens now sometimes expire. I'm not sure how long it takes, but the issue is being seen in a few issues. Our error handling logic was wrong in the old system (It was still operating under some mqtt3 logic instead of 5.)
Now, we identify bad auth errors, and pass forward a exception that makes that clear. Then we can prompt a reauth on the HA side.