From b12c69accbe42653e4e00d41e1c026c953b82a93 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Mon, 1 Apr 2024 15:36:48 -1000 Subject: [PATCH] Fix memory leak when importing a platform fails (#114602) * Fix memory leak when importing a platform fails re-raising ImportError would trigger a memory leak * fixes, coverage * Apply suggestions from code review --- homeassistant/loader.py | 31 ++++++------ tests/test_loader.py | 107 +++++++++++++++++++++++++++++++--------- 2 files changed, 98 insertions(+), 40 deletions(-) diff --git a/homeassistant/loader.py b/homeassistant/loader.py index 722f3fd83c7..eb70f0b83af 100644 --- a/homeassistant/loader.py +++ b/homeassistant/loader.py @@ -750,9 +750,7 @@ class Integration: self._import_futures: dict[str, asyncio.Future[ModuleType]] = {} cache: dict[str, ModuleType | ComponentProtocol] = hass.data[DATA_COMPONENTS] self._cache = cache - missing_platforms_cache: dict[str, ImportError] = hass.data[ - DATA_MISSING_PLATFORMS - ] + missing_platforms_cache: dict[str, bool] = hass.data[DATA_MISSING_PLATFORMS] self._missing_platforms_cache = missing_platforms_cache self._top_level_files = top_level_files or set() _LOGGER.info("Loaded %s from %s", self.domain, pkg_path) @@ -1085,8 +1083,7 @@ class Integration: import_futures: list[tuple[str, asyncio.Future[ModuleType]]] = [] for platform_name in platform_names: - full_name = f"{domain}.{platform_name}" - if platform := self._get_platform_cached_or_raise(full_name): + if platform := self._get_platform_cached_or_raise(platform_name): platforms[platform_name] = platform continue @@ -1095,6 +1092,7 @@ class Integration: in_progress_imports[platform_name] = future continue + full_name = f"{domain}.{platform_name}" if ( self.import_executor and full_name not in self.hass.config.components @@ -1166,14 +1164,18 @@ class Integration: return platforms - def _get_platform_cached_or_raise(self, full_name: str) -> ModuleType | None: + def _get_platform_cached_or_raise(self, platform_name: str) -> ModuleType | None: """Return a platform for an integration from cache.""" + full_name = f"{self.domain}.{platform_name}" if full_name in self._cache: # the cache is either a ModuleType or a ComponentProtocol # but we only care about the ModuleType here return self._cache[full_name] # type: ignore[return-value] if full_name in self._missing_platforms_cache: - raise self._missing_platforms_cache[full_name] + raise ModuleNotFoundError( + f"Platform {full_name} not found", + name=f"{self.pkg_path}.{platform_name}", + ) return None def platforms_are_loaded(self, platform_names: Iterable[str]) -> bool: @@ -1189,9 +1191,7 @@ class Integration: def get_platform(self, platform_name: str) -> ModuleType: """Return a platform for an integration.""" - if platform := self._get_platform_cached_or_raise( - f"{self.domain}.{platform_name}" - ): + if platform := self._get_platform_cached_or_raise(platform_name): return platform return self._load_platform(platform_name) @@ -1212,10 +1212,7 @@ class Integration: ): existing_platforms.append(platform_name) continue - missing_platforms[full_name] = ModuleNotFoundError( - f"Platform {full_name} not found", - name=f"{self.pkg_path}.{platform_name}", - ) + missing_platforms[full_name] = True return existing_platforms @@ -1233,11 +1230,13 @@ class Integration: cache: dict[str, ModuleType] = self.hass.data[DATA_COMPONENTS] try: cache[full_name] = self._import_platform(platform_name) - except ImportError as ex: + except ModuleNotFoundError: if self.domain in cache: # If the domain is loaded, cache that the platform # does not exist so we do not try to load it again - self._missing_platforms_cache[full_name] = ex + self._missing_platforms_cache[full_name] = True + raise + except ImportError: raise except RuntimeError as err: # _DeadlockError inherits from RuntimeError diff --git a/tests/test_loader.py b/tests/test_loader.py index e73029e14e2..41796f2f7d2 100644 --- a/tests/test_loader.py +++ b/tests/test_loader.py @@ -274,7 +274,61 @@ async def test_get_integration_exceptions(hass: HomeAssistant) -> None: async def test_get_platform_caches_failures_when_component_loaded( hass: HomeAssistant, ) -> None: - """Test get_platform cache failures only when the component is loaded.""" + """Test get_platform caches failures only when the component is loaded. + + Only ModuleNotFoundError is cached, ImportError is not cached. + """ + integration = await loader.async_get_integration(hass, "hue") + + with ( + pytest.raises(ModuleNotFoundError), + patch( + "homeassistant.loader.importlib.import_module", + side_effect=ModuleNotFoundError("Boom"), + ), + ): + assert integration.get_component() == hue + + with ( + pytest.raises(ModuleNotFoundError), + patch( + "homeassistant.loader.importlib.import_module", + side_effect=ModuleNotFoundError("Boom"), + ), + ): + assert integration.get_platform("light") == hue_light + + # Hue is not loaded so we should still hit the import_module path + with ( + pytest.raises(ModuleNotFoundError), + patch( + "homeassistant.loader.importlib.import_module", + side_effect=ModuleNotFoundError("Boom"), + ), + ): + assert integration.get_platform("light") == hue_light + + assert integration.get_component() == hue + + # Hue is loaded so we should cache the import_module failure now + with ( + pytest.raises(ModuleNotFoundError), + patch( + "homeassistant.loader.importlib.import_module", + side_effect=ModuleNotFoundError("Boom"), + ), + ): + assert integration.get_platform("light") == hue_light + + # Hue is loaded and the last call should have cached the import_module failure + with pytest.raises(ModuleNotFoundError): + assert integration.get_platform("light") == hue_light + + +async def test_get_platform_only_cached_module_not_found_when_component_loaded( + hass: HomeAssistant, +) -> None: + """Test get_platform cache only cache module not found when the component is loaded.""" integration = await loader.async_get_integration(hass, "hue") with ( @@ -317,41 +371,43 @@ async def test_get_platform_caches_failures_when_component_loaded( ): assert integration.get_platform("light") == hue_light - # Hue is loaded and the last call should have cached the import_module failure - with pytest.raises(ImportError): - assert integration.get_platform("light") == hue_light + # ImportError is not cached because we only cache ModuleNotFoundError + assert integration.get_platform("light") == hue_light async def test_async_get_platform_caches_failures_when_component_loaded( hass: HomeAssistant, ) -> None: - """Test async_get_platform cache failures only when the component is loaded.""" + """Test async_get_platform caches failures only when the component is loaded. + + Only ModuleNotFoundError is cached, ImportError is not cached. + """ integration = await loader.async_get_integration(hass, "hue") with ( - pytest.raises(ImportError), + pytest.raises(ModuleNotFoundError), patch( "homeassistant.loader.importlib.import_module", - side_effect=ImportError("Boom"), + side_effect=ModuleNotFoundError("Boom"), ), ): assert integration.get_component() == hue with ( - pytest.raises(ImportError), + pytest.raises(ModuleNotFoundError), patch( "homeassistant.loader.importlib.import_module", - side_effect=ImportError("Boom"), + side_effect=ModuleNotFoundError("Boom"), ), ): assert await integration.async_get_platform("light") == hue_light # Hue is not loaded so we should still hit the import_module path with ( - pytest.raises(ImportError), + pytest.raises(ModuleNotFoundError), patch( "homeassistant.loader.importlib.import_module", - side_effect=ImportError("Boom"), + side_effect=ModuleNotFoundError("Boom"), ), ): assert await integration.async_get_platform("light") == hue_light @@ -360,16 +416,16 @@ async def test_async_get_platform_caches_failures_when_component_loaded( # Hue is loaded so we should cache the import_module failure now with ( - pytest.raises(ImportError), + pytest.raises(ModuleNotFoundError), patch( "homeassistant.loader.importlib.import_module", - side_effect=ImportError("Boom"), + side_effect=ModuleNotFoundError("Boom"), ), ): assert await integration.async_get_platform("light") == hue_light # Hue is loaded and the last call should have cached the import_module failure - with pytest.raises(ImportError): + with pytest.raises(ModuleNotFoundError): assert await integration.async_get_platform("light") == hue_light # The cache should never be filled because the import error is remembered @@ -379,33 +435,36 @@ async def test_async_get_platform_caches_failures_when_component_loaded( async def test_async_get_platforms_caches_failures_when_component_loaded( hass: HomeAssistant, ) -> None: - """Test async_get_platforms cache failures only when the component is loaded.""" + """Test async_get_platforms cache failures only when the component is loaded. + + Only ModuleNotFoundError is cached, ImportError is not cached. + """ integration = await loader.async_get_integration(hass, "hue") with ( - pytest.raises(ImportError), + pytest.raises(ModuleNotFoundError), patch( "homeassistant.loader.importlib.import_module", - side_effect=ImportError("Boom"), + side_effect=ModuleNotFoundError("Boom"), ), ): assert integration.get_component() == hue with ( - pytest.raises(ImportError), + pytest.raises(ModuleNotFoundError), patch( "homeassistant.loader.importlib.import_module", - side_effect=ImportError("Boom"), + side_effect=ModuleNotFoundError("Boom"), ), ): assert await integration.async_get_platforms(["light"]) == {"light": hue_light} # Hue is not loaded so we should still hit the import_module path with ( - pytest.raises(ImportError), + pytest.raises(ModuleNotFoundError), patch( "homeassistant.loader.importlib.import_module", - side_effect=ImportError("Boom"), + side_effect=ModuleNotFoundError("Boom"), ), ): assert await integration.async_get_platforms(["light"]) == {"light": hue_light} @@ -414,16 +473,16 @@ async def test_async_get_platforms_caches_failures_when_component_loaded( # Hue is loaded so we should cache the import_module failure now with ( - pytest.raises(ImportError), + pytest.raises(ModuleNotFoundError), patch( "homeassistant.loader.importlib.import_module", - side_effect=ImportError("Boom"), + side_effect=ModuleNotFoundError("Boom"), ), ): assert await integration.async_get_platforms(["light"]) == {"light": hue_light} # Hue is loaded and the last call should have cached the import_module failure - with pytest.raises(ImportError): + with pytest.raises(ModuleNotFoundError): assert await integration.async_get_platforms(["light"]) == {"light": hue_light} # The cache should never be filled because the import error is remembered