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.

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

Fix MQTT re-subscription logic #18953

Merged
merged 4 commits into from
Dec 3, 2018

Conversation

OttoWinter
Copy link
Member

@OttoWinter OttoWinter commented Dec 3, 2018

Description:

The old re-subscription logic was wrong and re-subscribing when the topic didn't change (see log in attached issue). I didn't step through the old code to figure out what the issue was because it was quite messy and hard to understand. But I suspect it's two things combined:

  • the comparison of the message callback (will never be the same if an anonymous method)
  • the unsub() callback directly followed by the re-subscribe coroutine (a race condition)

Instead, I rewrote the code to be (I think) easier to understand using some nice attr data classes.
Using some quick testing I was able to confirm that the issue is resolved with this.

Also, the tests seem to be incorrect. I manually stepped through them and couldn't understand why the previous checks were there. The new comparisons look more correct to me when manually stepping through. Nevermind (the tests are way too long and should probably be split up at some point)

(if there's another patch release, it would be great to include it there)

Related issue (if applicable): fixes #18909

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.

CC: @emontnemery @balloob

@balloob
Copy link
Member

balloob commented Dec 3, 2018

You can merge it when you're ready.

@OttoWinter OttoWinter force-pushed the fix-mqtt-resubscribe branch from 62ea0e1 to da7c44f Compare December 3, 2018 13:10
@OttoWinter OttoWinter merged commit c8d92ce into home-assistant:dev Dec 3, 2018
@ghost ghost removed the in progress label Dec 3, 2018
@OttoWinter OttoWinter deleted the fix-mqtt-resubscribe branch December 3, 2018 18:17
@balloob balloob added this to the 0.83.4 milestone Dec 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MQTT sensors hangs / doesn't change state
4 participants