From 3f4689881e7068e2ba3d20d57f44cd23c1c949ec Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Tue, 11 Sep 2018 13:11:35 +0200 Subject: [PATCH 1/3] Fix invalid state --- homeassistant/const.py | 3 +++ homeassistant/core.py | 18 ++++++++++++++---- tests/test_core.py | 21 ++++++++++++++++++++- 3 files changed, 37 insertions(+), 5 deletions(-) diff --git a/homeassistant/const.py b/homeassistant/const.py index d27b5e3a1b7967..b9b71734241b7c 100644 --- a/homeassistant/const.py +++ b/homeassistant/const.py @@ -225,6 +225,9 @@ # Name ATTR_NAME = 'name' +# Data for a SERVICE_EXECUTED event +ATTR_SERVICE_CALL_ID = 'service_call_id' + # Contains one string or a list of strings, each being an entity id ATTR_ENTITY_ID = 'entity_id' diff --git a/homeassistant/core.py b/homeassistant/core.py index fdbbe49ea05d69..e3e42d4243e9a1 100644 --- a/homeassistant/core.py +++ b/homeassistant/core.py @@ -29,7 +29,7 @@ from homeassistant.const import ( ATTR_DOMAIN, ATTR_FRIENDLY_NAME, ATTR_NOW, ATTR_SERVICE, - ATTR_SERVICE_DATA, EVENT_CALL_SERVICE, + ATTR_SERVICE_CALL_ID, ATTR_SERVICE_DATA, EVENT_CALL_SERVICE, EVENT_HOMEASSISTANT_START, EVENT_HOMEASSISTANT_STOP, EVENT_SERVICE_EXECUTED, EVENT_SERVICE_REGISTERED, EVENT_STATE_CHANGED, EVENT_TIME_CHANGED, MATCH_ALL, EVENT_HOMEASSISTANT_CLOSE, @@ -904,6 +904,7 @@ def __init__(self, hass: HomeAssistant) -> None: self._services = {} # type: Dict[str, Dict[str, Service]] self._hass = hass self._async_unsub_call_event = None # type: Optional[CALLBACK_TYPE] + self._call_id = 0 @property def services(self) -> Dict[str, Dict[str, Service]]: @@ -1042,10 +1043,13 @@ async def async_call(self, domain: str, service: str, This method is a coroutine. """ context = context or Context() + self._call_id += 1 + call_id = self._call_id event_data = { ATTR_DOMAIN: domain.lower(), ATTR_SERVICE: service.lower(), ATTR_SERVICE_DATA: service_data, + ATTR_SERVICE_CALL_ID: call_id, } if not blocking: @@ -1058,7 +1062,7 @@ async def async_call(self, domain: str, service: str, @callback def service_executed(event: Event) -> None: """Handle an executed service.""" - if event.context == context: + if event.data[ATTR_SERVICE_CALL_ID] == call_id: fut.set_result(True) unsub = self._hass.bus.async_listen( @@ -1077,6 +1081,7 @@ async def _event_to_service_call(self, event: Event) -> None: service_data = event.data.get(ATTR_SERVICE_DATA) or {} domain = event.data.get(ATTR_DOMAIN).lower() # type: ignore service = event.data.get(ATTR_SERVICE).lower() # type: ignore + call_id = event.data.get(ATTR_SERVICE_CALL_ID) if not self.has_service(domain, service): if event.origin == EventOrigin.local: @@ -1088,12 +1093,17 @@ async def _event_to_service_call(self, event: Event) -> None: def fire_service_executed() -> None: """Fire service executed event.""" + if not call_id: + return + + data = {ATTR_SERVICE_CALL_ID: call_id} + if (service_handler.is_coroutinefunction or service_handler.is_callback): - self._hass.bus.async_fire(EVENT_SERVICE_EXECUTED, {}, + self._hass.bus.async_fire(EVENT_SERVICE_EXECUTED, data, EventOrigin.local, event.context) else: - self._hass.bus.fire(EVENT_SERVICE_EXECUTED, {}, + self._hass.bus.fire(EVENT_SERVICE_EXECUTED, data, EventOrigin.local, event.context) try: diff --git a/tests/test_core.py b/tests/test_core.py index ce066135709f1f..de3201ff5228d1 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -22,7 +22,7 @@ ATTR_NOW, EVENT_TIME_CHANGED, EVENT_HOMEASSISTANT_STOP, EVENT_HOMEASSISTANT_CLOSE, EVENT_SERVICE_REGISTERED, EVENT_SERVICE_REMOVED) -from tests.common import get_test_home_assistant +from tests.common import get_test_home_assistant, async_mock_service PST = pytz.timezone('America/Los_Angeles') @@ -969,3 +969,22 @@ def test_track_task_functions(loop): assert hass._track_task finally: yield from hass.async_stop() + + +async def test_service_executed_with_subservices(hass): + """Test we block correctly till all services done.""" + calls = async_mock_service(hass, 'test', 'inner') + + async def handle_outer(call): + """Handle outer service call.""" + calls.append(call) + await hass.services.async_call('test', 'inner', blocking=True, + context=call.context) + calls.append(call) + + hass.services.async_register('test', 'outer', handle_outer) + + await hass.services.async_call('test', 'outer', blocking=True) + + assert len(calls) == 3 + assert [call.service for call in calls] == ['outer', 'inner', 'outer'] From e4618ef75df43ecac73b0485b75516a82d98912a Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Tue, 11 Sep 2018 13:17:49 +0200 Subject: [PATCH 2/3] Make slightly more efficient in unsubscribing --- homeassistant/core.py | 4 +++- tests/test_core.py | 16 +++++++++++----- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/homeassistant/core.py b/homeassistant/core.py index e3e42d4243e9a1..0771b9db7a759f 100644 --- a/homeassistant/core.py +++ b/homeassistant/core.py @@ -1064,6 +1064,7 @@ def service_executed(event: Event) -> None: """Handle an executed service.""" if event.data[ATTR_SERVICE_CALL_ID] == call_id: fut.set_result(True) + unsub() unsub = self._hass.bus.async_listen( EVENT_SERVICE_EXECUTED, service_executed) @@ -1073,7 +1074,8 @@ def service_executed(event: Event) -> None: done, _ = await asyncio.wait([fut], timeout=SERVICE_CALL_LIMIT) success = bool(done) - unsub() + if not success: + unsub() return success async def _event_to_service_call(self, event: Event) -> None: diff --git a/tests/test_core.py b/tests/test_core.py index de3201ff5228d1..7e6d57136e45c6 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -20,7 +20,8 @@ from homeassistant.const import ( __version__, EVENT_STATE_CHANGED, ATTR_FRIENDLY_NAME, CONF_UNIT_SYSTEM, ATTR_NOW, EVENT_TIME_CHANGED, EVENT_HOMEASSISTANT_STOP, - EVENT_HOMEASSISTANT_CLOSE, EVENT_SERVICE_REGISTERED, EVENT_SERVICE_REMOVED) + EVENT_HOMEASSISTANT_CLOSE, EVENT_SERVICE_REGISTERED, EVENT_SERVICE_REMOVED, + EVENT_SERVICE_EXECUTED) from tests.common import get_test_home_assistant, async_mock_service @@ -978,13 +979,18 @@ async def test_service_executed_with_subservices(hass): async def handle_outer(call): """Handle outer service call.""" calls.append(call) - await hass.services.async_call('test', 'inner', blocking=True, - context=call.context) + call1 = hass.services.async_call('test', 'inner', blocking=True, + context=call.context) + call2 = hass.services.async_call('test', 'inner', blocking=True, + context=call.context) + await asyncio.wait([call1, call2]) calls.append(call) hass.services.async_register('test', 'outer', handle_outer) await hass.services.async_call('test', 'outer', blocking=True) - assert len(calls) == 3 - assert [call.service for call in calls] == ['outer', 'inner', 'outer'] + assert len(calls) == 4 + assert [call.service for call in calls] == [ + 'outer', 'inner', 'inner', 'outer'] + assert len(hass.bus.async_listeners().get(EVENT_SERVICE_EXECUTED, [])) == 0 From edf431d028b53b30f8c4958339268a3279b5375b Mon Sep 17 00:00:00 2001 From: Paulus Schoutsen Date: Tue, 11 Sep 2018 21:25:46 +0200 Subject: [PATCH 3/3] Use uuid4" --- homeassistant/core.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/homeassistant/core.py b/homeassistant/core.py index 0771b9db7a759f..39ee20cb1a8c82 100644 --- a/homeassistant/core.py +++ b/homeassistant/core.py @@ -904,7 +904,6 @@ def __init__(self, hass: HomeAssistant) -> None: self._services = {} # type: Dict[str, Dict[str, Service]] self._hass = hass self._async_unsub_call_event = None # type: Optional[CALLBACK_TYPE] - self._call_id = 0 @property def services(self) -> Dict[str, Dict[str, Service]]: @@ -1043,8 +1042,7 @@ async def async_call(self, domain: str, service: str, This method is a coroutine. """ context = context or Context() - self._call_id += 1 - call_id = self._call_id + call_id = uuid.uuid4().hex event_data = { ATTR_DOMAIN: domain.lower(), ATTR_SERVICE: service.lower(),