From bee55a0494123b102d662bc275e28fe777f02a61 Mon Sep 17 00:00:00 2001 From: "J. Nick Koston" Date: Wed, 31 Mar 2021 03:06:49 -1000 Subject: [PATCH] Prevent ping integration from delaying startup (#43869) --- .coveragerc | 1 + homeassistant/components/ping/__init__.py | 46 +++++- .../components/ping/binary_sensor.py | 102 +++++++++----- homeassistant/components/ping/const.py | 17 +++ .../components/ping/device_tracker.py | 133 +++++++++--------- homeassistant/components/ping/manifest.json | 2 +- requirements_all.txt | 2 +- requirements_test_all.txt | 2 +- 8 files changed, 196 insertions(+), 109 deletions(-) diff --git a/.coveragerc b/.coveragerc index 24ceeae65be..0cadadfc5e8 100644 --- a/.coveragerc +++ b/.coveragerc @@ -741,6 +741,7 @@ omit = homeassistant/components/picotts/tts.py homeassistant/components/piglow/light.py homeassistant/components/pilight/* + homeassistant/components/ping/__init__.py homeassistant/components/ping/const.py homeassistant/components/ping/binary_sensor.py homeassistant/components/ping/device_tracker.py diff --git a/homeassistant/components/ping/__init__.py b/homeassistant/components/ping/__init__.py index 19207ada1b7..726bb212574 100644 --- a/homeassistant/components/ping/__init__.py +++ b/homeassistant/components/ping/__init__.py @@ -1,13 +1,26 @@ """The ping component.""" +from __future__ import annotations + +import logging + +from icmplib import SocketPermissionError, ping as icmp_ping from homeassistant.core import callback +from homeassistant.helpers.reload import async_setup_reload_service -DOMAIN = "ping" -PLATFORMS = ["binary_sensor"] +from .const import DEFAULT_START_ID, DOMAIN, MAX_PING_ID, PING_ID, PING_PRIVS, PLATFORMS -PING_ID = "ping_id" -DEFAULT_START_ID = 129 -MAX_PING_ID = 65534 +_LOGGER = logging.getLogger(__name__) + + +async def async_setup(hass, config): + """Set up the template integration.""" + await async_setup_reload_service(hass, DOMAIN, PLATFORMS) + hass.data[DOMAIN] = { + PING_PRIVS: await hass.async_add_executor_job(_can_use_icmp_lib_with_privilege), + PING_ID: DEFAULT_START_ID, + } + return True @callback @@ -16,8 +29,7 @@ def async_get_next_ping_id(hass): Must be called in async """ - current_id = hass.data.setdefault(DOMAIN, {}).get(PING_ID, DEFAULT_START_ID) - + current_id = hass.data[DOMAIN][PING_ID] if current_id == MAX_PING_ID: next_id = DEFAULT_START_ID else: @@ -26,3 +38,23 @@ def async_get_next_ping_id(hass): hass.data[DOMAIN][PING_ID] = next_id return next_id + + +def _can_use_icmp_lib_with_privilege() -> None | bool: + """Verify we can create a raw socket.""" + try: + icmp_ping("127.0.0.1", count=0, timeout=0, privileged=True) + except SocketPermissionError: + try: + icmp_ping("127.0.0.1", count=0, timeout=0, privileged=False) + except SocketPermissionError: + _LOGGER.debug( + "Cannot use icmplib because privileges are insufficient to create the socket" + ) + return None + else: + _LOGGER.debug("Using icmplib in privileged=False mode") + return False + else: + _LOGGER.debug("Using icmplib in privileged=True mode") + return True diff --git a/homeassistant/components/ping/binary_sensor.py b/homeassistant/components/ping/binary_sensor.py index 48e47937e2e..9ae891d598b 100644 --- a/homeassistant/components/ping/binary_sensor.py +++ b/homeassistant/components/ping/binary_sensor.py @@ -10,7 +10,7 @@ import re import sys from typing import Any -from icmplib import SocketPermissionError, ping as icmp_ping +from icmplib import NameLookupError, ping as icmp_ping import voluptuous as vol from homeassistant.components.binary_sensor import ( @@ -18,12 +18,12 @@ from homeassistant.components.binary_sensor import ( PLATFORM_SCHEMA, BinarySensorEntity, ) -from homeassistant.const import CONF_HOST, CONF_NAME +from homeassistant.const import CONF_HOST, CONF_NAME, STATE_ON import homeassistant.helpers.config_validation as cv -from homeassistant.helpers.reload import setup_reload_service +from homeassistant.helpers.restore_state import RestoreEntity -from . import DOMAIN, PLATFORMS, async_get_next_ping_id -from .const import PING_TIMEOUT +from . import async_get_next_ping_id +from .const import DOMAIN, ICMP_TIMEOUT, PING_PRIVS, PING_TIMEOUT _LOGGER = logging.getLogger(__name__) @@ -63,32 +63,30 @@ PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend( ) -def setup_platform(hass, config, add_entities, discovery_info=None) -> None: +async def async_setup_platform( + hass, config, async_add_entities, discovery_info=None +) -> None: """Set up the Ping Binary sensor.""" - setup_reload_service(hass, DOMAIN, PLATFORMS) - host = config[CONF_HOST] count = config[CONF_PING_COUNT] name = config.get(CONF_NAME, f"{DEFAULT_NAME} {host}") - - try: - # Verify we can create a raw socket, or - # fallback to using a subprocess - icmp_ping("127.0.0.1", count=0, timeout=0) - ping_cls = PingDataICMPLib - except SocketPermissionError: + privileged = hass.data[DOMAIN][PING_PRIVS] + if privileged is None: ping_cls = PingDataSubProcess + else: + ping_cls = PingDataICMPLib - ping_data = ping_cls(hass, host, count) - - add_entities([PingBinarySensor(name, ping_data)], True) + async_add_entities( + [PingBinarySensor(name, ping_cls(hass, host, count, privileged))] + ) -class PingBinarySensor(BinarySensorEntity): +class PingBinarySensor(RestoreEntity, BinarySensorEntity): """Representation of a Ping Binary sensor.""" def __init__(self, name: str, ping) -> None: """Initialize the Ping Binary sensor.""" + self._available = False self._name = name self._ping = ping @@ -97,6 +95,11 @@ class PingBinarySensor(BinarySensorEntity): """Return the name of the device.""" return self._name + @property + def available(self) -> str: + """Return if we have done the first ping.""" + return self._available + @property def device_class(self) -> str: """Return the class of this sensor.""" @@ -105,7 +108,7 @@ class PingBinarySensor(BinarySensorEntity): @property def is_on(self) -> bool: """Return true if the binary sensor is on.""" - return self._ping.available + return self._ping.is_alive @property def extra_state_attributes(self) -> dict[str, Any]: @@ -121,6 +124,28 @@ class PingBinarySensor(BinarySensorEntity): async def async_update(self) -> None: """Get the latest data.""" await self._ping.async_update() + self._available = True + + async def async_added_to_hass(self): + """Restore previous state on restart to avoid blocking startup.""" + await super().async_added_to_hass() + + last_state = await self.async_get_last_state() + if last_state is not None: + self._available = True + + if last_state is None or last_state.state != STATE_ON: + self._ping.data = False + return + + attributes = last_state.attributes + self._ping.is_alive = True + self._ping.data = { + "min": attributes[ATTR_ROUND_TRIP_TIME_AVG], + "max": attributes[ATTR_ROUND_TRIP_TIME_MAX], + "avg": attributes[ATTR_ROUND_TRIP_TIME_MDEV], + "mdev": attributes[ATTR_ROUND_TRIP_TIME_MIN], + } class PingData: @@ -132,26 +157,37 @@ class PingData: self._ip_address = host self._count = count self.data = {} - self.available = False + self.is_alive = False class PingDataICMPLib(PingData): """The Class for handling the data retrieval using icmplib.""" + def __init__(self, hass, host, count, privileged) -> None: + """Initialize the data object.""" + super().__init__(hass, host, count) + self._privileged = privileged + async def async_update(self) -> None: """Retrieve the latest details from the host.""" _LOGGER.debug("ping address: %s", self._ip_address) - data = await self.hass.async_add_executor_job( - partial( - icmp_ping, - self._ip_address, - count=self._count, - timeout=1, - id=async_get_next_ping_id(self.hass), + try: + data = await self.hass.async_add_executor_job( + partial( + icmp_ping, + self._ip_address, + count=self._count, + timeout=ICMP_TIMEOUT, + id=async_get_next_ping_id(self.hass), + privileged=self._privileged, + ) ) - ) - self.available = data.is_alive - if not self.available: + except NameLookupError: + self.is_alive = False + return + + self.is_alive = data.is_alive + if not self.is_alive: self.data = False return @@ -166,7 +202,7 @@ class PingDataICMPLib(PingData): class PingDataSubProcess(PingData): """The Class for handling the data retrieval using the ping binary.""" - def __init__(self, hass, host, count) -> None: + def __init__(self, hass, host, count, privileged) -> None: """Initialize the data object.""" super().__init__(hass, host, count) if sys.platform == "win32": @@ -254,4 +290,4 @@ class PingDataSubProcess(PingData): async def async_update(self) -> None: """Retrieve the latest details from the host.""" self.data = await self.async_ping() - self.available = bool(self.data) + self.is_alive = bool(self.data) diff --git a/homeassistant/components/ping/const.py b/homeassistant/components/ping/const.py index 89b93c84169..62fca9123ba 100644 --- a/homeassistant/components/ping/const.py +++ b/homeassistant/components/ping/const.py @@ -1,4 +1,21 @@ """Tracks devices by sending a ICMP echo request (ping).""" +# The ping binary and icmplib timeouts are not the same +# timeout. ping is an overall timeout, icmplib is the +# time since the data was sent. + +# ping binary PING_TIMEOUT = 3 + +# icmplib timeout +ICMP_TIMEOUT = 1 + PING_ATTEMPTS_COUNT = 3 + +DOMAIN = "ping" +PLATFORMS = ["binary_sensor"] + +PING_ID = "ping_id" +PING_PRIVS = "ping_privs" +DEFAULT_START_ID = 129 +MAX_PING_ID = 65534 diff --git a/homeassistant/components/ping/device_tracker.py b/homeassistant/components/ping/device_tracker.py index 4139438bcfe..a6b75a9245b 100644 --- a/homeassistant/components/ping/device_tracker.py +++ b/homeassistant/components/ping/device_tracker.py @@ -1,10 +1,12 @@ """Tracks devices by sending a ICMP echo request (ping).""" +import asyncio from datetime import timedelta +from functools import partial import logging import subprocess import sys -from icmplib import SocketPermissionError, ping as icmp_ping +from icmplib import multiping import voluptuous as vol from homeassistant import const, util @@ -15,16 +17,18 @@ from homeassistant.components.device_tracker.const import ( SOURCE_TYPE_ROUTER, ) import homeassistant.helpers.config_validation as cv -from homeassistant.util.async_ import run_callback_threadsafe +from homeassistant.helpers.event import async_track_point_in_utc_time +from homeassistant.util.async_ import gather_with_concurrency from homeassistant.util.process import kill_subprocess from . import async_get_next_ping_id -from .const import PING_ATTEMPTS_COUNT, PING_TIMEOUT +from .const import DOMAIN, ICMP_TIMEOUT, PING_ATTEMPTS_COUNT, PING_PRIVS, PING_TIMEOUT _LOGGER = logging.getLogger(__name__) PARALLEL_UPDATES = 0 CONF_PING_COUNT = "count" +CONCURRENT_PING_LIMIT = 6 PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend( { @@ -37,16 +41,16 @@ PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend( class HostSubProcess: """Host object with ping detection.""" - def __init__(self, ip_address, dev_id, hass, config): + def __init__(self, ip_address, dev_id, hass, config, privileged): """Initialize the Host pinger.""" self.hass = hass self.ip_address = ip_address self.dev_id = dev_id self._count = config[CONF_PING_COUNT] if sys.platform == "win32": - self._ping_cmd = ["ping", "-n", "1", "-w", "1000", self.ip_address] + self._ping_cmd = ["ping", "-n", "1", "-w", "1000", ip_address] else: - self._ping_cmd = ["ping", "-n", "-q", "-c1", "-W1", self.ip_address] + self._ping_cmd = ["ping", "-n", "-q", "-c1", "-W1", ip_address] def ping(self): """Send an ICMP echo request and return True if success.""" @@ -63,86 +67,83 @@ class HostSubProcess: except subprocess.CalledProcessError: return False - def update(self, see): + def update(self) -> bool: """Update device state by sending one or more ping messages.""" failed = 0 while failed < self._count: # check more times if host is unreachable if self.ping(): - see(dev_id=self.dev_id, source_type=SOURCE_TYPE_ROUTER) return True failed += 1 _LOGGER.debug("No response from %s failed=%d", self.ip_address, failed) + return False -class HostICMPLib: - """Host object with ping detection.""" - - def __init__(self, ip_address, dev_id, hass, config): - """Initialize the Host pinger.""" - self.hass = hass - self.ip_address = ip_address - self.dev_id = dev_id - self._count = config[CONF_PING_COUNT] - - def ping(self): - """Send an ICMP echo request and return True if success.""" - next_id = run_callback_threadsafe( - self.hass.loop, async_get_next_ping_id, self.hass - ).result() - - return icmp_ping( - self.ip_address, count=PING_ATTEMPTS_COUNT, timeout=1, id=next_id - ).is_alive - - def update(self, see): - """Update device state by sending one or more ping messages.""" - if self.ping(): - see(dev_id=self.dev_id, source_type=SOURCE_TYPE_ROUTER) - return True - - _LOGGER.debug( - "No response from %s (%s) failed=%d", - self.ip_address, - self.dev_id, - PING_ATTEMPTS_COUNT, - ) - - -def setup_scanner(hass, config, see, discovery_info=None): +async def async_setup_scanner(hass, config, async_see, discovery_info=None): """Set up the Host objects and return the update function.""" - try: - # Verify we can create a raw socket, or - # fallback to using a subprocess - icmp_ping("127.0.0.1", count=0, timeout=0) - host_cls = HostICMPLib - except SocketPermissionError: - host_cls = HostSubProcess - - hosts = [ - host_cls(ip, dev_id, hass, config) - for (dev_id, ip) in config[const.CONF_HOSTS].items() - ] + privileged = hass.data[DOMAIN][PING_PRIVS] + ip_to_dev_id = {ip: dev_id for (dev_id, ip) in config[const.CONF_HOSTS].items()} interval = config.get( CONF_SCAN_INTERVAL, - timedelta(seconds=len(hosts) * config[CONF_PING_COUNT]) + SCAN_INTERVAL, + timedelta(seconds=len(ip_to_dev_id) * config[CONF_PING_COUNT]) + SCAN_INTERVAL, ) _LOGGER.debug( "Started ping tracker with interval=%s on hosts: %s", interval, - ",".join([host.ip_address for host in hosts]), + ",".join(ip_to_dev_id.keys()), ) - def update_interval(now): - """Update all the hosts on every interval time.""" - try: - for host in hosts: - host.update(see) - finally: - hass.helpers.event.track_point_in_utc_time( - update_interval, util.dt.utcnow() + interval + if privileged is None: + hosts = [ + HostSubProcess(ip, dev_id, hass, config, privileged) + for (dev_id, ip) in config[const.CONF_HOSTS].items() + ] + + async def async_update(now): + """Update all the hosts on every interval time.""" + results = await gather_with_concurrency( + CONCURRENT_PING_LIMIT, + *[hass.async_add_executor_job(host.update) for host in hosts], + ) + await asyncio.gather( + *[ + async_see(dev_id=host.dev_id, source_type=SOURCE_TYPE_ROUTER) + for idx, host in enumerate(hosts) + if results[idx] + ] ) - update_interval(None) + else: + + async def async_update(now): + """Update all the hosts on every interval time.""" + responses = await hass.async_add_executor_job( + partial( + multiping, + ip_to_dev_id.keys(), + count=PING_ATTEMPTS_COUNT, + timeout=ICMP_TIMEOUT, + privileged=privileged, + id=async_get_next_ping_id(hass), + ) + ) + _LOGGER.debug("Multiping responses: %s", responses) + await asyncio.gather( + *[ + async_see(dev_id=dev_id, source_type=SOURCE_TYPE_ROUTER) + for idx, dev_id in enumerate(ip_to_dev_id.values()) + if responses[idx].is_alive + ] + ) + + async def _async_update_interval(now): + try: + await async_update(now) + finally: + async_track_point_in_utc_time( + hass, _async_update_interval, util.dt.utcnow() + interval + ) + + await _async_update_interval(None) return True diff --git a/homeassistant/components/ping/manifest.json b/homeassistant/components/ping/manifest.json index bf997d8a4c9..09954787608 100644 --- a/homeassistant/components/ping/manifest.json +++ b/homeassistant/components/ping/manifest.json @@ -3,6 +3,6 @@ "name": "Ping (ICMP)", "documentation": "https://www.home-assistant.io/integrations/ping", "codeowners": [], - "requirements": ["icmplib==2.0.2"], + "requirements": ["icmplib==2.1.1"], "quality_scale": "internal" } diff --git a/requirements_all.txt b/requirements_all.txt index d91f9f31865..6ce5d01effe 100644 --- a/requirements_all.txt +++ b/requirements_all.txt @@ -814,7 +814,7 @@ ibm-watson==4.0.1 ibmiotf==0.3.4 # homeassistant.components.ping -icmplib==2.0.2 +icmplib==2.1.1 # homeassistant.components.iglo iglo==1.2.7 diff --git a/requirements_test_all.txt b/requirements_test_all.txt index 8ba4028fe6b..96bcbcab43d 100644 --- a/requirements_test_all.txt +++ b/requirements_test_all.txt @@ -443,7 +443,7 @@ hyperion-py==0.7.0 iaqualink==0.3.4 # homeassistant.components.ping -icmplib==2.0.2 +icmplib==2.1.1 # homeassistant.components.influxdb influxdb-client==1.14.0