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 harmony failing to switch activities when a switch is in progress #47212

Merged
merged 2 commits into from
Mar 2, 2021

Conversation

bdraco
Copy link
Member

@bdraco bdraco commented Mar 1, 2021

Proposed change

Fix harmony failing to switch activities when a switch is in progress

If an activity switch was in progress and another one is asked for, the
second switch would be dropped. This is most apparent when switching with
HomeKit since HomeKit restores the last activity, and then its impossible to
switch to another one because the switch is in progress already.

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example entry for configuration.yaml:

# Example configuration.yaml

Additional information

  • This PR fixes or closes issue: fixes #
  • This PR is related to issue:
  • Link to documentation pull request:

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • The code has been formatted using Black (black --fast homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

  • No score or internal
  • 🥈 Silver
  • 🥇 Gold
  • 🏆 Platinum

To help with the load of incoming pull requests:

@probot-home-assistant
Copy link

Hey there @ehendrix23, @bramkragten, @mkeesey, mind taking a look at this pull request as its been labeled with an integration (harmony) you are listed as a codeowner for? Thanks!
(message by CodeOwnersMention)

@bdraco bdraco added this to the 2021.3.0 milestone Mar 1, 2021
@mkeesey
Copy link
Contributor

mkeesey commented Mar 1, 2021

I'll be able to take a look at this tonight Denver time.

Co-authored-by: Paulus Schoutsen <paulus@home-assistant.io>
@balloob balloob merged commit d02218f into home-assistant:dev Mar 2, 2021
balloob added a commit that referenced this pull request Mar 2, 2021
…#47212)

Co-authored-by: Paulus Schoutsen <paulus@home-assistant.io>
@mkeesey
Copy link
Contributor

mkeesey commented Mar 2, 2021

tl;dr - The locking implementation seems reasonable and this seems fine. This also works fine locally. I'm not fast enough to trigger the original problem on the old code (and didn't really feel like I needed to write a huge test for it), but seems fine.

I think this will work fine for what you are trying to fix (one activity start followed very quickly by another), but will probably have surprising behavior if more than two go in flight at once since order will probably not be preserved by the lock (the second activity might start "last" even if a third is submitted because of lock fairness). Don't think we need to go down the road of using a queue though unless it really comes up a lot.

@bdraco
Copy link
Member Author

bdraco commented Mar 2, 2021

tl;dr - The locking implementation seems reasonable and this seems fine. This also works fine locally. I'm not fast enough to trigger the original problem on the old code (and didn't really feel like I needed to write a huge test for it), but seems fine.

I think this will work fine for what you are trying to fix (one activity start followed very quickly by another), but will probably have surprising behavior if more than two go in flight at once since order will probably not be preserved by the lock (the second activity might start "last" even if a third is submitted because of lock fairness). Don't think we need to go down the road of using a queue though unless it really comes up a lot.

If we switch to a queue for this so we could discard all but the last one when the user hits the wrong one multiple times. That would definitely be an improvement but seems unlikely to happen

@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants