Fix bootstrap circular imports (#4108)

* Fix bootstrap circular imports

* fix test

* Lint

homeassistant/bootstrap.py

@@ -102,56 +102,68 @@ def _async_setup_component(hass: core.HomeAssistant,
     """
     # pylint: disable=too-many-return-statements,too-many-branches
     # pylint: disable=too-many-statements
+    if not hasattr(hass, 'setup_lock'):
+        hass.setup_lock = asyncio.Lock(loop=hass.loop)
+
     if domain in hass.config.components:
         return True
 
-    if domain in _CURRENT_SETUP:
+    did_lock = False
-        _LOGGER.error('Attempt made to setup %s during setup of %s',
+    if not hass.setup_lock.locked():
-                      domain, domain)
+        yield from hass.setup_lock.acquire()
-        return False
+        did_lock = True
-
-    config = yield from async_prepare_setup_component(hass, config, domain)
-
-    if config is None:
-        return False
-
-    component = loader.get_component(domain)
-    _CURRENT_SETUP.append(domain)
 
     try:
-        if hasattr(component, 'async_setup'):
+        if domain in _CURRENT_SETUP:
-            result = yield from component.async_setup(hass, config)
+            _LOGGER.error('Attempt made to setup %s during setup of %s',
-        else:
+                          domain, domain)
-            result = yield from hass.loop.run_in_executor(
+            return False
-                None, component.setup, hass, config)
 
-        if result is False:
+        config = yield from async_prepare_setup_component(hass, config, domain)
-            _LOGGER.error('component %s failed to initialize', domain)
+
+        if config is None:
             return False
-        elif result is not True:
+
-            _LOGGER.error('component %s did not return boolean if setup '
+        component = loader.get_component(domain)
-                          'was successful. Disabling component.', domain)
+        _CURRENT_SETUP.append(domain)
-            loader.set_component(domain, None)
+
+        try:
+            if hasattr(component, 'async_setup'):
+                result = yield from component.async_setup(hass, config)
+            else:
+                result = yield from hass.loop.run_in_executor(
+                    None, component.setup, hass, config)
+
+            if result is False:
+                _LOGGER.error('component %s failed to initialize', domain)
+                return False
+            elif result is not True:
+                _LOGGER.error('component %s did not return boolean if setup '
+                              'was successful. Disabling component.', domain)
+                loader.set_component(domain, None)
+                return False
+        except Exception:  # pylint: disable=broad-except
+            _LOGGER.exception('Error during setup of component %s', domain)
             return False
-    except Exception:  # pylint: disable=broad-except
+        finally:
-        _LOGGER.exception('Error during setup of component %s', domain)
+            _CURRENT_SETUP.remove(domain)
-        return False
+
+        hass.config.components.append(component.DOMAIN)
+
+        # Assumption: if a component does not depend on groups
+        # it communicates with devices
+        if 'group' not in getattr(component, 'DEPENDENCIES', []) and \
+                hass.pool.worker_count <= 10:
+            hass.pool.add_worker()
+
+        hass.bus.async_fire(
+            EVENT_COMPONENT_LOADED, {ATTR_COMPONENT: component.DOMAIN}
+        )
+
+        return True
     finally:
-        _CURRENT_SETUP.remove(domain)
+        if did_lock:
-
+            hass.setup_lock.release()
-    hass.config.components.append(component.DOMAIN)
-
-    # Assumption: if a component does not depend on groups
-    # it communicates with devices
-    if 'group' not in getattr(component, 'DEPENDENCIES', []) and \
-       hass.pool.worker_count <= 10:
-        hass.pool.add_worker()
-
-    hass.bus.async_fire(
-        EVENT_COMPONENT_LOADED, {ATTR_COMPONENT: component.DOMAIN}
-    )
-
-    return True
 
 
 def prepare_setup_component(hass: core.HomeAssistant, config: dict,

homeassistant/helpers/discovery.py

@@ -1,9 +1,11 @@
 """Helper methods to help with platform discovery."""
+import asyncio
 
 from homeassistant import bootstrap, core
 from homeassistant.const import (
     ATTR_DISCOVERED, ATTR_SERVICE, EVENT_PLATFORM_DISCOVERED)
-from homeassistant.util.async import run_callback_threadsafe
+from homeassistant.util.async import (
+    run_callback_threadsafe, fire_coroutine_threadsafe)
 
 EVENT_LOAD_PLATFORM = 'load_platform.{}'
 ATTR_PLATFORM = 'platform'
@@ -87,20 +89,51 @@ def load_platform(hass, component, platform, discovered=None,
 
     Use `listen_platform` to register a callback for these events.
     """
-    def discover_platform():
+    fire_coroutine_threadsafe(
-        """Discover platform job."""
+        async_load_platform(hass, component, platform,
+                            discovered, hass_config), hass.loop)
+
+
+@asyncio.coroutine
+def async_load_platform(hass, component, platform, discovered=None,
+                        hass_config=None):
+    """Load a component and platform dynamically.
+
+    Target components will be loaded and an EVENT_PLATFORM_DISCOVERED will be
+    fired to load the platform. The event will contain:
+        { ATTR_SERVICE = LOAD_PLATFORM + '.' + <<component>>
+          ATTR_PLATFORM = <<platform>>
+          ATTR_DISCOVERED = <<discovery info>> }
+
+    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.
+
+    This method is a coroutine.
+    """
+    did_lock = False
+    if hasattr(hass, 'setup_lock') and hass.setup_lock.locked():
+        did_lock = True
+        yield from hass.setup_lock.acquire()
+
+    try:
         # No need to fire event if we could not setup component
-        if not bootstrap.setup_component(hass, component, hass_config):
+        res = yield from bootstrap.async_setup_component(
-            return
+            hass, component, hass_config)
+    finally:
+        if did_lock:
+            hass.setup_lock.release()
 
-        data = {
+    if not res:
-            ATTR_SERVICE: EVENT_LOAD_PLATFORM.format(component),
+        return
-            ATTR_PLATFORM: platform,
-        }
 
-        if discovered is not None:
+    data = {
-            data[ATTR_DISCOVERED] = discovered
+        ATTR_SERVICE: EVENT_LOAD_PLATFORM.format(component),
+        ATTR_PLATFORM: platform,
+    }
 
-        hass.bus.fire(EVENT_PLATFORM_DISCOVERED, data)
+    if discovered is not None:
+        data[ATTR_DISCOVERED] = discovered
 
-    hass.add_job(discover_platform)
+    hass.bus.async_fire(EVENT_PLATFORM_DISCOVERED, data)

tests/common.py

@@ -348,6 +348,16 @@ def patch_yaml_files(files_dict, endswith=True):
     return patch.object(yaml, 'open', mock_open_f, create=True)
 
 
+def mock_coro(return_value=None):
+    """Helper method to return a coro that returns a value."""
+    @asyncio.coroutine
+    def coro():
+        """Fake coroutine."""
+        return return_value
+
+    return coro
+
+
 @contextmanager
 def assert_setup_component(count, domain=None):
     """Collect valid configuration from setup_component.

tests/helpers/test_discovery.py

@@ -3,8 +3,10 @@ from unittest.mock import patch
 
 from homeassistant import loader, bootstrap
 from homeassistant.helpers import discovery
+from homeassistant.util.async import run_coroutine_threadsafe
 
-from tests.common import get_test_home_assistant, MockModule, MockPlatform
+from tests.common import (
+    get_test_home_assistant, MockModule, MockPlatform, mock_coro)
 
 
 class TestHelpersDiscovery:
@@ -12,7 +14,7 @@ class TestHelpersDiscovery:
 
     def setup_method(self, method):
         """Setup things to be run when tests are started."""
-        self.hass = get_test_home_assistant(1)
+        self.hass = get_test_home_assistant()
 
     def teardown_method(self, method):
         """Stop everything that was started."""
@@ -55,7 +57,8 @@ class TestHelpersDiscovery:
         assert ['test service', 'another service'] == [info[0] for info
                                                        in calls_multi]
 
-    @patch('homeassistant.bootstrap.setup_component')
+    @patch('homeassistant.bootstrap.async_setup_component',
+           return_value=mock_coro(True)())
     def test_platform(self, mock_setup_component):
         """Test discover platform method."""
         calls = []
@@ -91,7 +94,17 @@ class TestHelpersDiscovery:
         assert len(calls) == 1
 
     def test_circular_import(self):
-        """Test we don't break doing circular import."""
+        """Test we don't break doing circular import.
+
+        This test will have test_component discover the switch.test_circular
+        component while setting up.
+
+        The supplied config will load test_component and will load
+        switch.test_circular.
+
+        That means that after startup, we will have test_component and switch
+        setup. The test_circular platform has been loaded twice.
+        """
         component_calls = []
         platform_calls = []
 
@@ -122,9 +135,17 @@ class TestHelpersDiscovery:
                 'platform': 'test_circular',
             }],
         })
