[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 <dev@davidpace.de>
Signed-off-by: Ciprian Pascu <contact@ciprianpascu.ro>

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;

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
+ * The {@link BridgeDiscoveryParticipant} is responsible discovering the Bosch
- * Bosch Smart Home Controller as a Bridge with the mDNS services.
+ * 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<ThingTypeUID> 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 <code>null</code>, no Bosch SHC controller could be
+     * identified at the corresponding IP address.
+     */
+    private ExpiringCacheMap<String, @Nullable PublicInformation> 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.
+     * <p>
+     * Different service info objects can refer to the same Bosch SHC controller,
+     * e.g. the controller may be reachable via a <code>192.168.*.*</code> IP and an
+     * IP in the <code>169.254.*.*</code> range. The response from the controller
+     * contains the actual resolved IP address.
+     * <p>
+     * We ignore mDNS events if they do not contain any IP addresses or if the name
+     * property does not start with <code>Bosch SHC</code>.
+     */
     @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 + ")")
+                .withLabel("Bosch Smart Home Controller (" + publicInformation.shcIpAddress + ")")
-                .withProperty("ipAddress", bridgeInformation.shcIpAddress)
+                .withProperty("ipAddress", publicInformation.shcIpAddress)
-                .withProperty("shcGeneration", bridgeInformation.shcGeneration)
+                .withProperty("shcGeneration", publicInformation.shcGeneration)
-                .withProperty("apiVersions", bridgeInformation.apiVersions).withTTL(TTL_SECONDS).build();
+                .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.
+     * <p>
+     * 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
+     *         <code>null</code> 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;
+        String ipAddress = getFirstIPAddress(serviceInfo);
-        if (!ipAddress.isBlank()) {
+        if (ipAddress != null) {
-            return new ThingUID(BoschSHCBindingConstants.THING_TYPE_SHC, ipAddress.replace('.', '-'));
+            @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) {
+    protected @Nullable PublicInformation discoverBridge(String ipAddress) {
-        logger.trace("Discovering serviceInfo {}", serviceInfo);
+        logger.debug("Attempting to contact Bosch Smart Home Controller at IP {}", ipAddress);
-
+        PublicInformation bridgeInformation = getPublicInformationFromPossibleBridgeAddress(ipAddress);
-        if (serviceInfo.getHostAddresses() != null && serviceInfo.getHostAddresses().length > 0
+        if (bridgeInformation != null && bridgeInformation.shcIpAddress != null
-                && !serviceInfo.getHostAddresses()[0].isEmpty()) {
+                && !bridgeInformation.shcIpAddress.isBlank()) {
-            String address = serviceInfo.getHostAddresses()[0];
+            return bridgeInformation;
-            logger.trace("Discovering InetAddress {}", address);
-            // store all information for later access
-            bridgeInformation = getPublicInformationFromPossibleBridgeAddress(address);
         }
 
-        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 <code>null</code> 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());
+                logger.debug("Discovery failed with status code {}: {}", contentResponse.getStatus(),
-                return new PublicInformation();
+                        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;
     }
 }

bundles/org.openhab.binding.boschshc/src/main/resources/OH-INF/addon/addon.xml

@@ -8,4 +8,22 @@
 	<description>This is the binding for Bosch Smart Home.</description>
 	<connection>local</connection>
 
+	<discovery-methods>
+		<discovery-method>
+			<service-type>mdns</service-type>
+			<discovery-parameters>
+				<discovery-parameter>
+					<name>mdnsServiceType</name>
+					<value>_http._tcp.local.</value>
+				</discovery-parameter>
+			</discovery-parameters>
+			<match-properties>
+				<match-property>
+					<name>name</name>
+					<regex>Bosch SHC.*</regex>
+				</match-property>
+			</match-properties>
+		</discovery-method>
+	</discovery-methods>
+
 </addon:addon>

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 @NonNullByDefault({}) BridgeDiscoveryParticipant fixture;
-    private 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(otherDevice.getHostAddresses()).thenReturn(new String[] { "192.168.0.1" });
+        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;
+        @Nullable
-        assertThat(fixture.discoverBridge(shcBridge).shcIpAddress, is("192.168.0.123"));
+        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("192.168.0.1"), is(nullValue()));
-        assertThat(fixture.discoverBridge(otherDevice).shcIpAddress, is(""));
     }
 
     @Test
     void testGetPublicInformationFromPossibleBridgeAddress() throws Exception {
-        assert fixture != null;
+        @Nullable
-        assertThat(fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123").shcIpAddress,
+        PublicInformation bridgeInformation = fixture.getPublicInformationFromPossibleBridgeAddress("192.168.0.123");
-                is("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);
     }
 }