8000 Fix homeassistant.start trigger by azogue · Pull Request #8220 · home-assistant/core · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix homeassistant.start trigger #8220

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
Merged
merged 3 commits into from
Jun 27, 2017
Merged

Fix homeassistant.start trigger #8220

merged 3 commits into from
Jun 27, 2017

Conversation

azogue
Copy link
Member
@azogue azogue commented Jun 26, 2017 •
edited
Loading

Description:

Fix the async_block_till_done coroutine in order to fix the homeassistant.start trigger.

In my experience, when passing through components/automation/homeassistant.py:async_trigger(), core.py:async_start() is finished, so state is CoreState.running and condition is never met (as commented in #8185)

The while loop has no meaning there with the call to list.clear() in its body, and It looks like a last yield 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):

automation:
- alias: TEST HA start
  trigger:
    platform: homeassistant
    event: start
  action:
    - service: persistent_notification.create
      data:
        title: 'HA init'
        message: "Home Assistant start: {{ as_timestamp(now())| timestamp_local}}."
        notification_id: "init_notif"

Checklist:

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass

@mention-bot
Copy link

@azogue, thanks for your PR! By analyzing the history of the files in this pull request, we identified @balloob, @pvizeli and @fabaff to be potential reviewers.

@azogue azogue mentioned this pull request Jun 26, 2017
Closed
1 task
@lwis
Copy link
Member
lwis commented Jun 26, 2017

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.

balloob
balloob reviewed Jun 27, 2017
View reviewed changes
homeassistant/core.py Outdated
@@ -262,8 +262,8 @@ def async_block_till_done(self):
self._pending_tasks.clear()
if pending:
yield from asyncio.wait(pending, loop=self.loop)
else:
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

thanks for the explanation!

@pvizeli
Copy link
Member 8000
pvizeli commented Jun 27, 2017 •
edited
Loading

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.

@azogue
Copy link
Member Author
azogue commented Jun 27, 2017

Hi @pvizeli, there the sleep(0) won't work (tried). But just before self.state = CoreState.running is working ok. That was the first attempt of a fix, in #8216, but looked somehow non natural, so I started to messing around async_block_till_done (making a last and unconditional sleep(0) there also works)

Any idea on the explanatory comment?

@pvizeli
Copy link
Member
pvizeli commented Jun 27, 2017

Looks good. I was never able to reproduce it

@azogue
Copy link
Member Author
azogue commented Jun 27, 2017

@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 homeassistant.start never triggers, but with it always works as expected. The only 'non standard' configuration I can think of about this test machine is that automations are defined in configuration.yaml and in other yaml files (config with packages), but this couldn't make any difference, isn't it?

I tried to change tests/components/automation/test_homeassistant.py to reproduce this behaviour, but I failed. The test always passes, with or without this fix.

Thanks for reviewing, btw!

@pvizeli
Copy link
Member
pvizeli commented Jun 27, 2017

That is not possible to have test for that. It need a perfect timing to run into this problem.

@pvizeli pvizeli merged commit e39f7d3 into home-assistant:dev Jun 27, 2017
@lwis
Copy link
Member
lwis commented Jun 27, 2017

Can confirm this also fixes it for me, nice work @azogue

@balloob balloob mentioned this pull request Jul 1, 2017
Merged
@azogue azogue deleted the automation-start branch July 30, 2017 09:49
dethpickle pushed a commit to dethpickle/home-assistant that referenced this pull request Aug 18, 2017
@azogue @dethpickle
Fix homeassistant.start trigger (home-assistant#8220)
11de144 
* Fix homeassistant.start trigger

* ooops

* set sleep(0) just before changing to running state, revert async_block_till_done changes
@home-assistant home-assistant locked and limited conversation to collaborators Dec 11, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@balloob balloob balloob left review comments

@pvizeli pvizeli pvizeli approved these changes

Assignees
No one assigned
Labels
cla-signed core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

homeassistant.start not triggering automations
7 participants
@azogue @mention-bot @lwis @pvizeli @balloob @balloobbot @homeassistant
0