Fix homeassistant.start trigger#8220
Conversation
|
I'd like to see a test for this, to prove the functionality and to protect against future breakage. Also, this looks like it broke a bunch of tests. |
| self._pending_tasks.clear() | ||
| if pending: | ||
| yield from asyncio.wait(pending, loop=self.loop) | ||
| else: |
There was a problem hiding this comment.
The reason we yield inside the while loop is so that we give scheduled callbacks (which are called with loop.call_soon) time to execute as they are not added to self._pending_tasks.
There was a problem hiding this comment.
thanks for the explanation!
|
Thanks for analyse. I think a better fix is to add here: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/core.py#L171 a sleep 0 with a comment. |
|
Hi @pvizeli, there the sleep(0) won't work (tried). But just before Any idea on the explanatory comment? |
…k_till_done changes
|
Looks good. I was never able to reproduce it |
|
@pvizeli, In my test/dev machine, with a simple yaml config with a telegram_bot, some cameras, a media_player, some scripts and automations, without this fix I tried to change Thanks for reviewing, btw! |
|
That is not possible to have test for that. It need a perfect timing to run into this problem. |
|
Can confirm this also fixes it for me, nice work @azogue |
* Fix homeassistant.start trigger * ooops * set sleep(0) just before changing to running state, revert async_block_till_done changes
Description:
Fix the
async_block_till_donecoroutine in order to fix thehomeassistant.starttrigger.In my experience, when passing through
components/automation/homeassistant.py:async_trigger(),core.py:async_start()is finished, so state isCoreState.runningand condition is never met (as commented in #8185)TheIt looks like a lastwhileloop has no meaning there with the call tolist.clear()in its body, andyield from asyncio.sleep(0)is needed to trigger the automations as expected.Second attempt, I think a little more elegant than #8216 (I couldn't integrate changes in the same branch, so I'm making a new PR here), which I'm closing now.
Related issue (if applicable): fixes #8185 / #7058
Example entry for
configuration.yaml(if applicable):Checklist:
If the code does not interact with devices:
toxrun successfully. Your PR cannot be merged unless tests pass