From 66538ba34eeffd00675afaa3959a77c3188a6af4 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Sun, 28 Apr 2024 17:29:00 -0500 Subject: [PATCH] Add thread safety checks to async_create_task (#116339) * Add thread safety checks to async_create_task Calling async_create_task from a thread almost always results in an fast crash. Since most internals are using async_create_background_task or other task APIs, and this is the one integrations seem to get wrong the most, add a thread safety check here * Add thread safety checks to async_create_task Calling async_create_task from a thread almost always results in an fast crash. Since most internals are using async_create_background_task or other task APIs, and this is the one integrations seem to get wrong the most, add a thread safety check here * missed one * Update homeassistant/core.py * fix mocks * one more internal * more places where internal can be used * more places where internal can be used * more places where internal can be used * internal one more place since this is high volume and was already eager_start --- homeassistant/bootstrap.py | 2 +- homeassistant/config_entries.py | 4 +- homeassistant/core.py | 37 ++++++++++++++++++- homeassistant/helpers/entity.py | 2 +- homeassistant/helpers/entity_component.py | 2 +- homeassistant/helpers/entity_platform.py | 2 +- homeassistant/helpers/integration_platform.py | 4 +- homeassistant/helpers/intent.py | 2 +- homeassistant/helpers/restore_state.py | 4 +- homeassistant/helpers/script.py | 4 +- homeassistant/helpers/storage.py | 2 +- homeassistant/setup.py | 2 +- tests/common.py | 8 ++-- tests/test_core.py | 18 +++++++-- 14 files changed, 70 insertions(+), 23 deletions(-) diff --git a/homeassistant/bootstrap.py b/homeassistant/bootstrap.py index cbc808eb0fa..fc5eedffc39 100644 --- a/homeassistant/bootstrap.py +++ b/homeassistant/bootstrap.py @@ -731,7 +731,7 @@ async def async_setup_multi_components( # to wait to be imported, and the sooner we can get the base platforms # loaded the sooner we can start loading the rest of the integrations. futures = { - domain: hass.async_create_task( + domain: hass.async_create_task_internal( async_setup_component(hass, domain, config), f"setup component {domain}", eager_start=True, diff --git a/homeassistant/config_entries.py b/homeassistant/config_entries.py index 73e1d8debd6..619b2a4b48a 100644 --- a/homeassistant/config_entries.py +++ b/homeassistant/config_entries.py @@ -1087,7 +1087,7 @@ class ConfigEntry: target: target to call. """ - task = hass.async_create_task( + task = hass.async_create_task_internal( target, f"{name} {self.title} {self.domain} {self.entry_id}", eager_start ) if eager_start and task.done(): @@ -1643,7 +1643,7 @@ class ConfigEntries: # starting a new flow with the 'unignore' step. If the integration doesn't # implement async_step_unignore then this will be a no-op. if entry.source == SOURCE_IGNORE: - self.hass.async_create_task( + self.hass.async_create_task_internal( self.hass.config_entries.flow.async_init( entry.domain, context={"source": SOURCE_UNIGNORE}, diff --git a/homeassistant/core.py b/homeassistant/core.py index a3150adc221..2b1b9756a50 100644 --- a/homeassistant/core.py +++ b/homeassistant/core.py @@ -785,7 +785,9 @@ class HomeAssistant: target: target to call. """ self.loop.call_soon_threadsafe( - functools.partial(self.async_create_task, target, name, eager_start=True) + functools.partial( + self.async_create_task_internal, target, name, eager_start=True + ) ) @callback @@ -800,6 +802,37 @@ class HomeAssistant: This method must be run in the event loop. If you are using this in your integration, use the create task methods on the config entry instead. + target: target to call. + """ + # We turned on asyncio debug in April 2024 in the dev containers + # in the hope of catching some of the issues that have been + # reported. It will take a while to get all the issues fixed in + # custom components. + # + # In 2025.5 we should guard the `verify_event_loop_thread` + # check with a check for the `hass.config.debug` flag being set as + # long term we don't want to be checking this in production + # environments since it is a performance hit. + self.verify_event_loop_thread("async_create_task") + return self.async_create_task_internal(target, name, eager_start) + + @callback + def async_create_task_internal( + self, + target: Coroutine[Any, Any, _R], + name: str | None = None, + eager_start: bool = True, + ) -> asyncio.Task[_R]: + """Create a task from within the event loop, internal use only. + + This method is intended to only be used by core internally + and should not be considered a stable API. We will make + breaking change to this function in the future and it + should not be used in integrations. + + This method must be run in the event loop. If you are using this in your + integration, use the create task methods on the config entry instead. + target: target to call. """ if eager_start: @@ -2695,7 +2728,7 @@ class ServiceRegistry: coro = self._execute_service(handler, service_call) if not blocking: - self._hass.async_create_task( + self._hass.async_create_task_internal( self._run_service_call_catch_exceptions(coro, service_call), f"service call background {service_call.domain}.{service_call.service}", eager_start=True, diff --git a/homeassistant/helpers/entity.py b/homeassistant/helpers/entity.py index a91b4c32d21..6352a56dc90 100644 --- a/homeassistant/helpers/entity.py +++ b/homeassistant/helpers/entity.py @@ -1490,7 +1490,7 @@ class Entity( is_remove = action == "remove" self._removed_from_registry = is_remove if action == "update" or is_remove: - self.hass.async_create_task( + self.hass.async_create_task_internal( self._async_process_registry_update_or_remove(event), eager_start=True ) diff --git a/homeassistant/helpers/entity_component.py b/homeassistant/helpers/entity_component.py index f467b5683a9..eb54d83e1dd 100644 --- a/homeassistant/helpers/entity_component.py +++ b/homeassistant/helpers/entity_component.py @@ -146,7 +146,7 @@ class EntityComponent(Generic[_EntityT]): # Look in config for Domain, Domain 2, Domain 3 etc and load them for p_type, p_config in conf_util.config_per_platform(config, self.domain): if p_type is not None: - self.hass.async_create_task( + self.hass.async_create_task_internal( self.async_setup_platform(p_type, p_config), f"EntityComponent setup platform {p_type} {self.domain}", eager_start=True, diff --git a/homeassistant/helpers/entity_platform.py b/homeassistant/helpers/entity_platform.py index 2b9a5d436ed..f95c0a0b66a 100644 --- a/homeassistant/helpers/entity_platform.py +++ b/homeassistant/helpers/entity_platform.py @@ -477,7 +477,7 @@ class EntityPlatform: self, new_entities: Iterable[Entity], update_before_add: bool = False ) -> None: """Schedule adding entities for a single platform async.""" - task = self.hass.async_create_task( + task = self.hass.async_create_task_internal( self.async_add_entities(new_entities, update_before_add=update_before_add), f"EntityPlatform async_add_entities {self.domain}.{self.platform_name}", eager_start=True, diff --git a/homeassistant/helpers/integration_platform.py b/homeassistant/helpers/integration_platform.py index be525b384e0..fbd26019b64 100644 --- a/homeassistant/helpers/integration_platform.py +++ b/homeassistant/helpers/integration_platform.py @@ -85,7 +85,7 @@ def _async_integration_platform_component_loaded( # At least one of the platforms is not loaded, we need to load them # so we have to fall back to creating a task. - hass.async_create_task( + hass.async_create_task_internal( _async_process_integration_platforms_for_component( hass, integration, platforms_that_exist, integration_platforms_by_name ), @@ -206,7 +206,7 @@ async def async_process_integration_platforms( # We use hass.async_create_task instead of asyncio.create_task because # we want to make sure that startup waits for the task to complete. # - future = hass.async_create_task( + future = hass.async_create_task_internal( _async_process_integration_platforms( hass, platform_name, top_level_components.copy(), process_job ), diff --git a/homeassistant/helpers/intent.py b/homeassistant/helpers/intent.py index 0ddf4a1e329..119142ec14a 100644 --- a/homeassistant/helpers/intent.py +++ b/homeassistant/helpers/intent.py @@ -659,7 +659,7 @@ class DynamicServiceIntentHandler(IntentHandler): ) await self._run_then_background( - hass.async_create_task( + hass.async_create_task_internal( hass.services.async_call( domain, service, diff --git a/homeassistant/helpers/restore_state.py b/homeassistant/helpers/restore_state.py index 40c898fe1d2..2b3afc2f57b 100644 --- a/homeassistant/helpers/restore_state.py +++ b/homeassistant/helpers/restore_state.py @@ -236,7 +236,9 @@ class RestoreStateData: # Dump the initial states now. This helps minimize the risk of having # old states loaded by overwriting the last states once Home Assistant # has started and the old states have been read. - self.hass.async_create_task(_async_dump_states(), "RestoreStateData dump") + self.hass.async_create_task_internal( + _async_dump_states(), "RestoreStateData dump" + ) # Dump states periodically cancel_interval = async_track_time_interval( diff --git a/homeassistant/helpers/script.py b/homeassistant/helpers/script.py index d739fbfef98..1bbe7749ff7 100644 --- a/homeassistant/helpers/script.py +++ b/homeassistant/helpers/script.py @@ -734,7 +734,7 @@ class _ScriptRun: ) trace_set_result(params=params, running_script=running_script) response_data = await self._async_run_long_action( - self._hass.async_create_task( + self._hass.async_create_task_internal( self._hass.services.async_call( **params, blocking=True, @@ -1208,7 +1208,7 @@ class _ScriptRun: async def _async_run_script(self, script: Script) -> None: """Execute a script.""" result = await self._async_run_long_action( - self._hass.async_create_task( + self._hass.async_create_task_internal( script.async_run(self._variables, self._context), eager_start=True ) ) diff --git a/homeassistant/helpers/storage.py b/homeassistant/helpers/storage.py index 20054274275..315d28e06e6 100644 --- a/homeassistant/helpers/storage.py +++ b/homeassistant/helpers/storage.py @@ -468,7 +468,7 @@ class Store(Generic[_T]): # wrote. Reschedule the timer to the next write time. self._async_reschedule_delayed_write(self._next_write_time) return - self.hass.async_create_task( + self.hass.async_create_task_internal( self._async_callback_delayed_write(), eager_start=True ) diff --git a/homeassistant/setup.py b/homeassistant/setup.py index fab70e31d9d..7ba51b644e5 100644 --- a/homeassistant/setup.py +++ b/homeassistant/setup.py @@ -600,7 +600,7 @@ def _async_when_setup( _LOGGER.exception("Error handling when_setup callback for %s", component) if component in hass.config.components: - hass.async_create_task( + hass.async_create_task_internal( when_setup(), f"when setup {component}", eager_start=True ) return diff --git a/tests/common.py b/tests/common.py index b5fe0f7bae1..a3af2a3103b 100644 --- a/tests/common.py +++ b/tests/common.py @@ -234,7 +234,7 @@ async def async_test_home_assistant( orig_async_add_job = hass.async_add_job orig_async_add_executor_job = hass.async_add_executor_job - orig_async_create_task = hass.async_create_task + orig_async_create_task_internal = hass.async_create_task_internal orig_tz = dt_util.DEFAULT_TIME_ZONE def async_add_job(target, *args, eager_start: bool = False): @@ -263,18 +263,18 @@ async def async_test_home_assistant( return orig_async_add_executor_job(target, *args) - def async_create_task(coroutine, name=None, eager_start=True): + def async_create_task_internal(coroutine, name=None, eager_start=True): """Create task.""" if isinstance(coroutine, Mock) and not isinstance(coroutine, AsyncMock): fut = asyncio.Future() fut.set_result(None) return fut - return orig_async_create_task(coroutine, name, eager_start) + return orig_async_create_task_internal(coroutine, name, eager_start) hass.async_add_job = async_add_job hass.async_add_executor_job = async_add_executor_job - hass.async_create_task = async_create_task + hass.async_create_task_internal = async_create_task_internal hass.data[loader.DATA_CUSTOM_COMPONENTS] = {} diff --git a/tests/test_core.py b/tests/test_core.py index a553d5bbbed..66b5be718b1 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -329,7 +329,7 @@ async def test_async_create_task_schedule_coroutine() -> None: async def job(): pass - ha.HomeAssistant.async_create_task(hass, job(), eager_start=False) + ha.HomeAssistant.async_create_task_internal(hass, job(), eager_start=False) assert len(hass.loop.call_soon.mock_calls) == 0 assert len(hass.loop.create_task.mock_calls) == 1 assert len(hass.add_job.mock_calls) == 0 @@ -342,7 +342,7 @@ async def test_async_create_task_eager_start_schedule_coroutine() -> None: async def job(): pass - ha.HomeAssistant.async_create_task(hass, job(), eager_start=True) + ha.HomeAssistant.async_create_task_internal(hass, job(), eager_start=True) # Should create the task directly since 3.12 supports eager_start assert len(hass.loop.create_task.mock_calls) == 0 assert len(hass.add_job.mock_calls) == 0 @@ -355,7 +355,7 @@ async def test_async_create_task_schedule_coroutine_with_name() -> None: async def job(): pass - task = ha.HomeAssistant.async_create_task( + task = ha.HomeAssistant.async_create_task_internal( hass, job(), "named task", eager_start=False ) assert len(hass.loop.call_soon.mock_calls) == 0 @@ -3480,3 +3480,15 @@ async def test_async_remove_thread_safety(hass: HomeAssistant) -> None: await hass.async_add_executor_job( hass.services.async_remove, "test_domain", "test_service" ) + + +async def test_async_create_task_thread_safety(hass: HomeAssistant) -> None: + """Test async_create_task thread safety.""" + + async def _any_coro(): + pass + + with pytest.raises( + RuntimeError, match="Detected code that calls async_create_task from a thread." + ): + await hass.async_add_executor_job(hass.async_create_task, _any_coro)