Update service call return values and error handling (#94657)

* Update return signature of service calls

* Add timeout error handling in websocket api for service calls

* Update recorder tests to remove assertion on service call

* Remove timeout behavior and update callers that depend on it today

* Fix tests

* Add missing else

* await coro directly

* Fix more tests

* Update the intent task to use wait instead of timeout

* Remove script service call limits and limit constants

* Update tests that depend on service call limits

* Use wait instead of wait_for and add test

* Update homeassistant/helpers/intent.py

Co-authored-by: Martin Hjelmare <[email protected]>

---------

Co-authored-by: Paulus Schoutsen <[email protected]>
Co-authored-by: Martin Hjelmare <[email protected]>

Author: 3 people

homeassistant/components/intent/__init__.py

@@ -70,18 +70,22 @@ async def async_call_service(self, intent_obj: intent.Intent, state: State) -> N
         if state.domain == COVER_DOMAIN:
             # on = open
             # off = close
-            await hass.services.async_call(
-                COVER_DOMAIN,
-                SERVICE_OPEN_COVER
-                if self.service == SERVICE_TURN_ON
-                else SERVICE_CLOSE_COVER,
-                {ATTR_ENTITY_ID: state.entity_id},
-                context=intent_obj.context,
-                blocking=True,
-                limit=self.service_timeout,
+            await self._run_then_background(
+                hass.async_create_task(
+                    hass.services.async_call(
+                        COVER_DOMAIN,
+                        SERVICE_OPEN_COVER
+                        if self.service == SERVICE_TURN_ON
+                        else SERVICE_CLOSE_COVER,
+                        {ATTR_ENTITY_ID: state.entity_id},
+                        context=intent_obj.context,
+                        blocking=True,
+                    )
+                )
             )
+            return
 
-        elif not hass.services.has_service(state.domain, self.service):
+        if not hass.services.has_service(state.domain, self.service):
             raise intent.IntentHandleError(
                 f"Service {self.service} does not support entity {state.entity_id}"
             )

homeassistant/core.py

@@ -130,9 +130,6 @@
 # How long to wait to log tasks that are blocking
 BLOCK_LOG_TIMEOUT = 60
 
-# How long we wait for the result of a service call
-SERVICE_CALL_LIMIT = 10  # seconds
-
 
 class ConfigSource(StrEnum):
     """Source of core configuration."""
@@ -1807,17 +1804,14 @@ def call(
         service_data: dict[str, Any] | None = None,
         blocking: bool = False,
         context: Context | None = None,
-        limit: float | None = SERVICE_CALL_LIMIT,
         target: dict[str, Any] | None = None,
     ) -> bool | None:
         """Call a service.
 
         See description of async_call for details.
         """
         return asyncio.run_coroutine_threadsafe(
-            self.async_call(
-                domain, service, service_data, blocking, context, limit, target
-            ),
+            self.async_call(domain, service, service_data, blocking, context, target),
             self._hass.loop,
         ).result()
 
@@ -1828,16 +1822,11 @@ async def async_call(
         service_data: dict[str, Any] | None = None,
         blocking: bool = False,
         context: Context | None = None,
-        limit: float | None = SERVICE_CALL_LIMIT,
         target: dict[str, Any] | None = None,
-    ) -> bool | None:
+    ) -> None:
         """Call a service.
 
         Specify blocking=True to wait until service is executed.
-        Waits a maximum of limit, which may be None for no timeout.
-
-        If blocking = True, will return boolean if service executed
-        successfully within limit.
 
         This method will fire an event to indicate the service has been called.
 
@@ -1888,33 +1877,9 @@ async def async_call(
         coro = self._execute_service(handler, service_call)
         if not blocking:
             self._run_service_in_background(coro, service_call)
-            return None
+            return
 
-        task = self._hass.async_create_task(coro)
-        try:
-            await asyncio.wait({task}, timeout=limit)
-        except asyncio.CancelledError:
-            # Task calling us was cancelled, so cancel service call task, and wait for
-            # it to be cancelled, within reason, before leaving.
-            _LOGGER.debug("Service call was cancelled: %s", service_call)
-            task.cancel()
-            await asyncio.wait({task}, timeout=SERVICE_CALL_LIMIT)
-            raise
-
-        if task.cancelled():
-            # Service call task was cancelled some other way, such as during shutdown.
-            _LOGGER.debug("Service was cancelled: %s", service_call)
-            raise asyncio.CancelledError
-        if task.done():
-            # Propagate any exceptions that might have happened during service call.
-            task.result()
-            # Service call completed successfully!
-            return True
-        # Service call task did not complete before timeout expired.
-        # Let it keep running in background.
-        self._run_service_in_background(task, service_call)
-        _LOGGER.debug("Service did not complete before timeout: %s", service_call)
-        return False
+        await coro
 
     def _run_service_in_background(
         self,

homeassistant/helpers/intent.py

@@ -493,15 +493,36 @@ async def async_handle_states(
     async def async_call_service(self, intent_obj: Intent, state: State) -> None:
         """Call service on entity."""
         hass = intent_obj.hass
-        await hass.services.async_call(
-            self.domain,
-            self.service,
-            {ATTR_ENTITY_ID: state.entity_id},
-            context=intent_obj.context,
-            blocking=True,
-            limit=self.service_timeout,
+        await self._run_then_background(
+            hass.async_create_task(
+                hass.services.async_call(
+                    self.domain,
+                    self.service,
+                    {ATTR_ENTITY_ID: state.entity_id},
+                    context=intent_obj.context,
+                    blocking=True,
+                ),
+                f"intent_call_service_{self.domain}_{self.service}",
+            )
         )
 
+    async def _run_then_background(self, task: asyncio.Task) -> None:
+        """Run task with timeout to (hopefully) catch validation errors.
+
+        After the timeout the task will continue to run in the background.
+        """
+        try:
+            await asyncio.wait({task}, timeout=self.service_timeout)
+        except asyncio.TimeoutError:
+            pass
+        except asyncio.CancelledError:
+            # Task calling us was cancelled, so cancel service call task, and wait for
+            # it to be cancelled, within reason, before leaving.
+            _LOGGER.debug("Service call was cancelled: %s", task.get_name())
+            task.cancel()
+            await asyncio.wait({task}, timeout=5)
+            raise
+
 
 class IntentCategory(Enum):
     """Category of an intent."""

homeassistant/helpers/script.py

@@ -64,7 +64,6 @@
     SERVICE_TURN_ON,
 )
 from homeassistant.core import (
-    SERVICE_CALL_LIMIT,
     Context,
     Event,
     HassJob,
@@ -664,28 +663,16 @@ async def _async_call_service_step(self):
             and params[CONF_SERVICE] == "trigger"
             or params[CONF_DOMAIN] in ("python_script", "script")
         )
-        # If this might start a script then disable the call timeout.
-        # Otherwise use the normal service call limit.
-        if running_script:
-            limit = None
-        else:
-            limit = SERVICE_CALL_LIMIT
-
-        trace_set_result(params=params, running_script=running_script, limit=limit)
-        service_task = self._hass.async_create_task(
-            self._hass.services.async_call(
-                **params,
-                blocking=True,
-                context=self._context,
-                limit=limit,
+        trace_set_result(params=params, running_script=running_script)
+        await self._async_run_long_action(
+            self._hass.async_create_task(
+                self._hass.services.async_call(
+                    **params,
+                    blocking=True,
+                    context=self._context,
+                )
             )
         )
-        if limit is not None:
-            # There is a call limit, so just wait for it to finish.
-            await service_task
-            return
-
-        await self._async_run_long_action(service_task)
 
     async def _async_device_step(self):
         """Perform the device automation specified in the action."""

tests/common.py

@@ -1353,7 +1353,7 @@ def async_capture_events(hass: HomeAssistant, event_name: str) -> list[Event]:
     def capture_events(event: Event) -> None:
         events.append(event)
 
-    hass.bus.async_listen(event_name, capture_events)
+    hass.bus.async_listen(event_name, capture_events, run_immediately=True)
 
     return events
 

tests/components/abode/test_light.py

@@ -63,7 +63,7 @@ async def test_switch_off(hass: HomeAssistant) -> None:
     await setup_platform(hass, LIGHT_DOMAIN)
 
     with patch("jaraco.abode.devices.light.Light.switch_off") as mock_switch_off:
-        assert await hass.services.async_call(
+        await hass.services.async_call(
             LIGHT_DOMAIN, SERVICE_TURN_OFF, {ATTR_ENTITY_ID: DEVICE_ID}, blocking=True
         )
         await hass.async_block_till_done()

tests/components/abode/test_switch.py

@@ -49,7 +49,7 @@ async def test_switch_on(hass: HomeAssistant) -> None:
     await setup_platform(hass, SWITCH_DOMAIN)
 
     with patch("jaraco.abode.devices.switch.Switch.switch_on") as mock_switch_on:
-        assert await hass.services.async_call(
+        await hass.services.async_call(
             SWITCH_DOMAIN, SERVICE_TURN_ON, {ATTR_ENTITY_ID: DEVICE_ID}, blocking=True
         )
         await hass.async_block_till_done()
@@ -62,7 +62,7 @@ async def test_switch_off(hass: HomeAssistant) -> None:
     await setup_platform(hass, SWITCH_DOMAIN)
 
     with patch("jaraco.abode.devices.switch.Switch.switch_off") as mock_switch_off:
-        assert await hass.services.async_call(
+        await hass.services.async_call(
             SWITCH_DOMAIN, SERVICE_TURN_OFF, {ATTR_ENTITY_ID: DEVICE_ID}, blocking=True
         )
         await hass.async_block_till_done()