Better locking while setting up components + discovery (#4463)

homeassistant/bootstrap.py

@@ -4,7 +4,7 @@ import logging
 import logging.handlers
 import os
 import sys
-from collections import defaultdict
+from collections import OrderedDict
 
 from types import ModuleType
 from typing import Any, Optional, Dict
@@ -57,7 +57,7 @@ def async_setup_component(hass: core.HomeAssistant, domain: str,
         yield from hass.loop.run_in_executor(None, loader.prepare, hass)
 
     if config is None:
-        config = defaultdict(dict)
+        config = {}
 
     components = loader.load_order_component(domain)
 
@@ -345,7 +345,7 @@ def from_config_dict(config: Dict[str, Any],
 
     # run task
     future = asyncio.Future(loop=hass.loop)
-    hass.loop.create_task(_async_init_from_config_dict(future))
+    hass.async_add_job(_async_init_from_config_dict(future))
     hass.loop.run_until_complete(future)
 
     return future.result()
@@ -365,6 +365,12 @@ def async_from_config_dict(config: Dict[str, Any],
     Dynamically loads required components and its dependencies.
     This method is a coroutine.
     """
+    setup_lock = hass.data.get('setup_lock')
+    if setup_lock is None:
+        setup_lock = hass.data['setup_lock'] = asyncio.Lock(loop=hass.loop)
+
+    yield from setup_lock.acquire()
+
     core_config = config.get(core.DOMAIN, {})
 
     try:
@@ -388,10 +394,12 @@ def async_from_config_dict(config: Dict[str, Any],
         yield from hass.loop.run_in_executor(None, loader.prepare, hass)
 
     # Make a copy because we are mutating it.
-    # Convert it to defaultdict so components can always have config dict
+    # Use OrderedDict in case original one was one.
     # Convert values to dictionaries if they are None
-    config = defaultdict(
+    new_config = OrderedDict()
-        dict, {key: value or {} for key, value in config.items()})
+    for key, value in config.items():
+        new_config[key] = value or {}
+    config = new_config
 
     # Filter out the repeating and common config section [homeassistant]
     components = set(key.split(' ')[0] for key in config.keys()
@@ -417,6 +425,8 @@ def async_from_config_dict(config: Dict[str, Any],
     for domain in loader.load_order_components(components):
         yield from _async_setup_component(hass, domain, config)
 
+    setup_lock.release()
+
     return hass
 
 

homeassistant/helpers/discovery.py

@@ -1,11 +1,16 @@
-"""Helper methods to help with platform discovery."""
+"""Helper methods to help with platform discovery.
+
+There are two different types of discoveries that can be fired/listened for.
+ - listen/discover is for services. These are targetted at a component.
+ - listen_platform/discover_platform is for platforms. These are used by
+   components to allow discovery of their platofrms.
+"""
 import asyncio
 
 from homeassistant import bootstrap, core
 from homeassistant.const import (
     ATTR_DISCOVERED, ATTR_SERVICE, EVENT_PLATFORM_DISCOVERED)
-from homeassistant.util.async import (
+from homeassistant.util.async import run_callback_threadsafe
-    run_callback_threadsafe, fire_coroutine_threadsafe)
 
 EVENT_LOAD_PLATFORM = 'load_platform.{}'
 ATTR_PLATFORM = 'platform'
@@ -43,8 +48,29 @@ def async_listen(hass, service, callback):
 
 def discover(hass, service, discovered=None, component=None, hass_config=None):
     """Fire discovery event. Can ensure a component is loaded."""
-    if component is not None:
+    hass.async_add_job(
-        bootstrap.setup_component(hass, component, hass_config)
+        async_discover(hass, service, discovered, component, hass_config))
+
+
+@asyncio.coroutine
+def async_discover(hass, service, discovered=None, component=None,
+                   hass_config=None):
+    """Fire discovery event. Can ensure a component is loaded."""
+    if component is not None and component not in hass.config.components:
+        did_lock = False
+        setup_lock = hass.data.get('setup_lock')
+        if setup_lock and setup_lock.locked():
+            did_lock = True
+            yield from setup_lock.acquire()
+
+        try:
+            # Could have been loaded while waiting for lock.
+            if component not in hass.config.components:
+                yield from bootstrap.async_setup_component(hass, component,
+                                                           hass_config)
+        finally:
+            if did_lock:
+                setup_lock.release()
 
     data = {
         ATTR_SERVICE: service
@@ -53,7 +79,7 @@ def discover(hass, service, discovered=None, component=None, hass_config=None):
     if discovered is not None:
         data[ATTR_DISCOVERED] = discovered
 
-    hass.bus.fire(EVENT_PLATFORM_DISCOVERED, data)
+    hass.bus.async_fire(EVENT_PLATFORM_DISCOVERED, data)
 
 
 def listen_platform(hass, component, callback):
@@ -101,9 +127,9 @@ def load_platform(hass, component, platform, discovered=None,
 
     Use `listen_platform` to register a callback for these events.
     """
-    fire_coroutine_threadsafe(
+    hass.async_add_job(
-        async_load_platform(hass, component, platform,
+        async_load_platform(hass, component, platform, discovered,
-                            discovered, hass_config), hass.loop)
+                            hass_config))
 
 
 @asyncio.coroutine
@@ -120,25 +146,30 @@ def async_load_platform(hass, component, platform, discovered=None,
     Use `listen_platform` to register a callback for these events.
 
     Warning: Do not yield from this inside a setup method to avoid a dead lock.
-    Use `hass.loop.create_task(async_load_platform(..))` instead.
+    Use `hass.loop.async_add_job(async_load_platform(..))` instead.
 
     This method is a coroutine.
     """
     did_lock = False
+    if component not in hass.config.components:
         setup_lock = hass.data.get('setup_lock')
         if setup_lock and setup_lock.locked():
             did_lock = True
             yield from setup_lock.acquire()
 
+    setup_success = True
+
     try:
-        # No need to fire event if we could not setup component
+        # Could have been loaded while waiting for lock.
-        res = yield from bootstrap.async_setup_component(
+        if component not in hass.config.components:
+            setup_success = yield from bootstrap.async_setup_component(
                 hass, component, hass_config)
     finally:
         if did_lock:
             setup_lock.release()
 
-    if not res:
+    # No need to fire event if we could not setup component
+    if not setup_success:
         return
 
     data = {

tests/helpers/test_discovery.py

@@ -1,7 +1,9 @@
 """Test discovery helpers."""
+from collections import OrderedDict
 from unittest.mock import patch
 
 from homeassistant import loader, bootstrap
+from homeassistant.core import callback
 from homeassistant.helpers import discovery
 from homeassistant.util.async import run_coroutine_threadsafe
 
@@ -20,16 +22,18 @@ class TestHelpersDiscovery:
         """Stop everything that was started."""
         self.hass.stop()
 
-    @patch('homeassistant.bootstrap.setup_component')
+    @patch('homeassistant.bootstrap.async_setup_component')
     def test_listen(self, mock_setup_component):
         """Test discovery listen/discover combo."""
         calls_single = []
         calls_multi = []
 
+        @callback
         def callback_single(service, info):
             """Service discovered callback."""
             calls_single.append((service, info))
 
+        @callback
         def callback_multi(service, info):
             """Service discovered callback."""
             calls_multi.append((service, info))
@@ -42,16 +46,16 @@ class TestHelpersDiscovery:
                            'test_component')
         self.hass.block_till_done()
 
-        discovery.discover(self.hass, 'another service', 'discovery info',
-                           'test_component')
-        self.hass.block_till_done()
-
         assert mock_setup_component.called
         assert mock_setup_component.call_args[0] == \
             (self.hass, 'test_component', None)
         assert len(calls_single) == 1
         assert calls_single[0] == ('test service', 'discovery info')
 
+        discovery.discover(self.hass, 'another service', 'discovery info',
+                           'test_component')
+        self.hass.block_till_done()
+
         assert len(calls_single) == 1
         assert len(calls_multi) == 2
         assert ['test service', 'another service'] == [info[0] for info
@@ -63,6 +67,7 @@ class TestHelpersDiscovery:
         """Test discover platform method."""
         calls = []
 
+        @callback
         def platform_callback(platform, info):
             """Platform callback method."""
             calls.append((platform, info))
@@ -149,3 +154,44 @@ class TestHelpersDiscovery:
         assert len(platform_calls) == 2
         assert 'test_component' in self.hass.config.components
         assert 'switch' in self.hass.config.components
+
+    def test_1st_discovers_2nd_component(self):
+        """Test that we don't break if one component discovers the other.
+
+        If the first component fires a discovery event to setup the
+        second component while the second component is about to be setup,
+        it should not setup the second component twice.
+        """
+        component_calls = []
+
+        def component1_setup(hass, config):
+            """Setup mock component."""
+            discovery.discover(hass, 'test_component2')
+            return True
+
+        def component2_setup(hass, config):
+            """Setup mock component."""
+            component_calls.append(1)
+            return True
+
+        loader.set_component(
+            'test_component1',
+            MockModule('test_component1', setup=component1_setup))
+
+        loader.set_component(
+            'test_component2',
+            MockModule('test_component2', setup=component2_setup))
+
+        config = OrderedDict()
+        config['test_component1'] = {}
+        config['test_component2'] = {}
+
+        self.hass.loop.run_until_complete = \
+            lambda _: self.hass.block_till_done()
+
+        bootstrap.from_config_dict(config, self.hass)
+
+        self.hass.block_till_done()
+
+        # test_component will only be setup once
+        assert len(component_calls) == 1