Fix automations listening to HOMEASSISTANT_START by balloob · Pull Request #6936 · home-assistant/core

balloob · 2017-04-04T16:13:31Z

Description:

To prevent false triggers, we only enable automations after all components are setup. This however introduced a breaking change: we no longer allow people to listen to HOMEASSISTANT_START.

This PR is just to start discussion, the included fix is actually a bad one.

I still want people to be able to run something on startup.

Make it backwards compatible by checking if listening for HOMEASSISTANT_START event and hass.state == CoreState.starting. This is a hack.
Add a new special automation platform that runs things on startup (and also checks against hass.state)

This PR forced me to fix how CoreState is set and ensures we wait till all tasks done before we start firing the timer. This caused some changes to how we mock HASS in tests.

Breaking changes

Deprecation: using automation event platform to listen for homeassistant_start on startup is deprecated and will be removed in Home Assistant 0.45. Use the new automation homeassistant platform instead.
Devs: during startup, hass.state is set to CoreState.starting. All tasks will be awaited during startup and the timer will only start when that is done. At that point in time hass.state is CoreState.running.

Configuration

automation:
  trigger:
    platform: homeassistant
    event: start
  action:
    service: light.turn_on

automation 2:
  trigger:
    platform: homeassistant
    event: shutdown
  action:
    service: light.turn_on

# Will work but raises a warning.
automation deprecated:
  trigger:
    platform: event
    event: homeassistant_start
  action:
    service: light.turn_on

Checklist:

If the code does not interact with devices:

Local tests with tox run successfully. Your PR cannot be merged unless tests pass
Tests have been added to verify that the new code works.

mention-bot · 2017-04-04T16:13:32Z

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

pvizeli · 2017-04-04T21:48:50Z

I think the seconds variant is not realy bad. With this special automation platform we allow to trigger on startup and shutdown. Other automation make it also in this way. Maybe we have other case for that platform in future.

like:

action:
  platform: homeassistant
  state: startup
...
action:
  platform: homeassistant
  state: shutdown

I like that.

balloob · 2017-04-05T03:30:55Z

Alright. Will support the old method but deprecate it. Then will also add the new platform.

houndci-bot · 2017-04-05T04:53:10Z

+    assert automation.is_on(hass, 'automation.hello')
+    assert len(calls) == 0
+
+    with patch.object(hass.loop, 'stop'), patch.object(hass.executor, 'shutdown'):


line too long (82 > 79 characters)

balloob · 2017-04-05T04:58:35Z

    This method is a coroutine.
    """
    start = time()
-    hass.async_track_tasks()


Track tasks is now enabled by default. It's what we want during startup.

balloob · 2017-04-05T05:03:58Z


        This method must be run in the event loop.
        """
-        if event_type != EVENT_HOMEASSISTANT_STOP and \


This was silently (because exception suppressed) killing shutdown tasks (noticed it when adding shutdown event in automation). It should be fine to just wait till all is done. Timer is already stopped so nothing new should come in.

balloob · 2017-04-05T06:27:00Z

        self.loop._thread_ident = threading.get_ident()
-        _async_create_timer(self)
        self.bus.async_fire(EVENT_HOMEASSISTANT_START)
+        yield from self.async_stop_track_tasks()


So this fixes the fact that CoreState.starting was never detectable before, as we would immediately overwrite it.

However, this has the downside that it is impossible for components/platforms to start a permanent async job as a result from EVENT_HOMEASSISTANT_START.

(note that things like MQTT server rely on loop.create_server which will work. Z-Wave depends on a thread and that will work)

Maybe add a timeout to async_stop_track_tasks after which we will no longer stop tracking tasks?

I think that should not be needed to add a timeout

pvizeli · 2017-04-05T09:39:34Z

        """Stop Home Assistant and shuts down all threads."""
-        run_coroutine_threadsafe(self.async_stop(), self.loop)
+        self.loop.call_soon_threadsafe(
+            self.loop.create_task, self.async_stop())


Why not only call loop.create_task?

Because that's not threadsafe

You are sure? I found no comments in doc that will be not threadsafe.

https://docs.python.org/3/library/asyncio-dev.html#concurrency-and-multithreading

To schedule a coroutine object from a different thread, the run_coroutine_threadsafe() function should be used.

Also, the tests won't pass if I just call create_task.

(note, we include the run_coroutine_threadsafe function from Python in HASS because it was only introduced in Python 3.5.1)

asyncio.ensure_future maby the same but don't support with python 3.4.2

homeassistant added cla-signed platform: automation.event labels Apr 4, 2017

balloob force-pushed the automations-on-startup branch from a628e72 to 4da2c63 Compare April 4, 2017 16:34

homeassistant added the cla-signed label Apr 4, 2017

balloobbot added the core label Apr 4, 2017

balloob added 7 commits April 4, 2017 20:34

Fire EVENT_HOMEASSISTANT_START automations off right away while starting

81eebe4

Actually have core state be set to 'starting' during boot

99a6b7a

Fix correct start implementation

3fe28bc

Test and deprecate event automation platform on start

94ef182

Fix doc strings

604ceb0

Remove shutting down exception

48e2750

More strict when to mark an instance as finished

deee087

balloob force-pushed the automations-on-startup branch from 4da2c63 to 3b46e44 Compare April 5, 2017 04:52

Add automation platform to listen for start/shutdown

3b46e44

homeassistant added the cla-signed label Apr 5, 2017

balloob added the breaking-change label Apr 5, 2017

houndci-bot reviewed Apr 5, 2017

View reviewed changes

balloob commented Apr 5, 2017

View reviewed changes

balloob added 2 commits April 4, 2017 23:17

When we stop we should wait till it's all done

22ec6df

Fix testing

bbe33a3

homeassistant added the cla-signed label Apr 5, 2017

balloob commented Apr 5, 2017

View reviewed changes

balloob added 4 commits April 5, 2017 00:24

Fix async bugs in tests

aeda4fe

Only set UVLOOP when hass starts from CLI

650dfa6

This hangs normal asyncio event loop

d758871

Clean up Z-Wave node entity test

293d575

pvizeli reviewed Apr 5, 2017

View reviewed changes

pvizeli approved these changes Apr 5, 2017

View reviewed changes

balloob merged commit 29f385e into dev Apr 6, 2017

balloob deleted the automations-on-startup branch April 6, 2017 06:23

fabaff mentioned this pull request Apr 6, 2017

0.42 #6956

Merged

brianjking mentioned this pull request Apr 17, 2017

Do not use event: homeassistant_start (Deprecated in 0.45) brianjking/homeassistant-config#84

Closed

home-assistant locked and limited conversation to collaborators Jul 17, 2017

ghost added integration: event and removed platform: automation.event labels Mar 21, 2019

Uh oh!

Conversation

balloob commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Breaking changes

Configuration

Checklist:

Uh oh!

mention-bot commented Apr 4, 2017

Uh oh!

pvizeli commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

balloob commented Apr 5, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

balloob Apr 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

balloob commented Apr 4, 2017 •

edited

Loading

pvizeli commented Apr 4, 2017 •

edited

Loading

balloob Apr 5, 2017 •

edited

Loading

balloob Apr 5, 2017 •

edited

Loading