+
+        # We wait for the setup_lock to finish
+        run_coroutine_threadsafe(
+            self.hass.setup_lock.acquire(), self.hass.loop).result()
+
         self.hass.block_till_done()
 
+        # test_component will only be setup once
+        assert len(component_calls) == 1
+        # The platform will be setup once via the config in `setup_component`
+        # and once via the discovery inside test_component.
+        assert len(platform_calls) == 2
         assert 'test_component' in self.hass.config.components
         assert 'switch' in self.hass.config.components
-        assert len(component_calls) == 1
-        assert len(platform_calls) == 1

tests/helpers/test_entity_component.py

@@ -13,7 +13,8 @@ from homeassistant.helpers import discovery
 import homeassistant.util.dt as dt_util
 
 from tests.common import (
-    get_test_home_assistant, MockPlatform, MockModule, fire_time_changed)
+    get_test_home_assistant, MockPlatform, MockModule, fire_time_changed,
+    mock_coro)
 
 _LOGGER = logging.getLogger(__name__)
 DOMAIN = "test_domain"
@@ -226,7 +227,8 @@ class TestHelpersEntityComponent(unittest.TestCase):
 
     @patch('homeassistant.helpers.entity_component.EntityComponent'
            '._async_setup_platform')
-    @patch('homeassistant.bootstrap.setup_component', return_value=True)
+    @patch('homeassistant.bootstrap.async_setup_component',
+           return_value=mock_coro(True)())
     def test_setup_does_discovery(self, mock_setup_component, mock_setup):
         """Test setup for discovery."""
         component = EntityComponent(_LOGGER, DOMAIN, self.hass)