From 7cb8a8bbc9c1ce438a907d9f29ec80bcb84dea29 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Tue, 5 Mar 2024 04:59:52 -1000 Subject: [PATCH] Migrate remaining calls in config modules to async_get_component (#112293) * Migrate remaining calls in config modules to async_get_component There were a few cases that were still using get_component that could have done blocking I/O in the event loop, although it was unlikely. The caching check in async_get_component has been moved up to avoid creating the future if the module is already in the cache * fix one more --- homeassistant/config.py | 2 +- homeassistant/config_entries.py | 12 ++++++------ homeassistant/loader.py | 27 +++++++++++++++++++-------- tests/test_loader.py | 7 +++---- tests/test_setup.py | 2 +- 5 files changed, 30 insertions(+), 20 deletions(-) diff --git a/homeassistant/config.py b/homeassistant/config.py index 8962ff8e7a5..bd6d14f8c10 100644 --- a/homeassistant/config.py +++ b/homeassistant/config.py @@ -1083,7 +1083,7 @@ async def merge_packages_config( integration = await async_get_integration_with_requirements( hass, domain ) - component = integration.get_component() + component = await integration.async_get_component() except LOAD_EXCEPTIONS as exc: _log_pkg_error( hass, diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index 0dae0359d8a..e6fe5d5d70b 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -490,7 +490,7 @@ class ConfigEntry: hass, self.domain ) try: - component = integration.get_component() + component = await integration.async_get_component() except ImportError as err: _LOGGER.error( "Error importing integration %s to set up %s configuration entry: %s", @@ -677,7 +677,7 @@ class ConfigEntry: self._async_set_state(hass, ConfigEntryState.NOT_LOADED, None) return True - component = integration.get_component() + component = await integration.async_get_component() if integration.domain == self.domain: if not self.state.recoverable: @@ -734,7 +734,7 @@ class ConfigEntry: # entry. return - component = integration.get_component() + component = await integration.async_get_component() if not hasattr(component, "async_remove_entry"): return try: @@ -783,7 +783,7 @@ class ConfigEntry: if not (integration := self._integration_for_domain): integration = await loader.async_get_integration(hass, self.domain) - component = integration.get_component() + component = await integration.async_get_component() supports_migrate = hasattr(component, "async_migrate_entry") if not supports_migrate: if same_major_version: @@ -2497,14 +2497,14 @@ def _handle_entry_updated_filter(event: Event) -> bool: async def support_entry_unload(hass: HomeAssistant, domain: str) -> bool: """Test if a domain supports entry unloading.""" integration = await loader.async_get_integration(hass, domain) - component = integration.get_component() + component = await integration.async_get_component() return hasattr(component, "async_unload_entry") async def support_remove_from_device(hass: HomeAssistant, domain: str) -> bool: """Test if a domain supports being removed from a device.""" integration = await loader.async_get_integration(hass, domain) - component = integration.get_component() + component = await integration.async_get_component() return hasattr(component, "async_remove_config_entry_device") diff --git a/homeassistant/loader.py b/homeassistant/loader.py index a7798f7b5e2..95b00547fb8 100644 --- a/homeassistant/loader.py +++ b/homeassistant/loader.py @@ -901,6 +901,10 @@ class Integration: and will check if import_executor is set and load it in the executor, otherwise it will load it in the event loop. """ + domain = self.domain + if domain in (cache := self._cache): + return cache[domain] + if self._component_future: return await self._component_future @@ -914,7 +918,7 @@ class Integration: or (self.config_flow and f"{self.pkg_path}.config_flow" not in sys.modules) ) if not load_executor: - comp = self.get_component() + comp = self._get_component() if debug: _LOGGER.debug( "Component %s import took %.3f seconds (loaded_executor=False)", @@ -927,7 +931,7 @@ class Integration: try: try: comp = await self.hass.async_add_import_executor_job( - self.get_component, True + self._get_component, True ) except ImportError as ex: load_executor = False @@ -936,7 +940,7 @@ class Integration: ) # If importing in the executor deadlocks because there is a circular # dependency, we fall back to the event loop. - comp = self.get_component() + comp = self._get_component() self._component_future.set_result(comp) except BaseException as ex: self._component_future.set_exception(ex) @@ -959,22 +963,29 @@ class Integration: return comp - def get_component(self, preload_platforms: bool = False) -> ComponentProtocol: + def get_component(self) -> ComponentProtocol: """Return the component. This method must be thread-safe as it's called from the executor and the event loop. + This method checks the cache and if the component is not loaded + it will load it in the executor if import_executor is set, otherwise + it will load it in the event loop. + This is mostly a thin wrapper around importlib.import_module with a dict cache which is thread-safe since importlib has appropriate locks. """ + domain = self.domain + if domain in (cache := self._cache): + return cache[domain] + return self._get_component() + + def _get_component(self, preload_platforms: bool = False) -> ComponentProtocol: + """Return the component.""" cache = self._cache domain = self.domain - - if domain in cache: - return cache[domain] - try: cache[domain] = cast( ComponentProtocol, importlib.import_module(self.pkg_path) diff --git a/tests/test_loader.py b/tests/test_loader.py index 74eb4c6ed69..8be7cf09b2c 100644 --- a/tests/test_loader.py +++ b/tests/test_loader.py @@ -1188,10 +1188,9 @@ async def test_async_get_component_loads_loop_if_already_in_sys_modules( ), patch("homeassistant.loader.importlib.import_module", import_module): module = await integration.async_get_component() - # Everything is there so we should load in the event loop - # since it will all be cached - assert "loaded_executor=False" in caplog.text - assert "loaded_executor=True" not in caplog.text + # Everything is already in the integration cache + # so it should not have to call the load + assert "loaded_executor" not in caplog.text assert module is module_mock diff --git a/tests/test_setup.py b/tests/test_setup.py index 29ced934745..c54c02fd880 100644 --- a/tests/test_setup.py +++ b/tests/test_setup.py @@ -617,7 +617,7 @@ async def test_async_when_setup_or_start_already_loaded(hass: HomeAssistant) -> async def test_setup_import_blows_up(hass: HomeAssistant) -> None: """Test that we handle it correctly when importing integration blows up.""" with patch( - "homeassistant.loader.Integration.get_component", side_effect=ImportError + "homeassistant.loader.Integration.async_get_component", side_effect=ImportError ): assert not await setup.async_setup_component(hass, "sun", {})