Simplify recorder connection

[balloob]

Description:

The Recorder has become a dependency for setting up certain platforms and components. Yet, it would still try to setup the connection async, causing weird race conditions and blocking threads.

This is all simplified by making sure we synchronously try to connect to the database before we set up any other component. That way we can also correctly report that the database has failed to setup.

Related issue (if applicable): fixes #6167 #6192

Example entry for configuration.yaml (if applicable):

recorder:

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]

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

[pvizeli]

Instead to use the global, can we switch to hass.data?

[balloob]

I didn't want to tackle that in this PR. All queries etc would require hass to be passed in. That is currently not the case.

[pvizeli]

I didn't look to code. I've ask me that while I see it on diffs. But you are right, make not a lot sense

[kellerza]

I do not like this. It is a lot of blocking IO on startup, my suggested approach:

• async friendly blocking asyncio.Event similar to db_ready
• remove recording_start threading Event

[balloob]

@kellerza but we're doing that blocking I/O already. Home Assistant sets up all components in sequence. Each component sets their platforms up in sequence.

Let me schedule timeline in current dev:

• Setup recorder, launch thread, no block
• Setup component light
  • Setup platform 1 -> async_restore_state -> query states -> block till connection is done
• Recorder thread establishes connection
• Setup platform 1 continues
• Bootstrap sets up other components / platforms

Timeline with my branch:

• Setup recorder, blocks till connected connected
• Setup component light
  • Setup platform 1 -> async_restore_state -> query states -> no block
• Bootstrap sets up other components / platforms
• HOMEASSISTANT_STARt -> launch Recorder thread

[balloob]

Bootstrap setting up components in sequence: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/bootstrap.py#L72-L77

My bad, just found out that the EntityComponent will actually set up platforms in parallel: https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/helpers/entity_component.py#L71-L76

Any platform from a component that has restore_state support will still block till the connection is made.

[balloob]

So Home Assistant will be "faster" (not sure how much) if the component that is setup after the recorder component is not using restore state. However, we are going to make sure all components have restore state so this odds will get smaller and smaller.

[balloob]

As added bonus, we will now correctly mark the recorder component as not initialized if setup fails.

[pvizeli]

• STALE - I need more coffee 😄

At the end, I think that is the cleaner solution for our problem as #6167. I hope a timeout will be exists on sqlalchemy and it block not for ever.

[pvizeli]

We need not block any thing after this PR 💯 We can use the magic of async. After this PR we can do i.e. input_boolean:

@asyncio.coroutine
def async_added_to_hass(self):
    """Called when entity about to be added to hass."""
    @asyncio.coroutine
    def async_do_restore():
        """Do restore after db Connection is ready."""
        state = yield from async_get_last_state(self.hass, self.entity_id)
        if not state:
            return
        self._state = state.state == STATE_ON
        self.hass.async_add_job(self.async_update_ha_state())

    self.hass.async_add_job(async_do_restore())

I think we shuld make async_added_to_hass as callback and use only async_add_job inside for less coros.

[kellerza]

@pvizeli we need to block in async_added_to_hass since it need to be update before we add it to hass (last step in add_devices). Else we will have the state flapping on "add" and potentially trigger automations

@balloob the async block of get_last state will only happen after the "time expensive" parts in other component's setup has been completed (just before add) and I believe there is still value in doing this in parallel

[pvizeli]

@kellerza you think to sync 😄 we can fix that i.e. with: if we have async_added_to_hass we don't update the state on entity_component and it will done in this function, or move automation as last off all init.

We going to a amazing flow that not lost any speed, the real Magic of async. I know that is a bit complex and not how a human will thinking, but at the end we have the same but realy fast.

EDIT: Anyway, we should pin recorder to first of all and automation to last on all.

[kellerza]

@pvizeli async_added_to_hass is completely optional, and add_devices should not depend on it to schedule update_ha_state. So have to think about sequence of calls in add_devices

@balloob If components are not set up async/in parellel, but blocks (only platforms async) this PR is probably not that bad in terms of speed penalty. Are we sure Pascal will not change components setup async soon? :-)

Still feels wrong to connect, select migrate, select run, (optionally: close run) & create run all before we start doing anything productive.

[pvizeli]

@kellerza you thinking to hard in old programming routines. So you can programming sync and make that "async" but have no real benefit our you think compete in new way and do it real async and at the end you have the same but with big more turbo.

I don't bind async_added_to_hass to schedule_update_ha_state. I think you missanderstand me and how it would work after that.

After we task the update like above, we can change entity_component to this:

if hasattr(entity, 'async_added_to_hass'): 
    self.hass.async_add_job(entity.async_added_to_hass())
else:
    yield from entity.async_update_ha_state() 

That is only a draft, don't hang me on that, it should show how it could be work.

@asyncio.coroutine
def async_added_to_hass(self):
    """Called when entity about to be added to hass."""
    @asyncio.coroutine
    def async_do_restore():
        """Do restore after db Connection is ready."""
        try:
            state = yield from async_get_last_state(self.hass, self.entity_id)
            if not state:
                return
            self._state = state.state == STATE_ON
        finally:
            self.hass.async_add_job(self.async_update_ha_state())

    self.hass.async_add_job(async_do_restore())

EDIT: Update function to task async_added_to_hass

We can also package the async_added_to_hass around to move try/finally for update into one point.

[kellerza]

Beginning to like @pvizeli's idea, but dont like the brancing in added to hass.

Should we not rather make async_addedtohasss part of Entity and in the base implementation already add the schedule_update_hass, so if we override addedtohass to hass we can simply call super after we are done, it would look more natural to call super than remembering to call some other function

[pvizeli]

It is only a example how that can work async. At the end I think we should not change the async_added_to_hass logic and change the hole logic inside EntityComponent to work full async.

At the end we need to update data from a real device and try to restore data from fake device/webservice. EntityComponent should be able to detect it and call the right function ad a task.

[pvizeli]

@balloob @kellerza I think this PR is the right way but we need EntityComponent to real async or we can have a executor deadlock with this PR.

[kellerza]

@pvizeli actually if EntityComponent is changed not to block filling the restore cache (something like your code snippets) then this PR, simplifying the recorder&blocking all other components is not the way to go. Made a suggestion in #6167