Annotate more async functions correctly #31802
Conversation
balloob
requested review from
Jc2k,
MartinHjelmare,
OttoWinter,
robbiet480 and
a team
as code owners
probot-home-assistant
bot
added
core
integration: coolmaster
integration: esphome
integration: homekit_controller
integration: mysensors
integration: sonos
integration: starline
integration: upnp
small-pr
|
Hey there @OttoWinter, mind taking a look at this pull request as its been labeled with a integration ( |
|
Hey there @Anonym-tsk, mind taking a look at this pull request as its been labeled with a integration ( |
|
Hey there @robbiet480, mind taking a look at this pull request as its been labeled with a integration ( |
|
Hey there @MartinHjelmare, mind taking a look at this pull request as its been labeled with a integration ( |
| @@ -150,11 +151,13 @@ def _turn_on_rgb_and_w(self, hex_template, **kwargs): | |||
| self._values[value_type] = STATE_OFF | |||
| self.async_schedule_update_ha_state() | |||
|
|
|||
| @callback | |||
There was a problem hiding this comment.
These methods are just called internally and don't need to be decorated.
There was a problem hiding this comment.
I think that @callback is not just a decorator to instruct Home Assistant on how to execute the function, it's also an indicator fo rthe reader that this is a function that should be called from within the event loop.
There was a problem hiding this comment.
Like I wrote in the other PR. We will never be able to name functions or decorate things to 100% so we can't trust this. Sure it might help where it does match but since we can't trust the existence or absence of this decorator to be the truth about where a function should be run, I don't think we should spend time on trying to add this everywhere.
There was a problem hiding this comment.
If we do want to use a system like this for the purpose you describe and be able to trust it, we would have to introduce another decorator, called eg sync and decorate all sync functions with it. Then we could check if a function is a coroutine, a callback that should run in the event loop or a sync function that should not run in the event loop. We would know that an undecorated function that isn't a coroutine would be unmarked and we could exclude that function from the trust chain.
There was a problem hiding this comment.
Well this is my third PR doing this actually, and I did find some issues where things were accidentally being called in the executor instead of a callback. I think that sticking to prefixing function names with async_ and adding either async def or @callback will help us catch these things. It's not 100%, but it's better than nothing. Async stuff running outside of the event loop can cause a whole bunch of issues, as async functions are not thread safe (because they are run after one another).
There was a problem hiding this comment.
I am not 100% convinced of a sync decorator, as this is about functions that have the async_ prefix. For sync functions there is no disctinction. Right now it's no decorator -> it's sync.
There was a problem hiding this comment.
Well, that's the problem. Without a decorator we can't tell if someone has looked at the function and decided it's not async or just haven't looked at it.
There was a problem hiding this comment.
We would need two new decorators, probably. One sync for functions that should not run in the event loop, and one safe for functions that can both run in the event loop or in another thread.
So in total it's four different type of functions.
There was a problem hiding this comment.
We could consider that. I don't think that's a priority.
I've found real bugs making sure all functions starting with async_ are decorated or async. And so we should continue doing so.
Codecov Report
@@ Coverage Diff @@
## dev #31802 +/- ##
==========================================
- Coverage 94.68% 94.67% -0.01%
==========================================
Files 763 763
Lines 55128 55151 +23
==========================================
+ Hits 52197 52214 +17
- Misses 2931 2937 +6
Continue to review full report at Codecov.
|
Breaking change
Proposed change
In my previous PRs I looked for functions where the name started with
async_but were not async and had no@callbackdecorator. Now I expanded the search for functions starting with_async_Type of change
Example entry for
configuration.yaml:# Example configuration.yamlAdditional information
Checklist
black --fast 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..coveragerc.The integration reached or maintains the following Integration Quality Scale: