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

Add an asyncio Lock around pairing, which cant be used concurrently #21933

Merged
merged 1 commit into from Mar 11, 2019

Conversation

Projects
None yet
3 participants
@Jc2k
Copy link
Contributor

commented Mar 11, 2019

Description:

I'll need to rebase this after #21901 is merged to fix a conflict in the update() method.

I was recently discussing plans for homekit_controller with another user/contributor and realised that there is no explicit locking around the pairing. The homekit_controller Pairing object is not safe to use concurrently, so users with bridges may be experiencing glitches as the asynchronicity trips up the session encryption.

I think there is already some logic around EntityPlatforms that limits concurrent polls (PARALLEL_UPDATES) which means you are unlikely to see problems with just polling. However if you have concurrent writes (or a write and poll at the same time) then you'll get a AccessoryDisconnected error and the request will fail. In one of my longer running branches to add BLE homekit support I actually noticed this quite a lot with even a single device. BLE requests are quite slow so multi tapping on UI elements was enough to trigger the problem.

It seemed like it would be easiest to reason about this if we used the async_ version of the entity API as much as possible and handled the locking needed for the pairing object on the HA side as close to the pairing object as possible. I also wanted an approach that played well with future plans to make an asyncio variant of homekit_python (the upstream we are using for this component).

I have tested this with a homekit_python demoserver.py setup as a fake bridge that has 50 fake light bulbs attached to it, and the tests pass locally.

As I was editing an adjacent function i've removed update_characteristics as its no longer used.

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.
@balloob
Copy link
Member

left a comment

Looking good 👍

@balloob balloob merged commit 5e2302e into home-assistant:dev Mar 11, 2019

4 checks passed

Hound No violations found. Woof!
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 92.75%
Details

@ghost ghost removed the in progress label Mar 11, 2019

@Jc2k Jc2k deleted the Jc2k:homekit_concurrency branch Mar 11, 2019

Jc2k added a commit to Jc2k/home-assistant that referenced this pull request Mar 12, 2019

@Jc2k Jc2k referenced this pull request Mar 12, 2019

Merged

Fix regression introduced by #21933 #21988

3 of 3 tasks complete

robbiet480 added a commit that referenced this pull request Mar 13, 2019

@balloob balloob referenced this pull request Mar 20, 2019

Merged

0.90.0 #22216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.