Skip to content

Fix async IO in Sesame lock component.#11054

Merged
pvizeli merged 7 commits into
home-assistant:devfrom
veleek:dev
Dec 22, 2017
Merged

Fix async IO in Sesame lock component.#11054
pvizeli merged 7 commits into
home-assistant:devfrom
veleek:dev

Conversation

@veleek
Copy link
Copy Markdown
Contributor

@veleek veleek commented Dec 9, 2017

Description:

Switch Sesame lock component to use async_add_devices and force the object to cache all it's properties. Unless someone manually overrides the value of use_cached_state on the internal sesame instance there will be no async IO in the component properties.

Checklist:

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

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable ([example][ex-requir]).
  • New dependencies are only imported inside functions that use them ([example][ex-import]).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

@homeassistant
Copy link
Copy Markdown
Contributor

Hi @veleek,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

Comment thread homeassistant/components/lock/sesame.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace

Comment thread homeassistant/components/lock/sesame.py Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

trailing whitespace

Copy link
Copy Markdown
Contributor

@emlove emlove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, please get rid of the phrase "fixes #4210" from the PR description, or that issue will be closed when this is merged.

Comment thread homeassistant/components/lock/sesame.py Outdated
def setup_platform(hass, config: ConfigType,
add_devices: Callable[[list], None], discovery_info=None):
@asyncio.coroutine
def async_setup_platform(
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't switch to async_setup_platform. This isn't an async platform. Adding update_before_add=True should be the only necessary change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@armills. Got it, so this would only be needed if the actual update methods (and the underlying update in pysesame) was async, correct?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly. You'd need to switch to the async equivalents if you had other async coroutines you needed to call from setup/update on the external lib.

@home-assistant home-assistant deleted a comment from houndci-bot Dec 10, 2017
Copy link
Copy Markdown
Member

@balloob balloob left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work as properties are always evaluated in an asyncio context. The correct fix is to assign all properties that we want to read from self._sesame to instance variables inside the update method.

@veleek
Copy link
Copy Markdown
Contributor Author

veleek commented Dec 11, 2017

@balloob I'm not sure what won't work. Pysesame defines the properties like this:

    @property
    def nickname(self):
        """Return the Sesame nickname."""
        if not self.use_cached_state:
            self.update_state(False)
    return self._nickname

So since I'm adding update_before_add, self.use_cached_state will always be true and it will just return the local value of self._nickname. I guess I'm not sure what you mean by "always evaluated in an asyncio context".

I'm happy to change to caching all of the pysesame properties inside the HA Sesame object, I just want to make sure that I understand why I'm doing it (my first commit to the project an all 😄).

@emlove
Copy link
Copy Markdown
Contributor

emlove commented Dec 11, 2017

It would be best as @balloob said to use the update method to cache everything into the home assistant class. Although this does correct the problem, it's cleanest not to depend on the upstream behavior of sometimes blocks sometimes returns immediately.

@veleek
Copy link
Copy Markdown
Contributor Author

veleek commented Dec 11, 2017

Sounds good. Thanks for the feedback. I'll make the changes.

Copy link
Copy Markdown
Contributor

@emlove emlove left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small request, otherwise looks great!

Comment thread homeassistant/components/lock/sesame.py Outdated
_sesame = None

# Cached sesame properties
_device_id = None
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Declare these inside of __init__ instead of here. Variables declared under the class are static for all instances of the class. We can get rid of the _sesame = None above as well. Should just look like this in __init__:

self._sesame = sesame
self._device_id = None
self._nickname = None
...

@pvizeli pvizeli merged commit 295caeb into home-assistant:dev Dec 22, 2017
@pvizeli pvizeli added this to the 0.60.1 milestone Dec 22, 2017
balloob pushed a commit that referenced this pull request Jan 5, 2018
* Call update on Sesame devices to cache initial state

* Switch to using async_add_devices

* Fix line length

* Fix Lint errors

* Fix more Lint errors

* Cache pysesame properties

* Updates from CR feedback
@balloob balloob mentioned this pull request Jan 5, 2018
@home-assistant home-assistant locked and limited conversation to collaborators May 29, 2018
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.

7 participants