From 9ea1101aba438035879ecaa8b4b8f83ec5d67841 Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Sat, 29 Oct 2016 12:54:47 -0700 Subject: [PATCH] Fix bootstrap circular imports (#4108) * Fix bootstrap circular imports * fix test * Lint --- homeassistant/bootstrap.py | 94 +++++++++++++++----------- homeassistant/helpers/discovery.py | 59 ++++++++++++---- tests/common.py | 10 +++ tests/helpers/test_discovery.py | 33 +++++++-- tests/helpers/test_entity_component.py | 6 +- 5 files changed, 140 insertions(+), 62 deletions(-) diff --git a/homeassistant/bootstrap.py b/homeassistant/bootstrap.py index 6a0d6540688..179c819f611 100644 --- a/homeassistant/bootstrap.py +++ b/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: - _LOGGER.error('Attempt made to setup %s during setup of %s', - domain, domain) - return False - - 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) + did_lock = False + if not hass.setup_lock.locked(): + yield from hass.setup_lock.acquire() + did_lock = True 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 domain in _CURRENT_SETUP: + _LOGGER.error('Attempt made to setup %s during setup of %s', + domain, domain) + return False - if result is False: - _LOGGER.error('component %s failed to initialize', domain) + config = yield from async_prepare_setup_component(hass, config, domain) + + if config is None: 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) + + component = loader.get_component(domain) + _CURRENT_SETUP.append(domain) + + 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 - _LOGGER.exception('Error during setup of component %s', domain) - return False + finally: + _CURRENT_SETUP.remove(domain) + + 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) - - 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 + if did_lock: + hass.setup_lock.release() def prepare_setup_component(hass: core.HomeAssistant, config: dict, diff --git a/homeassistant/helpers/discovery.py b/homeassistant/helpers/discovery.py index eb36fc9e1d5..de432804e9c 100644 --- a/homeassistant/helpers/discovery.py +++ b/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(): - """Discover platform job.""" + fire_coroutine_threadsafe( + 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 + '.' + <> + ATTR_PLATFORM = <> + ATTR_DISCOVERED = <> } + + 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): - return + res = yield from bootstrap.async_setup_component( + hass, component, hass_config) + finally: + if did_lock: + hass.setup_lock.release() - data = { - ATTR_SERVICE: EVENT_LOAD_PLATFORM.format(component), - ATTR_PLATFORM: platform, - } + if not res: + return - if discovered is not None: - data[ATTR_DISCOVERED] = discovered + data = { + 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) diff --git a/tests/common.py b/tests/common.py index 4f2f447c1b0..9ee4dda5bfe 100644 --- a/tests/common.py +++ b/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. diff --git a/tests/helpers/test_discovery.py b/tests/helpers/test_discovery.py index a0868691e3f..6851874ca37 100644 --- a/tests/helpers/test_discovery.py +++ b/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 diff --git a/tests/helpers/test_entity_component.py b/tests/helpers/test_entity_component.py index 6f658a70518..bc94c9f44dc 100644 --- a/tests/helpers/test_entity_component.py +++ b/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)