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.

Sign up for GitHub

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

Jump to bottom

Fix non thread safe state write in acmeda integration #125483

Draft
wants to merge 4 commits into
base: dev
Choose a base branch
from
Draft

Fix non thread safe state write in acmeda integration #125483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1852dd7
Replaced callback to write_ha_state with schedule_update_ha_state
atmurray Sep 8, 2024
3554ed6
Change to use existing hass asyncio event loop in aiopulse module.
atmurray Sep 8, 2024
624637c
Merge branch 'dev' into acmeda-fixnotify
atmurray Sep 8, 2024
bdc0769
Merge branch 'dev' into acmeda-fixnotify
atmurray Sep 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions homeassistant/components/acmeda/base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@

async def async_added_to_hass(self) -> None:
"""Entity has been added to hass."""
self.roller.callback_subscribe(self.notify_update)
self.roller.callback_subscribe(self.async_notify_update)

Check warning on line 47 in homeassistant/components/acmeda/base.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/acmeda/base.py#L47 

Added line #L47 was not covered by tests

self.async_on_remove(
async_dispatcher_connect(
Expand All @@ -56,10 +56,10 @@

async def async_will_remove_from_hass(self) -> None:
"""Entity being removed from hass."""
self.roller.callback_unsubscribe(self.notify_update)
self.roller.callback_unsubscribe(self.async_notify_update)

Check warning on line 59 in homeassistant/components/acmeda/base.py

View check run for this annotation

Codecov / codecov/patch

homeassistant/components/acmeda/base.py#L59 

Added line #L59 was not covered by tests

@callback
Copy link
Member

Choose a reason for hiding this comment

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

If I remember correctly, callbacks can't be async

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Within the library, the callback resolver looks at the callback function and if it is a coroutine (async) creates a task on the event loop, otherwise it runs the callback on the executor.

Copy link
Member

Choose a reason for hiding this comment

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

I'll wait for bdraco on this one, since the next patch will be out next friday, we don't have a rush to merge :)

Copy link
Member

Choose a reason for hiding this comment

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

This will work but its very inefficient since creating a task is an order of magnitude more latent than executing a callback.

In core we have a decorator, @callback to tell core how to run a callback.

If its decorated with @callback it runs in the loop, if not it runs in the executor

core/homeassistant/core.py

Line 247 in 5405279

def callback[_CallableT: Callable[..., Any]](func: _CallableT) -> _CallableT:

core/homeassistant/core.py

Line 253 in 5405279

def is_callback(func: Callable[..., Any]) -> bool:

core/homeassistant/core.py

Line 387 in 5405279

if is_callback(check_target):

In the library you could do something similar, however if you are always running everything in the same loop there is likely no need to ever run anything in the executor and you could run the callbacks directly in the library and remove the executor code.

def notify_update(self) -> None:
async def async_notify_update(self) -> None:
"""Write updated device state information."""
LOGGER.debug("Device update notification received: %s", self.name)
self.async_write_ha_state()
Expand Down
3 changes: 1 addition & 2 deletions homeassistant/components/acmeda/hub.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,11 @@
class PulseHub:
"""Manages a single Pulse Hub."""

api: aiopulse.Hub

def __init__(self, hass: HomeAssistant, config_entry: ConfigEntry) -> None:
"""Initialize the system."""
self.config_entry = config_entry
self.hass = hass
self.api = aiopulse.Hub(hass.loop)
self.tasks: list[asyncio.Task[None]] = []
self.current_rollers: dict[int, aiopulse.Roller] = {}
self.cleanup_callbacks: list[Callable[[], None]] = []
Expand Down
Loading