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

Camera platform for buienradar imagery #23358

Merged
merged 12 commits into from Jun 11, 2019

Use asyncio.Conditions instead of asyncio.Lock

  • Loading branch information...
ties committed Apr 24, 2019
commit b7862732f1b2005d8da1d2361ad344bfce8806e9
@@ -9,10 +9,12 @@

from homeassistant.components.camera import PLATFORM_SCHEMA, Camera
from homeassistant.const import CONF_ID, CONF_NAME

from homeassistant.helpers import config_validation as cv
from homeassistant.helpers.aiohttp_client import async_get_clientsession

from homeassistant.util import dt as dt_util
from homeassistant.util.async_ import run_coroutine_threadsafe

CONF_DIMENSION = 'dimension'
CONF_INTERVAL = 'interval'
@@ -22,9 +24,12 @@

_LOG = logging.getLogger(__name__)

# Maximum range according to docs
DIM_RANGE = vol.All(vol.Coerce(int), vol.Range(min=120, max=700))

PLATFORM_SCHEMA = vol.All(
PLATFORM_SCHEMA.extend({
vol.Optional(CONF_DIMENSION): cv.positive_int,
vol.Optional(CONF_DIMENSION): DIM_RANGE,
vol.Optional(CONF_INTERVAL): cv.positive_int,
vol.Optional(CONF_NAME): cv.string,
}))
@@ -36,7 +41,7 @@
c_id = config.get(CONF_ID)
dimension = config.get(CONF_DIMENSION) or 512
This conversation was marked as resolved by ties

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare May 6, 2019

Member

Please move all config defaults to the config schema.

interval = config.get(CONF_INTERVAL) or 600
name = config.get(CONF_NAME) or "Buienradar Radar Loop"
name = config.get(CONF_NAME) or "Buienradar loop"

async_add_entities([BuienradarCam(hass, name, c_id, dimension, interval)])

@@ -47,10 +52,19 @@ class BuienradarCam(Camera):
_deadline: Optional[int]
_name: str
_interval: Optional[int]
_condition: asyncio.Condition
""" Loading status """
_loading: bool

_last_image: Optional[bytes]
""" last modified HTTP response header"""
_last_modified: Optional[str]
"""
A camera component producing animated buienradar radar-imagery GIFs.
Image URL taken from https://www.buienradar.nl/overbuienradar/gratis-weerdata
"""

