From: David Pace Date: Mon, 15 Jan 2024 20:09:45 +0000 (+0100) Subject: [boschshc] Cache mDNS-based bridge discovery results (#16211) X-Git-Url: https://git.basschouten.com/?a=commitdiff_plain;h=777def6ed233ab47050ed3d86b2847750c9fbbc1;p=openhab-addons.git [boschshc] Cache mDNS-based bridge discovery results (#16211) * [boschshc] Cache mDNS-based bridge discovery results The bridge discovery participant receives lots of mDNS events. Previously, all events that contained IP addresses of potential bridges were actively contacted using HTTP requests. On some systems eventually the long polling stops due to too many requests. With this change, we * only consider mDNS events where the name property starts with "Bosch SHC" * cache already discovered bridges so we don't have to contact them over and over again * make sure that this happens in a thread-safe manner because the mDNS events are handled in individual concurrently running threads Signed-off-by: David Pace --- diff --git a/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BoschHttpClient.java b/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BoschHttpClient.java index efe146dba1..945b3889c1 100644 --- a/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BoschHttpClient.java +++ b/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/devices/bridge/BoschHttpClient.java @@ -56,6 +56,16 @@ public class BoschHttpClient extends HttpClient { private final Logger logger = LoggerFactory.getLogger(getClass()); + /** + * Default number of seconds for HTTP request timeouts + */ + public static final long DEFAULT_TIMEOUT_SECONDS = 10; + + /** + * The time unit used for default HTTP request timeouts + */ + public static final TimeUnit DEFAULT_TIMEOUT_UNIT = TimeUnit.SECONDS; + private final String ipAddress; private final String systemPassword; @@ -299,7 +309,7 @@ public class BoschHttpClient extends HttpClient { Request request = this.newRequest(url).method(method).header("Content-Type", "application/json") .header("api-version", "3.2") // see https://github.com/BoschSmartHome/bosch-shc-api-docs/issues/80 - .timeout(10, TimeUnit.SECONDS); // Set default timeout + .timeout(DEFAULT_TIMEOUT_SECONDS, DEFAULT_TIMEOUT_UNIT); // Set default timeout if (content != null) { final String body; diff --git a/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipant.java b/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipant.java index 5ef163a304..fd7d5f8d4b 100644 --- a/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipant.java +++ b/bundles/org.openhab.binding.boschshc/src/main/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipant.java @@ -31,6 +31,7 @@ import org.eclipse.jetty.util.ssl.SslContextFactory; import org.openhab.binding.boschshc.internal.devices.BoschSHCBindingConstants; import org.openhab.binding.boschshc.internal.devices.bridge.BoschHttpClient; import org.openhab.binding.boschshc.internal.devices.bridge.dto.PublicInformation; +import org.openhab.core.cache.ExpiringCacheMap; import org.openhab.core.config.discovery.DiscoveryResult; import org.openhab.core.config.discovery.DiscoveryResultBuilder; import org.openhab.core.config.discovery.mdns.MDNSDiscoveryParticipant; @@ -46,23 +47,32 @@ import org.slf4j.LoggerFactory; import com.google.gson.Gson; /** - * The {@link BridgeDiscoveryParticipant} is responsible discovering the - * Bosch Smart Home Controller as a Bridge with the mDNS services. + * The {@link BridgeDiscoveryParticipant} is responsible discovering the Bosch + * Smart Home Controller as a Bridge with the mDNS services. * * @author Gerd Zanker - Initial contribution + * @author David Pace - Discovery result caching */ @NonNullByDefault @Component(configurationPid = "discovery.boschsmarthomebridge") public class BridgeDiscoveryParticipant implements MDNSDiscoveryParticipant { - private static final long TTL_SECONDS = Duration.ofHours(1).toSeconds(); + private static final String NAME_PREFIX_BOSCH_SHC = "Bosch SHC"; + private static final Duration TTL_DURATION = Duration.ofMinutes(10); + private static final long TTL_SECONDS = TTL_DURATION.toSeconds(); + public static final Set SUPPORTED_THING_TYPES_UIDS = Set.of(BoschSHCBindingConstants.THING_TYPE_SHC); private final Logger logger = LoggerFactory.getLogger(BridgeDiscoveryParticipant.class); private final HttpClient httpClient; private final Gson gson = new Gson(); - /// SHC Bridge Information, read via public REST API if bridge is detected. Otherwise, strings are empty. - private PublicInformation bridgeInformation = new PublicInformation(); + /** + * Cache for bridge discovery results. Uses the IP address of mDNS events as + * key. If the value is null, no Bosch SHC controller could be + * identified at the corresponding IP address. + */ + private ExpiringCacheMap discoveryResultCache = new ExpiringCacheMap<>( + TTL_DURATION); @Activate public BridgeDiscoveryParticipant(@Reference HttpClientFactory httpClientFactory) { @@ -89,9 +99,43 @@ public class BridgeDiscoveryParticipant implements MDNSDiscoveryParticipant { return "_http._tcp.local."; } + /** + * This method is frequently called by the mDNS discovery framework in different + * threads with individual service info instances. + *

+ * Different service info objects can refer to the same Bosch SHC controller, + * e.g. the controller may be reachable via a 192.168.*.* IP and an + * IP in the 169.254.*.* range. The response from the controller + * contains the actual resolved IP address. + *

+ * We ignore mDNS events if they do not contain any IP addresses or if the name + * property does not start with Bosch SHC. + */ @Override public @Nullable DiscoveryResult createResult(ServiceInfo serviceInfo) { - logger.trace("Bridge Discovery started for {}", serviceInfo); + if (logger.isTraceEnabled()) { + logger.trace("Bridge discovery invoked with mDNS service info {}", serviceInfo); + } + + String name = serviceInfo.getName(); + if (name == null || !name.startsWith(NAME_PREFIX_BOSCH_SHC)) { + if (logger.isTraceEnabled()) { + logger.trace("Ignoring mDNS service event because name '{}' does not start with '{}')", name, + NAME_PREFIX_BOSCH_SHC); + } + return null; + } + + @Nullable + String ipAddress = getFirstIPAddress(serviceInfo); + if (ipAddress == null || ipAddress.isBlank()) { + return null; + } + + PublicInformation publicInformation = getOrComputePublicInformation(ipAddress); + if (publicInformation == null) { + return null; + } @Nullable final ThingUID uid = getThingUID(serviceInfo); @@ -99,63 +143,103 @@ public class BridgeDiscoveryParticipant implements MDNSDiscoveryParticipant { return null; } - logger.trace("Discovered Bosch Smart Home Controller at {}", bridgeInformation.shcIpAddress); - return DiscoveryResultBuilder.create(uid) - .withLabel("Bosch Smart Home Controller (" + bridgeInformation.shcIpAddress + ")") - .withProperty("ipAddress", bridgeInformation.shcIpAddress) - .withProperty("shcGeneration", bridgeInformation.shcGeneration) - .withProperty("apiVersions", bridgeInformation.apiVersions).withTTL(TTL_SECONDS).build(); + .withLabel("Bosch Smart Home Controller (" + publicInformation.shcIpAddress + ")") + .withProperty("ipAddress", publicInformation.shcIpAddress) + .withProperty("shcGeneration", publicInformation.shcGeneration) + .withProperty("apiVersions", publicInformation.apiVersions).withTTL(TTL_SECONDS).build(); + } + + private @Nullable String getFirstIPAddress(ServiceInfo serviceInfo) { + String[] hostAddresses = serviceInfo.getHostAddresses(); + if (hostAddresses != null && hostAddresses.length > 0 && !hostAddresses[0].isEmpty()) { + return hostAddresses[0]; + } + + return null; + } + + /** + * Provides a cached discovery result if available, or performs an actual + * communication attempt to the device with the given IP address. + *

+ * This method is synchronized because multiple threads try to access discovery + * results concurrently. We only want one thread to "win" and to invoke the + * actual HTTP communication. + * + * @param ipAddress IP address to contact if no cached result is available + * @return the {@link PublicInformation} of the Bosch Smart Home Controller or + * null if the device with the given IP address could not + * be identified as Bosch Smart Home Controller + */ + protected synchronized @Nullable PublicInformation getOrComputePublicInformation(String ipAddress) { + return discoveryResultCache.putIfAbsentAndGet(ipAddress, () -> { + logger.trace("No cached bridge discovery result available for IP {}, trying to contact SHC", ipAddress); + return discoverBridge(ipAddress); + }); } @Override public @Nullable ThingUID getThingUID(ServiceInfo serviceInfo) { - String ipAddress = discoverBridge(serviceInfo).shcIpAddress; - if (!ipAddress.isBlank()) { - return new ThingUID(BoschSHCBindingConstants.THING_TYPE_SHC, ipAddress.replace('.', '-')); + String ipAddress = getFirstIPAddress(serviceInfo); + if (ipAddress != null) { + @Nullable + PublicInformation publicInformation = getOrComputePublicInformation(ipAddress); + if (publicInformation != null) { + String resolvedIpAddress = publicInformation.shcIpAddress; + return new ThingUID(BoschSHCBindingConstants.THING_TYPE_SHC, resolvedIpAddress.replace('.', '-')); + } } return null; } - protected PublicInformation discoverBridge(ServiceInfo serviceInfo) { - logger.trace("Discovering serviceInfo {}", serviceInfo); - - if (serviceInfo.getHostAddresses() != null && serviceInfo.getHostAddresses().length > 0 - && !serviceInfo.getHostAddresses()[0].isEmpty()) { - String address = serviceInfo.getHostAddresses()[0]; - logger.trace("Discovering InetAddress {}", address); - // store all information for later access - bridgeInformation = getPublicInformationFromPossibleBridgeAddress(address); + protected @Nullable PublicInformation discoverBridge(String ipAddress) { + logger.debug("Attempting to contact Bosch Smart Home Controller at IP {}", ipAddress); + PublicInformation bridgeInformation = getPublicInformationFromPossibleBridgeAddress(ipAddress); + if (bridgeInformation != null && bridgeInformation.shcIpAddress != null + && !bridgeInformation.shcIpAddress.isBlank()) { + return bridgeInformation; } - return bridgeInformation; + return null; } - protected PublicInformation getPublicInformationFromPossibleBridgeAddress(String ipAddress) { + /** + * Attempts to send a HTTP request to the given IP address in order to determine + * if the device is a Bosch Smart Home Controller. + * + * @param ipAddress the IP address of the potential Bosch Smart Home Controller + * @return a {@link PublicInformation} object if the bridge was successfully + * contacted or null if the communication failed + */ + protected @Nullable PublicInformation getPublicInformationFromPossibleBridgeAddress(String ipAddress) { String url = BoschHttpClient.getPublicInformationUrl(ipAddress); - logger.trace("Discovering ipAddress {}", url); + logger.trace("Requesting SHC information via URL {}", url); try { httpClient.start(); - ContentResponse contentResponse = httpClient.newRequest(url).method(HttpMethod.GET).send(); + ContentResponse contentResponse = httpClient.newRequest(url).method(HttpMethod.GET) + .timeout(BoschHttpClient.DEFAULT_TIMEOUT_SECONDS, BoschHttpClient.DEFAULT_TIMEOUT_UNIT).send(); + // check HTTP status code if (!HttpStatus.getCode(contentResponse.getStatus()).isSuccess()) { - logger.debug("Discovering failed with status code: {}", contentResponse.getStatus()); - return new PublicInformation(); + logger.debug("Discovery failed with status code {}: {}", contentResponse.getStatus(), + contentResponse.getContentAsString()); + return null; } // get content from response String content = contentResponse.getContentAsString(); - logger.trace("Discovered SHC - public info {}", content); + logger.debug("Discovered SHC at IP {}, public info: {}", ipAddress, content); PublicInformation bridgeInfo = gson.fromJson(content, PublicInformation.class); - if (bridgeInfo.shcIpAddress != null) { + if (bridgeInfo != null && bridgeInfo.shcIpAddress != null && !bridgeInfo.shcIpAddress.isBlank()) { return bridgeInfo; } } catch (TimeoutException | ExecutionException e) { - logger.debug("Discovering failed with exception {}", e.getMessage()); + logger.debug("Discovery could not reach SHC at IP {}: {}", ipAddress, e.getMessage()); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } catch (Exception e) { - logger.debug("Discovering failed during http client request {}", e.getMessage()); + logger.warn("Discovery failed during HTTP client request: {}", e.getMessage(), e); } - return new PublicInformation(); + return null; } } diff --git a/bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/addon/addon.xml b/bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/addon/addon.xml index b32da8ee02..cfaa69b1bf 100644 --- a/bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/addon/addon.xml +++ b/bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/addon/addon.xml @@ -8,4 +8,22 @@ This is the binding for Bosch Smart Home. local + + + mdns + + + mdnsServiceType + _http._tcp.local. + + + + + name + Bosch SHC.* + + + + + diff --git a/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipantTest.java b/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipantTest.java index 6e7552db04..c98d24142f 100644 --- a/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipantTest.java +++ b/bundles/org.openhab.binding.boschshc/src/test/java/org/openhab/binding/boschshc/internal/discovery/BridgeDiscoveryParticipantTest.java @@ -13,10 +13,20 @@ package org.openhab.binding.boschshc.internal.discovery; import static org.hamcrest.CoreMatchers.is; +import static org.hamcrest.CoreMatchers.not; +import static org.hamcrest.CoreMatchers.nullValue; import static org.hamcrest.MatcherAssert.assertThat; -import static org.junit.jupiter.api.Assertions.*; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.Mockito.*; +import static org.mockito.ArgumentMatchers.anyLong; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.net.ConnectException; +import java.util.concurrent.ExecutionException; import javax.jmdns.ServiceInfo; @@ -31,10 +41,12 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; import org.mockito.Mock; +import org.mockito.Spy; import org.mockito.junit.jupiter.MockitoExtension; import org.mockito.junit.jupiter.MockitoSettings; import org.mockito.quality.Strictness; import org.openhab.binding.boschshc.internal.devices.BoschSHCBindingConstants; +import org.openhab.binding.boschshc.internal.devices.bridge.dto.PublicInformation; import org.openhab.core.config.discovery.DiscoveryResult; import org.openhab.core.thing.ThingUID; @@ -48,31 +60,47 @@ import org.openhab.core.thing.ThingUID; @NonNullByDefault class BridgeDiscoveryParticipantTest { - @Nullable - private BridgeDiscoveryParticipant fixture; + private @NonNullByDefault({}) BridgeDiscoveryParticipant fixture; private final String url = "https://192.168.0.123:8446/smarthome/public/information"; + private final String urlOtherDevice = "https://192.168.0.1:8446/smarthome/public/information"; private @Mock @NonNullByDefault({}) ServiceInfo shcBridge; private @Mock @NonNullByDefault({}) ServiceInfo otherDevice; + private @Mock @NonNullByDefault({}) ContentResponse contentResponse; + + /** + * Spy needed because some final methods can't be mocked + */ + private @Spy @NonNullByDefault({}) HttpClient mockHttpClient; + @BeforeEach public void beforeEach() throws Exception { when(shcBridge.getHostAddresses()).thenReturn(new String[] { "192.168.0.123" }); + when(shcBridge.getName()).thenReturn("Bosch SHC [xx-xx-xx-xx-xx-xx]"); + when(otherDevice.getHostAddresses()).thenReturn(new String[] { "192.168.0.1" }); + when(otherDevice.getName()).thenReturn("Other Device"); - ContentResponse contentResponse = mock(ContentResponse.class); when(contentResponse.getContentAsString()).thenReturn( "{\"apiVersions\":[\"2.9\",\"3.2\"], \"shcIpAddress\":\"192.168.0.123\", \"shcGeneration\":\"SHC_1\"}"); when(contentResponse.getStatus()).thenReturn(HttpStatus.OK_200); Request mockRequest = mock(Request.class); when(mockRequest.send()).thenReturn(contentResponse); - when(mockRequest.method((HttpMethod) any())).thenReturn(mockRequest); + when(mockRequest.method(any(HttpMethod.class))).thenReturn(mockRequest); + when(mockRequest.timeout(anyLong(), any())).thenReturn(mockRequest); - HttpClient mockHttpClient = spy(HttpClient.class); // spy needed, because some final methods can't be mocked when(mockHttpClient.newRequest(url)).thenReturn(mockRequest); + Request failingRequest = mock(Request.class); + when(failingRequest.method(any(HttpMethod.class))).thenReturn(failingRequest); + when(failingRequest.timeout(anyLong(), any())).thenReturn(failingRequest); + when(failingRequest.send()).thenThrow(new ExecutionException(new ConnectException("Connection refused"))); + + when(mockHttpClient.newRequest(urlOtherDevice)).thenReturn(failingRequest); + fixture = new BridgeDiscoveryParticipant(mockHttpClient); } @@ -84,7 +112,6 @@ class BridgeDiscoveryParticipantTest { @Test void testGetSupportedThingTypeUIDs() { - assert fixture != null; assertTrue(fixture.getSupportedThingTypeUIDs().contains(BoschSHCBindingConstants.THING_TYPE_SHC)); } @@ -95,13 +122,11 @@ class BridgeDiscoveryParticipantTest { */ @Test void testGetServiceType() throws Exception { - assert fixture != null; assertThat(fixture.getServiceType(), is("_http._tcp.local.")); } @Test void testCreateResult() throws Exception { - assert fixture != null; DiscoveryResult result = fixture.createResult(shcBridge); assertNotNull(result); assertThat(result.getBindingId(), is(BoschSHCBindingConstants.BINDING_ID)); @@ -112,14 +137,19 @@ class BridgeDiscoveryParticipantTest { @Test void testCreateResultOtherDevice() throws Exception { - assert fixture != null; DiscoveryResult result = fixture.createResult(otherDevice); assertNull(result); } + @Test + void testCreateResultNoIPAddress() throws Exception { + when(shcBridge.getHostAddresses()).thenReturn(new String[] { "" }); + DiscoveryResult result = fixture.createResult(shcBridge); + assertNull(result); + } + @Test void testGetThingUID() throws Exception { - assert fixture != null; ThingUID thingUID = fixture.getThingUID(shcBridge); assertNotNull(thingUID); assertThat(thingUID.getBindingId(), is(BoschSHCBindingConstants.BINDING_ID)); @@ -128,64 +158,53 @@ class BridgeDiscoveryParticipantTest { @Test void testGetThingUIDOtherDevice() throws Exception { - assert fixture != null; assertNull(fixture.getThingUID(otherDevice)); } @Test void testGetBridgeAddress() throws Exception { - assert fixture != null; - assertThat(fixture.discoverBridge(shcBridge).shcIpAddress, is("192.168.0.123")); + @Nullable + PublicInformation bridgeInformation = fixture.discoverBridge("192.168.0.123"); + assertThat(bridgeInformation, not(nullValue())); + assertThat(bridgeInformation.shcIpAddress, is("192.168.0.123")); } @Test void testGetBridgeAddressOtherDevice() throws Exception { - assert fixture != null; - assertThat(fixture.discoverBridge(otherDevice).shcIpAddress, is("")); + assertThat(fixture.discoverBridge("192.168.0.1"), is(nullValue())); } @Test void testGetPublicInformationFromPossibleBridgeAddress() throws Exception { - assert fixture != null; - assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123").shcIpAddress, - is("192.168.0.123")); + @Nullable + PublicInformation bridgeInformation = fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123"); + assertThat(bridgeInformation, not(nullValue())); + assertThat(bridgeInformation.shcIpAddress, is("192.168.0.123")); } @Test void testGetPublicInformationFromPossibleBridgeAddressInvalidContent() throws Exception { - assert fixture != null; - - ContentResponse contentResponse = mock(ContentResponse.class); when(contentResponse.getContentAsString()).thenReturn("{\"nothing\":\"useful\"}"); - when(contentResponse.getStatus()).thenReturn(HttpStatus.OK_200); - - Request mockRequest = mock(Request.class); - when(mockRequest.send()).thenReturn(contentResponse); - when(mockRequest.method((HttpMethod) any())).thenReturn(mockRequest); - - HttpClient mockHttpClient = spy(HttpClient.class); // spy needed, because some final methods can't be mocked - when(mockHttpClient.newRequest(url)).thenReturn(mockRequest); fixture = new BridgeDiscoveryParticipant(mockHttpClient); - assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("shcAddress").shcIpAddress, is("")); + assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123"), is(nullValue())); } @Test void testGetPublicInformationFromPossibleBridgeAddressInvalidStatus() throws Exception { - assert fixture != null; - - ContentResponse contentResponse = mock(ContentResponse.class); - // when(contentResponse.getContentAsString()).thenReturn("{\"nothing\":\"useful\"}"); no content needed when(contentResponse.getStatus()).thenReturn(HttpStatus.BAD_REQUEST_400); - Request mockRequest = mock(Request.class); - when(mockRequest.send()).thenReturn(contentResponse); - when(mockRequest.method((HttpMethod) any())).thenReturn(mockRequest); - - HttpClient mockHttpClient = spy(HttpClient.class); // spy needed, because some final methods can't be mocked - when(mockHttpClient.newRequest(url)).thenReturn(mockRequest); - fixture = new BridgeDiscoveryParticipant(mockHttpClient); - assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("shcAddress").shcIpAddress, is("")); + assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123"), is(nullValue())); + } + + @Test + void testGetOrComputePublicInformation() throws Exception { + @Nullable + PublicInformation result = fixture.getOrComputePublicInformation("192.168.0.123"); + assertNotNull(result); + @Nullable + PublicInformation result2 = fixture.getOrComputePublicInformation("192.168.0.123"); + assertSame(result, result2); } }