Renew auth on eventsocket and improve reconnect handling

abmantis / whirlpool-sixth-sense

Whirlpool unofficial API for 6th Sense appliances

MIT License

13 stars 12 forks source link

Renew auth on eventsocket and improve reconnect handling #13

Closed mkmer closed 1 year ago

mkmer commented 1 year ago

I believe this will address the authkey renewal. Also found that connection timeout needs to be set or the event socket hangs at connect when the host decides not to connect...

mkmer commented 1 year ago

After much struggling with a "dumb" call to the logger, it has survived multiple days. There appears to be 3 "problems" with the base code:

eventsocket didn't renew authkey. After disconnect it couldn't reconnect.
eventsocket has a retry count that caused the entire thread to stop and never run again without any log messages. We are only allowed 3 reconnects total for the life of the thread... It probably should reset after a successful connect at a minimum (or as it is in the PR: just keep trying "nicely")
on connection, if the host failed to respond the timeout was disabled (hangs thread) - added connect timeout

abmantis commented 1 year ago

I noticed that this is getting a few extra changes in addition to the websocket fixes, right? If so, please keep this PR only for the websocket thing. You can open another one for the rest.

I've made the PR a Draft. Please mark it as "Ready" when you feel it is ready for review (after removing non-websocket changes).

mkmer commented 1 year ago

I could do that... or we can have on PR that will fit with the HA release. HA with the laundry appliances will need everything I have added. Will that make sense?

On Wed, Sep 14, 2022 at 7:43 PM Abílio Costa @.***> wrote:

I noticed that this is getting a few extra changes in addition to the websocket fixes, right? If so, please keep this PR only for the websocket thing. You can open another one for the rest.

I've made the PR a Draft. Please mark it as "Ready" when you feel it is ready for review (after removing non-websocket changes).

— Reply to this email directly, view it on GitHub https://github.com/abmantis/whirlpool-sixth-sense/pull/13#issuecomment-1247406932, or unsubscribe https://github.com/notifications/unsubscribe-auth/AB3GVBEAIWAJ5EGKTLCI7PTV6JPI3ANCNFSM575VOQ5Q . You are receiving this because you authored the thread.Message ID: @.***>

abmantis commented 1 year ago

The release is done from the master branch, so the PR needs to get merged first. There is no problem in having things entering master in short PRs :) It is usually easier to review and work on smaller PRs, that was why I was asking to have smaller and separate ones.

If you need (for testing and so on) you can always base one PR branch on the other PR branch so you can have everything in the second one, for example.

mkmer commented 1 year ago

The socket has been alive for a week on my HA instance, happy to report recovery from multiple rauths and 1001 closes. I'm feeling like it's solid :)

abmantis commented 1 year ago

Thanks for your work!