def __init__(self, hass, name: str, c_id: Optional[str], dimension: int, interval: Optional[int]):
@@ -62,9 +76,11 @@ def __init__(self, hass, name: str, c_id: Optional[str], dimension: int, interva
self._dimension = dimension
self._interval = interval

self._lock = asyncio.Lock(loop=hass.loop)
self._condition = asyncio.Condition(loop=hass.loop)
This conversation was marked as resolved by ties

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare May 6, 2019

Member

If we create the condition before creating the entity, and pass the condition to the entity, we don't need to pass in hass to the entity.

We don't need to set loop explicitly on the condition if we're in a coroutine.

This comment has been minimized.

Copy link
@ties

ties May 7, 2019

Author Contributor

Creating a condition before creating the entity obscures the invariant that the condition is private to this entity. I would like keep the condition object internal because of this (sharing it would cause notify_all to notify where it shouldn't) and will document that.

I agree that it is not needed to store a private reference to hass, will add an optional parameter for the event loop since it is considered good practice to do so.

You imply an invariant here which was not clear to me from the documentation and other code. Note that this invariant is logical but does not necessarily hold.

I think that this holds: "async setup methods are called with the event loop of the thread (i.e. asyncio.get_event_loop() being equal to hass.loop". If this holds it would really help if part of the platform lifecycle is documented :)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare May 7, 2019

Member

I'm not 💯 on what you mean with invariant. Sorry, I don't have a computer science background.

You imply an invariant here which was not clear to me from the documentation and other code. Note that this invariant is logical but does not necessarily implicitly hold.

What are you referring to here? hass being set on the entity on addition to home assistant or current event loop being used for loop by default inside coroutines (inside the event loop)?

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare May 7, 2019

Member

We can create the condition inside init without passing in hass if we don't set loop explicitly. That's my suggestion after your explanation about condition being private, which makes sense.

This comment has been minimized.

Copy link
@ties

ties May 7, 2019

Author Contributor

My explanation was not 100% precise either, I removed an extraneous "implicitly". With invariant I mean "a property that always holds"/"it always is true that ..."

What are you referring to here? hass being set on the entity on addition to home assistant or
current event loop being used for loop by default inside coroutines (inside the event loop)?

To me it was not clear that hass.loop (in async_setup_component) is the same as the default event loop used by coroutines (inside the event loop). It feels logical but would cause potentially cause bugs if it is not. Being unsure I would prefer to pass it in explicitly

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare May 7, 2019

Member

The default loop in coroutines is the current loop used in the event loop that runs the coroutines. This is part of asyncio library. Since we're in a coroutine in async_setup_platform we don't need to care about what loop to use. We just ask asyncio to use the current loop by not setting it explicitly. This is safe.

This comment has been minimized.

Copy link
@ties

ties May 7, 2019

Author Contributor

The default loop in coroutines is the current loop used in the event loop that runs the coroutines. This is part of asyncio library. Since we're in a coroutine in async_setup_platform we don't need to care about what loop to use. We just ask asyncio to use the current loop by not setting it explicitly. This is safe.

There is a tiny, tiny caveat there. From my understanding, asyncio.get_running_loop gets the current loop. By default asyncio Lock's/Condiotion's/... use asyncio.get_event_loop().

This can be a different one, and will cause "surprising" issues in that situation (RuntimeError's or difficult to debug situations when there are multiple event loops)... Glad that is not the case here.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare May 7, 2019

Member

The docs on asyncio.get_event_loop doesn't say it can return a different loop than the current one, if I'm reading correctly. If there's no current loop set, it will create a new loop and set it as current. Yeah, there might be cases where this can be a problem. I don't see cases at the moment that I'm afraid should impact us.

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare May 7, 2019

Member

There's this note in the docs though, just below this section:

Note that the behaviour of get_event_loop(), set_event_loop(), and new_event_loop() functions can be altered by setting a custom event loop policy.

This comment has been minimized.

Copy link
@ties

ties May 8, 2019

Author Contributor

Yeah, there might be cases where this can be a problem. I don't see cases at the moment that I'm afraid should impact us.

Exactly! It is one of those 'should not happens' that cause people to be careful. I did a quick grep for this yesterday and the only place in this project (note that libraries can break this...) that could cause issues that I found is usage of homeassistant.util.async.asyncio_run which would unset the event loop afterwards.

But in coroutines it still seems to work in practice. See https://gist.github.com/ties/21d75aa31c399e6c9a100232d040f214 for a bunch of test cases :)

There's this note in the docs though, just below this section:

Note that the behaviour of get_event_loop(), set_event_loop(), and new_event_loop() functions can be altered by setting a custom event loop policy.

The general gist of this is what caused me to be defensive: As a new contributor you see that event loops are commonly passed in explicitly in the project and there are few guarantees.

I'll keep a note that it may be good to add the fact that the event loop returned by get_event_loop is assumed to be the correct one to the documentation. Without a custom event loop this is actually what seems to happen.

This comment has been minimized.

Copy link
@balloob

balloob May 9, 2019

Member

They fixed it in Python 3.5.3 so that get_event_loop called from inside an async function will always return the event loop that is running the async task: https://bugs.python.org/issue28613

(see also https://docs.python.org/3.5/whatsnew/changelog.html#python-3-5-3 and search for get_event_loop)

This comment has been minimized.

Copy link
@balloob

balloob May 9, 2019

Member

If you see code passing in the event loop, it's because it was most likely written prior to our minimum requirement being raised to 3.5.3

This comment has been minimized.

Copy link
@ties

ties May 9, 2019

Author Contributor

Really nice piece of clarification. Thanks! 👍


self._last_image = None
self._last_modified = None
self._loading = False
# deadline for image to be refreshed
self._deadline = None

@@ -81,45 +97,80 @@ def camera_image(self):

def __needs_refresh(self) -> bool:
if not (self._interval and self._deadline and self._last_image):
_LOG.info("refresh due to preconditions")
return True

if dt_util.utcnow() > self._deadline:
_LOG.info("refresh due to refresh interval")
return dt_util.utcnow() > self._deadline

async def __retrieve_radar_image(self) -> bool:
This conversation was marked as resolved by ties

This comment has been minimized.

Copy link
@balloob

balloob Apr 25, 2019

Member

Convention is that for private methods, use a single underscore prefix. Double underscore prefix is reserved for Python built-in methods.

This comment has been minimized.

Copy link
@ties

ties Apr 25, 2019

Author Contributor

I used double underscores since name mangling prevents conflicts with function names from parent classes.

This is different from __[builtin]__which has two trailing underscores. I will adjust this for __retrieve_radar_image but would prefer to keep it for the other case if mangled names are allowed in this project's style.

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2019

Member

Oh, TIL.

"""
Retrieve new radar image and return whether this succeeded.
"""
session: asyncio.ClientSession = async_get_clientsession(self._hass)

url = RADAR_MAP_URL_TEMPLATE.format(w=self._dimension,
h=self._dimension)

if self._last_modified:
headers = {'If-Modified-Since': self._last_modified}
else:
headers = {}

try:
async with session.get(url, timeout=5, headers=headers) as res:
if res.status == 304:
_LOG.debug("HTTP 304 - success")
return True
elif res.status != 200:
_LOG.error("HTTP %s - failure", res.status)
return False
else:
last_modified = res.headers.get('Last-Modified', None)
if last_modified:
self._last_modified = last_modified

self._last_image = await res.read()
_LOG.debug("HTTP 200 - Last-Modified: %s", last_modified)

return True
except (asyncio.TimeoutError, aiohttp.ClientError) as e:
_LOG.error("Failed to fetch image, %s", type(e))
return False


async def async_camera_image(self):
"""Return a still image response from the camera."""
"""
Return a still image response from the camera.
Uses ayncio conditions to make sure only one task enters the critical

This comment has been minimized.

Copy link
@balloob

balloob Apr 25, 2019

Member

I wonder if we need to make this a decorator that can be reused. We have similar logic inside Hue and probably will have it for other integrations that share a resource among requests.

This comment has been minimized.

Copy link
@ties

ties Apr 25, 2019

Author Contributor

I agree that this is a concern that is shared between multiple integrations (of different families). I will consider this and will get back on this later (likely this weekend), possibly with an implementation.

For now, I see three scenario's here:

  • generator function used as coroutine, seems to rule out async functions. I do not find this intuitive
  • a decorator. Advantage: does not depend on function name, does not need to be called explicitly. Tricky because it will take parameters (the time delta) from the instance of the class and this protocol is implicit.
  • an abstract base class with multiple inheritance. Advantage: Makes the lifecycle explicit. Disadvantage: more verbose

This comment has been minimized.

Copy link
@ties

ties Apr 29, 2019

Author Contributor

I think I have found the generic/re-usable use case, will try to write down this design pattern later this week due to time constraints.

In the meantime I have made an assumption about lifecycle of integrations. The state returned is the state at the time of the return, instead of the time of calling? This would allow calls to be '"[folded](https://en.wikipedia.org/wiki/Fold_(higher-order_function)" to single calls in my design pattern.

This pattern can combine multiple "request update" calls into one, or combine multiple light switch actions into a single action performed on an integration (turn on A, turn on B, turn on C => turn on (A, B, C,))

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2019

Member

State is at time of the return.

We don't do folded turn_on yet by platform. Right now we do allow platforms to specify max concurrency for calling services. States are only polled once all commands have been sent.

section at the same time. Othertiwse, two http requests would start
when two tabs with home assistant are open.
An additional boolean (_loading) is used to indicate the loading status
instead of _last_image since that is initialized to None.
"""
if not self.__needs_refresh():
_LOG.info(f"r: cached image {self._deadline}, {self._interval}")
return self._last_image

async with self._lock:
# check if image was loaded while locked
if not self.__needs_refresh():
_LOG.info(f"return fresh cached image {self._deadline}, "
f"{self._interval}")
# get lock, check iff loading, await notification if loading
async with self._condition:
if self._loading:
_LOG.debug("already loading - waiting for notification")
await self._condition.wait()
return self._last_image

_LOG.info(f"getting new image.")

# deadline has passed, get shared ClientSession to load new image.
# note that this session dies when another component (i.e. aiohue)
# throws.
session: asyncio.ClientSession = async_get_clientsession(self._hass)
# Set loading status **while holding lock**, makes other tasks wait
self._loading = True

try:
now = dt_util.utcnow()
url = RADAR_MAP_URL_TEMPLATE.format(w=self._dimension,
h=self._dimension)
try:
async with session.get(url, timeout=5) as res:
if res.status != 200:
_LOG.error("Failed to fetch image, %s", res.status)
else:
self._last_image = await res.read()
self._deadline = now + timedelta(seconds=self._interval)
_LOG.info(f"return newly loaded image, deadline: {self._deadline}, {self._interval}")
except (asyncio.TimeoutError, aiohttp.ClientError) as e:
_LOG.error("Failed to fetch image, %s", type(e))
res = await self.__retrieve_radar_image()
# successful response? update deadline to time before loading
if res:
self._deadline = now + timedelta(seconds=self._interval)

return self._last_image
finally:
# get lock, unset loading status, notify all waiting tasks
async with self._condition:
This conversation was marked as resolved by ties

This comment has been minimized.

Copy link
@balloob

balloob Apr 25, 2019

Member

If one request is waiting for the result, it does so inside the async with self._condition. Doesn't that mean it never releases the lock ? And if so, we will never be able to acquire the lock here?

This comment has been minimized.

Copy link
@ties

ties Apr 25, 2019

Author Contributor

There could be deadlocks in here if I missed something (especially with exceptions) but this is not one 🙂

await self._condition.wait() releases the lock and acquires it before continuing. I updated the docstring to clarify this

This comment has been minimized.

Copy link
@balloob

balloob Apr 30, 2019

Member

Oh, TIL.

self._loading = False
self._condition.notify_all()
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.