-
-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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 non thread safe state write in acmeda integration #125483
base: dev
Are you sure you want to change the base?
Conversation
The underlying module uses asyncio and the callback is run within the library's event loop.
This change was previously included in the pull request #124960 but has been split out into a dedicated pull request. |
@bdraco would you mind checking this out. |
Yes that would be much better if possible.
|
Change callback to be async so that it is added to the event loop and not the executor.
OK done, that was surprisingly easy. Only additional thing I had to do was to make the callback async so that it ran within the event loop. |
@@ -56,10 +56,10 @@ async def async_added_to_hass(self) -> None: | |||
|
|||
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) | |||
|
|||
@callback |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
Line 247 in 5405279
def callback[_CallableT: Callable[..., Any]](func: _CallableT) -> _CallableT: |
Line 253 in 5405279
def is_callback(func: Callable[..., Any]) -> bool: |
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.
This PR has a merge conflict, could you take a look @atmurray ? ../Frenck |
Breaking change
Proposed change
The underlying module uses asyncio and the callback is run within the library's event loop.
As such, the following runtime error was being thrown when calling async_write_ha_state:
RuntimeError: Detected that integration 'acmeda' calls async_write_ha_state from a thread other than the event loop, which may cause Home Assistant to crash or data to corrupt. For more information, see https://developers.home-assistant.io/docs/asyncio_thread_safety/#async_write_ha_state at homeassistant/components/acmeda/base.py, line 65: self.async_write_ha_state().
To resolve this, the library is now initialised to use the existing event loop of home assistant. The callbacks are now async so that they are run within the home assistant event loop thereby allowing async_write_ha_state to be called.
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.To help with the load of incoming pull requests: