From 82a378ad5b1a3dc27e8d98cb1e4f6c05dec61ad8 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Tue, 24 Dec 2024 13:42:24 +0100 Subject: [PATCH 01/35] Fix performance Signed-off-by: Leo Siepel --- .../openhab/binding/network/internal/utils/NetworkUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index c9fe6b1a8cdca..e1aef4399cf87 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -151,7 +151,7 @@ public Set getInterfaceNames() { try { for (Enumeration en = NetworkInterface.getNetworkInterfaces(); en.hasMoreElements();) { NetworkInterface networkInterface = en.nextElement(); - if (!networkInterface.isLoopback()) { + if (networkInterface.isUp() && !networkInterface.isLoopback()) { result.add(networkInterface.getName()); } } From 20803465f28fb57e5cfc4331e6f4487bb823e19a Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Wed, 1 Jan 2025 23:26:22 +0100 Subject: [PATCH 02/35] Improvements Signed-off-by: Leo Siepel --- .../discovery/NetworkDiscoveryService.java | 68 +++++++++++-------- .../network/internal/utils/NetworkUtils.java | 28 +++++++- 2 files changed, 65 insertions(+), 31 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index 483689dfdb273..c1cb862a3e94b 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -19,6 +19,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; @@ -37,6 +38,7 @@ import org.openhab.core.config.discovery.AbstractDiscoveryService; import org.openhab.core.config.discovery.DiscoveryResultBuilder; import org.openhab.core.config.discovery.DiscoveryService; +import org.openhab.core.net.CidrAddress; import org.openhab.core.thing.ThingUID; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; @@ -133,37 +135,43 @@ protected void startScan() { return; } removeOlderResults(getTimestampOfLastScan(), null); - logger.trace("Starting Network Device Discovery"); - - final Set networkIPs = networkUtils.getNetworkIPs(MAXIMUM_IPS_PER_INTERFACE); - scannedIPcount.set(0); - - for (String ip : networkIPs) { - final PresenceDetection pd = new PresenceDetection(this, scheduler, Duration.ofSeconds(2)); - pd.setHostname(ip); - pd.setIOSDevice(true); - pd.setUseDhcpSniffing(false); - pd.setTimeout(PING_TIMEOUT); - // Ping devices - pd.setUseIcmpPing(true); - pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); - // TCP devices - pd.setServicePorts(tcpServicePorts); - - service.execute(() -> { - Thread.currentThread().setName("Discovery thread " + ip); - try { - pd.getValue(); - } catch (ExecutionException | InterruptedException e) { - stopScan(); - } - int count = scannedIPcount.incrementAndGet(); - if (count == networkIPs.size()) { - logger.trace("Scan of {} IPs successful", scannedIPcount); - stopScan(); - } - }); + logger.debug("Starting Network Device Discovery"); + Map> discoveryList = networkUtils.getNetworkIPsPerInterface(); + + for (String networkInterface : discoveryList.keySet()) { + final Set networkIPs = networkUtils.getNetworkIPs( + Objects.requireNonNull(discoveryList.get(networkInterface)), MAXIMUM_IPS_PER_INTERFACE); + logger.debug("Scanning {} IPs on interface {} ", networkIPs.size(), networkInterface); + scannedIPcount.set(0); + for (String ip : networkIPs) { + final PresenceDetection pd = new PresenceDetection(this, scheduler, Duration.ofSeconds(2)); + pd.setHostname(ip); + pd.setNetworkInterfaceNames(Set.of(networkInterface)); + pd.setIOSDevice(true); + pd.setUseDhcpSniffing(false); + pd.setTimeout(PING_TIMEOUT); + // Ping devices + pd.setUseIcmpPing(true); + pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); + // TCP devices + pd.setServicePorts(tcpServicePorts); + + service.execute(() -> { + Thread.currentThread().setName("Discovery thread " + ip); + try { + pd.getValue(); + } catch (ExecutionException | InterruptedException e) { + stopScan(); + } + int count = scannedIPcount.incrementAndGet(); + if (count == networkIPs.size()) { + logger.trace("Scan of {} IPs on interface {} successful", scannedIPcount, networkInterface); + stopScan(); + } + }); + } } + logger.debug("Finished Network Device Discovery"); } @Override diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index e1aef4399cf87..76750460a03ad 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -32,9 +32,11 @@ import java.time.Instant; import java.util.ArrayList; import java.util.Enumeration; +import java.util.HashMap; import java.util.HashSet; import java.util.LinkedHashSet; import java.util.List; +import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -172,6 +174,30 @@ public Set getNetworkIPs(int maximumPerInterface) { return getNetworkIPs(getInterfaceIPs(), maximumPerInterface); } + /** + * Retrieves a map of network interface names to their associated IP addresses. + * + * @return A map where the key is the name of the network interface and the value is a set of CidrAddress objects + * representing the IP addresses and network prefix lengths for that interface. + */ + public Map> getNetworkIPsPerInterface() { + Map> outputMap = new HashMap<>(); + try { + for (Enumeration en = NetworkInterface.getNetworkInterfaces(); en.hasMoreElements();) { + NetworkInterface networkInterface = en.nextElement(); + if (networkInterface.isUp() && !networkInterface.isLoopback()) { + outputMap.put(networkInterface.getName(), + networkInterface.getInterfaceAddresses().stream() + .map(m -> new CidrAddress(m.getAddress(), m.getNetworkPrefixLength())) + .collect(Collectors.toSet())); + } + } + } catch (SocketException e) { + logger.trace("Could not get network interfaces", e); + } + return outputMap; + } + /** * Takes the interfaceIPs and fetches every IP which can be assigned on their network * @@ -179,7 +205,7 @@ public Set getNetworkIPs(int maximumPerInterface) { * @param maximumPerInterface The maximum of IP addresses per interface or 0 to get all. * @return Every single IP which can be assigned on the Networks the computer is connected to */ - private Set getNetworkIPs(Set interfaceIPs, int maximumPerInterface) { + public Set getNetworkIPs(Set interfaceIPs, int maximumPerInterface) { Set networkIPs = new LinkedHashSet<>(); short minCidrPrefixLength = 8; // historic Class A network, addresses = 16777214 From dcce737b981f946b6f3656d6dc85063e5dd2d72f Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Fri, 30 May 2025 14:29:51 +0200 Subject: [PATCH 03/35] Add logging Signed-off-by: Leo Siepel --- .../openhab/binding/network/internal/utils/NetworkUtils.java | 3 +++ 1 file changed, 3 insertions(+) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index 76750460a03ad..4f288bddef312 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -186,10 +186,13 @@ public Map> getNetworkIPsPerInterface() { for (Enumeration en = NetworkInterface.getNetworkInterfaces(); en.hasMoreElements();) { NetworkInterface networkInterface = en.nextElement(); if (networkInterface.isUp() && !networkInterface.isLoopback()) { + logger.trace("Network interface: {} is included in the", networkInterface.getName()); outputMap.put(networkInterface.getName(), networkInterface.getInterfaceAddresses().stream() .map(m -> new CidrAddress(m.getAddress(), m.getNetworkPrefixLength())) .collect(Collectors.toSet())); + } else { + logger.trace("Network interface: {} is excluded in the search", networkInterface.getName()); } } } catch (SocketException e) { From 0f6dfd18f97737fdf3f082f5d71e308d9f361407 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Fri, 30 May 2025 14:52:05 +0200 Subject: [PATCH 04/35] Improve logging Signed-off-by: Leo Siepel --- .../openhab/binding/network/internal/utils/NetworkUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index 4f288bddef312..a1d8085cd0612 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -261,7 +261,7 @@ public PingResult servicePing(String host, int port, Duration timeout) throws IO socket.connect(new InetSocketAddress(host, port), (int) timeout.toMillis()); success = true; } catch (ConnectException | SocketTimeoutException | NoRouteToHostException e) { - logger.trace("Could not connect to {}:{}", host, port, e); + logger.trace("Could not connect to {}:{} {}", host, port, e.getMessage()); } return new PingResult(success, Duration.between(execStartTime, Instant.now())); } From 9618fb6e28104220bbdfdc49267e7ece7d66e3be Mon Sep 17 00:00:00 2001 From: lsiepel Date: Fri, 30 May 2025 20:38:07 +0200 Subject: [PATCH 05/35] Update bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java Co-authored-by: Wouter Born Signed-off-by: lsiepel --- .../openhab/binding/network/internal/utils/NetworkUtils.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index a1d8085cd0612..36138153efa4c 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -186,7 +186,7 @@ public Map> getNetworkIPsPerInterface() { for (Enumeration en = NetworkInterface.getNetworkInterfaces(); en.hasMoreElements();) { NetworkInterface networkInterface = en.nextElement(); if (networkInterface.isUp() && !networkInterface.isLoopback()) { - logger.trace("Network interface: {} is included in the", networkInterface.getName()); + logger.trace("Network interface: {} is included in the search", networkInterface.getName()); outputMap.put(networkInterface.getName(), networkInterface.getInterfaceAddresses().stream() .map(m -> new CidrAddress(m.getAddress(), m.getNetworkPrefixLength())) From ae384b03a7657684119686dd646376db55bc0248 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Thu, 28 Aug 2025 22:15:08 +0200 Subject: [PATCH 06/35] Improve thread handling Signed-off-by: Leo Siepel --- .../discovery/NetworkDiscoveryService.java | 26 +++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index c1cb862a3e94b..6df193692f5a2 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -23,7 +23,9 @@ import java.util.Set; import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; +import java.util.concurrent.LinkedBlockingQueue; +import java.util.concurrent.ThreadFactory; +import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -122,13 +124,33 @@ public void partialDetectionResult(PresenceDetectionValue value) { public void finalDetectionResult(PresenceDetectionValue value) { } + private ExecutorService createDiscoveryExecutor() { + int cores = Runtime.getRuntime().availableProcessors(); + + return new ThreadPoolExecutor(cores * 2, // core pool size + cores * 8, // max pool size for bursts + 60L, TimeUnit.SECONDS, // keep-alive for idle threads + new LinkedBlockingQueue<>(cores * 50), // bounded queue (e.g. 400 items if 8 cores) + new ThreadFactory() { + private final AtomicInteger count = new AtomicInteger(1); + + @Override + public Thread newThread(@Nullable Runnable r) { + Thread t = new Thread(r, "NetworkDiscovery-" + count.getAndIncrement()); + t.setDaemon(true); + return t; + } + }, new ThreadPoolExecutor.CallerRunsPolicy() // backpressure when saturated + ); + } + /** * Starts the DiscoveryThread for each IP on each interface on the network */ @Override protected void startScan() { if (executorService == null) { - executorService = Executors.newFixedThreadPool(Runtime.getRuntime().availableProcessors() * 2); + executorService = createDiscoveryExecutor(); } final ExecutorService service = executorService; if (service == null) { From ce746290d106a2e013317a0d500520105146d323 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Thu, 28 Aug 2025 22:18:57 +0200 Subject: [PATCH 07/35] Improve shutdown Signed-off-by: Leo Siepel --- .../internal/discovery/NetworkDiscoveryService.java | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index 6df193692f5a2..d44800b343f98 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -204,12 +204,15 @@ protected synchronized void stopScan() { return; } + service.shutdown(); // initiate shutdown try { - service.awaitTermination(PING_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS); + if (!service.awaitTermination(PING_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS)) { + service.shutdownNow(); // force shutdown if still busy + } } catch (InterruptedException e) { - Thread.currentThread().interrupt(); // Reset interrupt flag + service.shutdownNow(); + Thread.currentThread().interrupt(); } - service.shutdown(); executorService = null; } From 72cd118e9737189e651c3e6f41ddb3850b338a23 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Thu, 28 Aug 2025 22:22:19 +0200 Subject: [PATCH 08/35] thread cleanup Signed-off-by: Leo Siepel --- .../discovery/NetworkDiscoveryService.java | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index d44800b343f98..fafa0c80f0009 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -24,7 +24,6 @@ import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.LinkedBlockingQueue; -import java.util.concurrent.ThreadFactory; import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; @@ -102,7 +101,7 @@ protected void modified(@Nullable Map config) { @Deactivate protected void deactivate() { if (executorService != null) { - executorService.shutdown(); + executorService.shutdownNow(); } super.deactivate(); } @@ -126,20 +125,16 @@ public void finalDetectionResult(PresenceDetectionValue value) { private ExecutorService createDiscoveryExecutor() { int cores = Runtime.getRuntime().availableProcessors(); + AtomicInteger count = new AtomicInteger(1); return new ThreadPoolExecutor(cores * 2, // core pool size cores * 8, // max pool size for bursts 60L, TimeUnit.SECONDS, // keep-alive for idle threads - new LinkedBlockingQueue<>(cores * 50), // bounded queue (e.g. 400 items if 8 cores) - new ThreadFactory() { - private final AtomicInteger count = new AtomicInteger(1); - - @Override - public Thread newThread(@Nullable Runnable r) { - Thread t = new Thread(r, "NetworkDiscovery-" + count.getAndIncrement()); - t.setDaemon(true); - return t; - } + new LinkedBlockingQueue<>(cores * 50), // bounded queue + r -> { + Thread t = new Thread(r, "NetworkDiscovery-" + count.getAndIncrement()); + t.setDaemon(true); + return t; }, new ThreadPoolExecutor.CallerRunsPolicy() // backpressure when saturated ); } @@ -179,7 +174,6 @@ protected void startScan() { pd.setServicePorts(tcpServicePorts); service.execute(() -> { - Thread.currentThread().setName("Discovery thread " + ip); try { pd.getValue(); } catch (ExecutionException | InterruptedException e) { From a07516d99baefcf68da27341fd8ac1661dc1b919 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Thu, 28 Aug 2025 22:43:57 +0200 Subject: [PATCH 09/35] Improve per thread allocation Signed-off-by: Leo Siepel --- .../discovery/NetworkDiscoveryService.java | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index fafa0c80f0009..ab0c6401bb6f3 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -16,7 +16,6 @@ import static org.openhab.binding.network.internal.utils.NetworkUtils.durationToMillis; import java.time.Duration; -import java.util.HashMap; import java.util.List; import java.util.Map; import java.util.Objects; @@ -151,8 +150,10 @@ protected void startScan() { if (service == null) { return; } + removeOlderResults(getTimestampOfLastScan(), null); logger.debug("Starting Network Device Discovery"); + Map> discoveryList = networkUtils.getNetworkIPsPerInterface(); for (String networkInterface : discoveryList.keySet()) { @@ -160,20 +161,21 @@ protected void startScan() { Objects.requireNonNull(discoveryList.get(networkInterface)), MAXIMUM_IPS_PER_INTERFACE); logger.debug("Scanning {} IPs on interface {} ", networkIPs.size(), networkInterface); scannedIPcount.set(0); - for (String ip : networkIPs) { - final PresenceDetection pd = new PresenceDetection(this, scheduler, Duration.ofSeconds(2)); - pd.setHostname(ip); - pd.setNetworkInterfaceNames(Set.of(networkInterface)); - pd.setIOSDevice(true); - pd.setUseDhcpSniffing(false); - pd.setTimeout(PING_TIMEOUT); - // Ping devices - pd.setUseIcmpPing(true); - pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); - // TCP devices - pd.setServicePorts(tcpServicePorts); + for (String ip : networkIPs) { service.execute(() -> { + PresenceDetection pd = presenceDetectorThreadLocal.get(); + + // Reset per-IP fields + pd.setHostname(ip); + pd.setNetworkInterfaceNames(Set.of(networkInterface)); + pd.setIOSDevice(true); + pd.setUseDhcpSniffing(false); + pd.setTimeout(PING_TIMEOUT); + pd.setUseIcmpPing(true); + pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); + pd.setServicePorts(tcpServicePorts); + try { pd.getValue(); } catch (ExecutionException | InterruptedException e) { @@ -190,6 +192,9 @@ protected void startScan() { logger.debug("Finished Network Device Discovery"); } + private final ThreadLocal presenceDetectorThreadLocal = ThreadLocal + .withInitial(() -> new PresenceDetection(this, scheduler, Duration.ofSeconds(2))); + @Override protected synchronized void stopScan() { super.stopScan(); @@ -236,11 +241,8 @@ public void newServiceDevice(String ip, int tcpPort) { }; label += " (" + ip + ":" + tcpPort + ")"; - Map properties = new HashMap<>(); - properties.put(PARAMETER_HOSTNAME, ip); - properties.put(PARAMETER_PORT, tcpPort); thingDiscovered(DiscoveryResultBuilder.create(createServiceUID(ip, tcpPort)).withTTL(DISCOVERY_RESULT_TTL) - .withProperties(properties).withLabel(label).build()); + .withProperty(PARAMETER_HOSTNAME, ip).withProperty(PARAMETER_PORT, tcpPort).withLabel(label).build()); } public static ThingUID createPingUID(String ip) { @@ -256,8 +258,7 @@ public static ThingUID createPingUID(String ip) { public void newPingDevice(String ip) { logger.trace("Found pingable network device with IP address {}", ip); - Map properties = Map.of(PARAMETER_HOSTNAME, ip); thingDiscovered(DiscoveryResultBuilder.create(createPingUID(ip)).withTTL(DISCOVERY_RESULT_TTL) - .withProperties(properties).withLabel("Network Device (" + ip + ")").build()); + .withProperty(PARAMETER_HOSTNAME, ip).withLabel("Network Device (" + ip + ")").build()); } } From 8cd2ceb5bcc22ced31503e6762ad8591cbb7165d Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Thu, 28 Aug 2025 22:55:59 +0200 Subject: [PATCH 10/35] Stop on finishing all interfaces Signed-off-by: Leo Siepel --- .../discovery/NetworkDiscoveryService.java | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index ab0c6401bb6f3..47af4be6cb743 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -67,7 +67,6 @@ public class NetworkDiscoveryService extends AbstractDiscoveryService implements // TCP port 554 (Windows share / Linux samba) // TCP port 1025 (Xbox / MS-RPC) private Set tcpServicePorts = Set.of(80, 548, 554, 1025); - private AtomicInteger scannedIPcount = new AtomicInteger(0); private @Nullable ExecutorService executorService = null; private final NetworkBindingConfiguration configuration = new NetworkBindingConfiguration(); private final NetworkUtils networkUtils = new NetworkUtils(); @@ -155,12 +154,15 @@ protected void startScan() { logger.debug("Starting Network Device Discovery"); Map> discoveryList = networkUtils.getNetworkIPsPerInterface(); + // Track completion for all interfaces + final int totalInterfaces = discoveryList.size(); + final AtomicInteger completedInterfaces = new AtomicInteger(0); for (String networkInterface : discoveryList.keySet()) { final Set networkIPs = networkUtils.getNetworkIPs( Objects.requireNonNull(discoveryList.get(networkInterface)), MAXIMUM_IPS_PER_INTERFACE); logger.debug("Scanning {} IPs on interface {} ", networkIPs.size(), networkInterface); - scannedIPcount.set(0); + final AtomicInteger scannedIPcount = new AtomicInteger(0); for (String ip : networkIPs) { service.execute(() -> { @@ -179,12 +181,18 @@ protected void startScan() { try { pd.getValue(); } catch (ExecutionException | InterruptedException e) { - stopScan(); + logger.warn("Error scanning IP {} on interface {}: {}", ip, networkInterface, e.getMessage()); + // Do not stop the whole scan; just log and continue } int count = scannedIPcount.incrementAndGet(); if (count == networkIPs.size()) { - logger.trace("Scan of {} IPs on interface {} successful", scannedIPcount, networkInterface); - stopScan(); + logger.debug("Scan of {} IPs on interface {} completed", scannedIPcount.get(), + networkInterface); + // Only call stopScan after all interfaces are done + if (completedInterfaces.incrementAndGet() == totalInterfaces) { + logger.debug("All network interface scans completed. Stopping scan."); + stopScan(); + } } }); } From a62ef9e9b3f9b11b7f231cb5bb8f60e75fd37e1c Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Fri, 29 Aug 2025 10:52:28 +0200 Subject: [PATCH 11/35] Change Arping to make use of completeablefeature Signed-off-by: Leo Siepel --- .../network/internal/PresenceDetection.java | 64 +++++++++++++------ .../internal/PresenceDetectionTest.java | 3 +- 2 files changed, 45 insertions(+), 22 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java index 260be92d03917..53da0b7e98808 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java @@ -29,8 +29,10 @@ import java.util.concurrent.CompletableFuture; import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; +import java.util.concurrent.Executor; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.ForkJoinPool; import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -101,11 +103,19 @@ public class PresenceDetection implements IPRequestReceivedCallback { int detectionChecks; private String lastReachableNetworkInterfaceName = ""; + // Executor for async tasks (e.g., iOS wakeup/arp chain) + private final Executor asyncExecutor; + + public PresenceDetection(final PresenceDetectionListener updateListener, + ScheduledExecutorService scheduledExecutorService, Duration cacheDeviceStateTime) { + this(updateListener, scheduledExecutorService, cacheDeviceStateTime, ForkJoinPool.commonPool()); + } + public PresenceDetection(final PresenceDetectionListener updateListener, - ScheduledExecutorService scheduledExecutorService, Duration cacheDeviceStateTime) - throws IllegalArgumentException { + ScheduledExecutorService scheduledExecutorService, Duration cacheDeviceStateTime, Executor asyncExecutor) { this.updateListener = updateListener; this.scheduledExecutorService = scheduledExecutorService; + this.asyncExecutor = asyncExecutor; cache = new ExpiringCacheAsync<>(cacheDeviceStateTime); } @@ -537,27 +547,39 @@ protected void performArpPing(PresenceDetectionValue pdv, String interfaceName) logger.trace("Perform ARP ping presence detection for {} on interface: {}", hostname, interfaceName); withDestinationAddress(destinationAddress -> { - try { - if (iosDevice) { - networkUtils.wakeUpIOS(destinationAddress); - Thread.sleep(50); - } - - PingResult pingResult = networkUtils.nativeArpPing(arpPingMethod, arpPingUtilPath, interfaceName, - destinationAddress.getHostAddress(), timeout); - if (pingResult != null) { - if (pingResult.isSuccess()) { - updateReachable(pdv, ARP_PING, getLatency(pingResult)); - lastReachableNetworkInterfaceName = interfaceName; - } else if (lastReachableNetworkInterfaceName.equals(interfaceName)) { - logger.trace("{} is no longer reachable on network interface: {}", hostname, interfaceName); - lastReachableNetworkInterfaceName = ""; + Runnable arpPingTask = () -> { + try { + PingResult pingResult = networkUtils.nativeArpPing(arpPingMethod, arpPingUtilPath, interfaceName, + destinationAddress.getHostAddress(), timeout); + if (pingResult != null) { + if (pingResult.isSuccess()) { + updateReachable(pdv, ARP_PING, getLatency(pingResult)); + lastReachableNetworkInterfaceName = interfaceName; + } else if (lastReachableNetworkInterfaceName.equals(interfaceName)) { + logger.trace("{} is no longer reachable on network interface: {}", hostname, interfaceName); + lastReachableNetworkInterfaceName = ""; + } } + } catch (IOException e) { + logger.trace("Failed to execute an ARP ping for {}", hostname, e); + } catch (InterruptedException e) { + // This can be ignored, the thread will end anyway } - } catch (IOException e) { - logger.trace("Failed to execute an ARP ping for {}", hostname, e); - } catch (InterruptedException ignored) { - // This can be ignored, the thread will end anyway + }; + + if (iosDevice) { + CompletableFuture.runAsync(() -> { + try { + networkUtils.wakeUpIOS(destinationAddress); + Thread.sleep(50); + } catch (InterruptedException e) { + // Ignore, thread will end + } catch (IOException e) { + logger.trace("Failed to wake up iOS device for {}", hostname, e); + } + }, asyncExecutor).thenRunAsync(arpPingTask, asyncExecutor); + } else { + arpPingTask.run(); } }); } diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java index c12e638060b4b..3783e7d619206 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java @@ -65,7 +65,8 @@ public void setUp() { doReturn(ArpPingUtilEnum.IPUTILS_ARPING).when(networkUtils).determineNativeArpPingMethod(anyString()); doReturn(IpPingMethodEnum.WINDOWS_PING).when(networkUtils).determinePingMethod(); - subject = spy(new PresenceDetection(listener, scheduledExecutorService, Duration.ofSeconds(2))); + // Inject a direct executor so async tasks run synchronously in tests + subject = spy(new PresenceDetection(listener, scheduledExecutorService, Duration.ofSeconds(2), Runnable::run)); subject.networkUtils = networkUtils; // Set a useful configuration. The default presenceDetection is a no-op. From 92a82bc9ae16305b002c092b1f1c31b7be07a490 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Fri, 29 Aug 2025 11:54:02 +0200 Subject: [PATCH 12/35] Seperate presence detection from lifecycle Signed-off-by: Leo Siepel --- .../network/internal/PresenceDetection.java | 61 ++++--------------- .../discovery/NetworkDiscoveryService.java | 2 +- .../internal/handler/NetworkHandler.java | 23 ++++--- .../internal/PresenceDetectionTest.java | 2 +- .../internal/handler/NetworkHandlerTest.java | 17 ++---- 5 files changed, 31 insertions(+), 74 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java index 53da0b7e98808..4a95c01a40149 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java @@ -33,8 +33,6 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; import java.util.concurrent.ForkJoinPool; -import java.util.concurrent.ScheduledExecutorService; -import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; @@ -79,7 +77,6 @@ public class PresenceDetection implements IPRequestReceivedCallback { private boolean useIcmpPing; private Set tcpPorts = new HashSet<>(); - private Duration refreshInterval = Duration.ofMinutes(1); private Duration timeout = Duration.ofSeconds(5); private @Nullable Instant lastSeen; @@ -93,10 +90,8 @@ public class PresenceDetection implements IPRequestReceivedCallback { ExpiringCacheAsync cache; private final PresenceDetectionListener updateListener; - private ScheduledExecutorService scheduledExecutorService; private Set networkInterfaceNames = Set.of(); - private @Nullable ScheduledFuture refreshJob; protected @Nullable ExecutorService detectionExecutorService; protected @Nullable ExecutorService waitForResultExecutorService; private String dhcpState = "off"; @@ -106,15 +101,13 @@ public class PresenceDetection implements IPRequestReceivedCallback { // Executor for async tasks (e.g., iOS wakeup/arp chain) private final Executor asyncExecutor; - public PresenceDetection(final PresenceDetectionListener updateListener, - ScheduledExecutorService scheduledExecutorService, Duration cacheDeviceStateTime) { - this(updateListener, scheduledExecutorService, cacheDeviceStateTime, ForkJoinPool.commonPool()); + public PresenceDetection(final PresenceDetectionListener updateListener, Duration cacheDeviceStateTime) { + this(updateListener, cacheDeviceStateTime, ForkJoinPool.commonPool()); } - public PresenceDetection(final PresenceDetectionListener updateListener, - ScheduledExecutorService scheduledExecutorService, Duration cacheDeviceStateTime, Executor asyncExecutor) { + public PresenceDetection(final PresenceDetectionListener updateListener, Duration cacheDeviceStateTime, + Executor asyncExecutor) { this.updateListener = updateListener; - this.scheduledExecutorService = scheduledExecutorService; this.asyncExecutor = asyncExecutor; cache = new ExpiringCacheAsync<>(cacheDeviceStateTime); } @@ -127,10 +120,6 @@ public Set getServicePorts() { return tcpPorts; } - public Duration getRefreshInterval() { - return refreshInterval; - } - public Duration getTimeout() { return timeout; } @@ -178,10 +167,6 @@ public void setUseDhcpSniffing(boolean enable) { this.useDHCPsniffing = enable; } - public void setRefreshInterval(Duration refreshInterval) { - this.refreshInterval = refreshInterval; - } - public void setTimeout(Duration timeout) { this.timeout = timeout; } @@ -633,40 +618,16 @@ public void dhcpRequestReceived(String ipAddress) { updateReachable(DHCP_REQUEST, Duration.ZERO); } - /** - * Start/Restart a fixed scheduled runner to update the devices reach-ability state. - */ - public void startAutomaticRefresh() { - ScheduledFuture future = refreshJob; - if (future != null && !future.isDone()) { - future.cancel(true); + public void refresh() { + try { + logger.debug("Refreshing {} reachability state", hostname); + getValue(); + } catch (InterruptedException | ExecutionException e) { + logger.debug("Failed to refresh {} presence detection", hostname, e); } - refreshJob = scheduledExecutorService.scheduleWithFixedDelay(() -> { - try { - logger.debug("Refreshing {} reachability state", hostname); - getValue(); - } catch (InterruptedException | ExecutionException e) { - logger.debug("Failed to refresh {} presence detection", hostname, e); - } - }, 0, refreshInterval.toMillis(), TimeUnit.MILLISECONDS); - } - - /** - * Return true if automatic refreshing is enabled. - */ - public boolean isAutomaticRefreshing() { - return refreshJob != null; } - /** - * Stop automatic refreshing. - */ - public void stopAutomaticRefresh() { - ScheduledFuture future = refreshJob; - if (future != null && !future.isDone()) { - future.cancel(true); - refreshJob = null; - } + public void dispose() { InetAddress cached = cachedDestination; if (cached != null) { disableDHCPListen(cached); diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index 47af4be6cb743..ffecbc807566a 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -201,7 +201,7 @@ protected void startScan() { } private final ThreadLocal presenceDetectorThreadLocal = ThreadLocal - .withInitial(() -> new PresenceDetection(this, scheduler, Duration.ofSeconds(2))); + .withInitial(() -> new PresenceDetection(this, Duration.ofSeconds(2))); @Override protected synchronized void stopScan() { diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java index ac1cf09102bc8..c15984d903c31 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java @@ -21,8 +21,11 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ScheduledFuture; +import java.util.concurrent.TimeUnit; import org.eclipse.jdt.annotation.NonNullByDefault; +import org.eclipse.jdt.annotation.Nullable; import org.openhab.binding.network.internal.NetworkBindingConfiguration; import org.openhab.binding.network.internal.NetworkBindingConfigurationListener; import org.openhab.binding.network.internal.NetworkBindingConstants; @@ -62,6 +65,7 @@ public class NetworkHandler extends BaseThingHandler implements PresenceDetectionListener, NetworkBindingConfigurationListener { private final Logger logger = LoggerFactory.getLogger(NetworkHandler.class); private @NonNullByDefault({}) PresenceDetection presenceDetection; + private @Nullable ScheduledFuture refreshJob; private @NonNullByDefault({}) WakeOnLanPacketSender wakeOnLanPacketSender; private boolean isTCPServiceDevice; @@ -85,7 +89,7 @@ public NetworkHandler(Thing thing, boolean isTCPServiceDevice, NetworkBindingCon private void refreshValue(ChannelUID channelUID) { // We are not yet even initialized, don't do anything - if (presenceDetection == null || !presenceDetection.isAutomaticRefreshing()) { + if (presenceDetection == null || refreshJob == null) { return; } @@ -153,9 +157,10 @@ public void finalDetectionResult(PresenceDetectionValue value) { @Override public void dispose() { - PresenceDetection detection = presenceDetection; - if (detection != null) { - detection.stopAutomaticRefresh(); + ScheduledFuture refreshJob = this.refreshJob; + if (refreshJob != null) { + refreshJob.cancel(true); + this.refreshJob = null; } presenceDetection = null; } @@ -189,16 +194,17 @@ void initialize(PresenceDetection presenceDetection) { } this.retries = handlerConfiguration.retry.intValue(); - presenceDetection.setRefreshInterval(Duration.ofMillis(handlerConfiguration.refreshInterval)); presenceDetection.setTimeout(Duration.ofMillis(handlerConfiguration.timeout)); wakeOnLanPacketSender = new WakeOnLanPacketSender(handlerConfiguration.macAddress, handlerConfiguration.hostname, handlerConfiguration.port, handlerConfiguration.networkInterfaceNames); updateStatus(ThingStatus.ONLINE); - presenceDetection.startAutomaticRefresh(); - updateNetworkProperties(); + if (handlerConfiguration.refreshInterval > 0) { + refreshJob = scheduler.scheduleWithFixedDelay(presenceDetection::refresh, 0, + handlerConfiguration.refreshInterval, TimeUnit.MILLISECONDS); + } } private void updateNetworkProperties() { @@ -214,8 +220,7 @@ private void updateNetworkProperties() { // Create a new network service and apply all configurations. @Override public void initialize() { - initialize(new PresenceDetection(this, scheduler, - Duration.ofMillis(configuration.cacheDeviceStateTimeInMS.intValue()))); + initialize(new PresenceDetection(this, Duration.ofMillis(configuration.cacheDeviceStateTimeInMS.intValue()))); } /** diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java index 3783e7d619206..754b09fc2cf5e 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java @@ -66,7 +66,7 @@ public void setUp() { doReturn(IpPingMethodEnum.WINDOWS_PING).when(networkUtils).determinePingMethod(); // Inject a direct executor so async tasks run synchronously in tests - subject = spy(new PresenceDetection(listener, scheduledExecutorService, Duration.ofSeconds(2), Runnable::run)); + subject = spy(new PresenceDetection(listener, Duration.ofSeconds(2), Runnable::run)); subject.networkUtils = networkUtils; // Set a useful configuration. The default presenceDetection is a no-op. diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java index b32bf9accb0d8..24e08c1dcd5d1 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java @@ -79,21 +79,15 @@ public void checkAllConfigurations() { conf.put(NetworkBindingConstants.PARAMETER_RETRY, 10); conf.put(NetworkBindingConstants.PARAMETER_HOSTNAME, "127.0.0.1"); conf.put(NetworkBindingConstants.PARAMETER_PORT, 8080); - conf.put(NetworkBindingConstants.PARAMETER_REFRESH_INTERVAL, 101010); conf.put(NetworkBindingConstants.PARAMETER_TIMEOUT, 1234); return conf; }); - PresenceDetection presenceDetection = spy( - new PresenceDetection(handler, scheduledExecutorService, Duration.ofSeconds(2))); - // Mock start/stop automatic refresh - doNothing().when(presenceDetection).startAutomaticRefresh(); - doNothing().when(presenceDetection).stopAutomaticRefresh(); + PresenceDetection presenceDetection = spy(new PresenceDetection(handler, Duration.ofSeconds(2))); handler.initialize(presenceDetection); assertThat(handler.retries, is(10)); assertThat(presenceDetection.getHostname(), is("127.0.0.1")); assertThat(presenceDetection.getServicePorts().iterator().next(), is(8080)); - assertThat(presenceDetection.getRefreshInterval(), is(Duration.ofMillis(101010))); assertThat(presenceDetection.getTimeout(), is(Duration.ofMillis(1234))); } @@ -109,7 +103,7 @@ public void tcpDeviceInitTests() { conf.put(NetworkBindingConstants.PARAMETER_HOSTNAME, "127.0.0.1"); return conf; }); - handler.initialize(new PresenceDetection(handler, scheduledExecutorService, Duration.ofSeconds(2))); + handler.initialize(new PresenceDetection(handler, Duration.ofSeconds(2))); // Check that we are offline ArgumentCaptor statusInfoCaptor = ArgumentCaptor.forClass(ThingStatusInfo.class); verify(callback).statusUpdated(eq(thing), statusInfoCaptor.capture()); @@ -126,13 +120,10 @@ public void pingDeviceInitTests() { when(thing.getConfiguration()).thenAnswer(a -> { Configuration conf = new Configuration(); conf.put(NetworkBindingConstants.PARAMETER_HOSTNAME, "127.0.0.1"); + conf.put(NetworkBindingConstants.PARAMETER_REFRESH_INTERVAL, 0); // disable auto refresh return conf; }); - PresenceDetection presenceDetection = spy( - new PresenceDetection(handler, scheduledExecutorService, Duration.ofSeconds(2))); - // Mock start/stop automatic refresh - doNothing().when(presenceDetection).startAutomaticRefresh(); - doNothing().when(presenceDetection).stopAutomaticRefresh(); + PresenceDetection presenceDetection = spy(new PresenceDetection(handler, Duration.ofSeconds(2))); doReturn(Instant.now()).when(presenceDetection).getLastSeen(); handler.initialize(presenceDetection); From ce17620ce276be4b412fd93a20b99eb91a9b5823 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Fri, 29 Aug 2025 17:26:12 +0200 Subject: [PATCH 13/35] Improve logging and filtering Signed-off-by: Leo Siepel --- .../binding/network/internal/PresenceDetection.java | 2 +- .../internal/discovery/NetworkDiscoveryService.java | 12 ++++++++++-- .../network/internal/utils/LatencyParser.java | 4 +--- 3 files changed, 12 insertions(+), 6 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java index 4a95c01a40149..5538571c2ef96 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java @@ -435,7 +435,7 @@ public CompletableFuture performPresenceDetection() { try { completableFuture.join(); } catch (CancellationException | CompletionException e) { - logger.debug("Detection future failed to complete", e); + logger.debug("Detection future failed to complete {}", e.getMessage()); } }); logger.debug("All {} detection futures for {} have completed", completableFutures.size(), hostname); diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index ffecbc807566a..b8eae1c69996c 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -26,6 +26,7 @@ import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.stream.Collectors; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; @@ -153,7 +154,14 @@ protected void startScan() { removeOlderResults(getTimestampOfLastScan(), null); logger.debug("Starting Network Device Discovery"); - Map> discoveryList = networkUtils.getNetworkIPsPerInterface(); + Map> discoveryList = networkUtils.getNetworkIPsPerInterface().entrySet().stream() + .filter(e -> e.getValue() != null && !e.getValue().isEmpty()) + .filter(e -> e.getValue().stream().noneMatch(addr -> { + String hostAddress = addr.getAddress().getHostAddress(); + return hostAddress.startsWith("127.") // loopback 127.0.0.0/8 + || hostAddress.startsWith("169.254."); // link-local 169.254.0.0/16 + })).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + // Track completion for all interfaces final int totalInterfaces = discoveryList.size(); final AtomicInteger completedInterfaces = new AtomicInteger(0); @@ -192,12 +200,12 @@ protected void startScan() { if (completedInterfaces.incrementAndGet() == totalInterfaces) { logger.debug("All network interface scans completed. Stopping scan."); stopScan(); + logger.debug("Finished Network Device Discovery"); } } }); } } - logger.debug("Finished Network Device Discovery"); } private final ThreadLocal presenceDetectorThreadLocal = ThreadLocal diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java index efdd317a57307..11bf9d7c59ec3 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java @@ -60,7 +60,7 @@ public class LatencyParser { * contain a latency value which matches the known patterns. */ public @Nullable Duration parseLatency(String inputLine) { - logger.debug("Parsing latency from input {}", inputLine); + logger.trace("Parsing latency from input {}", inputLine); Matcher m = LATENCY_PATTERN.matcher(inputLine); if (m.find() && m.groupCount() >= 2) { @@ -69,8 +69,6 @@ public class LatencyParser { } return millisToDuration(Double.parseDouble(m.group(1).replace(",", "."))); } - - logger.debug("Did not find a latency value"); return null; } } From 1397df2258cabda001d06779f054632951d200ee Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sat, 30 Aug 2025 13:33:54 +0200 Subject: [PATCH 14/35] Create seperate ScheduledExecutorService Signed-off-by: Leo Siepel --- .../network/internal/NetworkHandlerFactory.java | 10 +++++++--- .../network/internal/handler/NetworkHandler.java | 8 ++++++-- .../network/internal/handler/NetworkHandlerTest.java | 6 +++--- 3 files changed, 16 insertions(+), 8 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkHandlerFactory.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkHandlerFactory.java index 8fe4b2bc8f4a5..afd5179edf1f2 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkHandlerFactory.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkHandlerFactory.java @@ -13,11 +13,13 @@ package org.openhab.binding.network.internal; import java.util.Map; +import java.util.concurrent.ScheduledExecutorService; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; import org.openhab.binding.network.internal.handler.NetworkHandler; import org.openhab.binding.network.internal.handler.SpeedTestHandler; +import org.openhab.core.common.ThreadPoolManager; import org.openhab.core.config.core.Configuration; import org.openhab.core.thing.Thing; import org.openhab.core.thing.ThingTypeUID; @@ -42,8 +44,10 @@ @Component(service = ThingHandlerFactory.class, configurationPid = "binding.network") public class NetworkHandlerFactory extends BaseThingHandlerFactory { final NetworkBindingConfiguration configuration = new NetworkBindingConfiguration(); - + private static final String NETWORK_HANDLER_THREADPOOL_NAME = "networkBinding"; private final Logger logger = LoggerFactory.getLogger(NetworkHandlerFactory.class); + private final ScheduledExecutorService executor = ThreadPoolManager + .getScheduledPool(NETWORK_HANDLER_THREADPOOL_NAME); @Override public boolean supportsThingType(ThingTypeUID thingTypeUID) { @@ -78,9 +82,9 @@ protected void modified(Map config) { if (thingTypeUID.equals(NetworkBindingConstants.PING_DEVICE) || thingTypeUID.equals(NetworkBindingConstants.BACKWARDS_COMPATIBLE_DEVICE)) { - return new NetworkHandler(thing, false, configuration); + return new NetworkHandler(thing, executor, false, configuration); } else if (thingTypeUID.equals(NetworkBindingConstants.SERVICE_DEVICE)) { - return new NetworkHandler(thing, true, configuration); + return new NetworkHandler(thing, executor, true, configuration); } else if (thingTypeUID.equals(NetworkBindingConstants.SPEEDTEST_DEVICE)) { return new SpeedTestHandler(thing); } diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java index c15984d903c31..22e14a97ca6bb 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java @@ -21,6 +21,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; @@ -76,12 +77,15 @@ public class NetworkHandler extends BaseThingHandler // Retry counter. Will be reset as soon as a device presence detection succeed. private int retryCounter = 0; private NetworkHandlerConfiguration handlerConfiguration = new NetworkHandlerConfiguration(); + private ScheduledExecutorService executor; /** * Do not call this directly, but use the {@see NetworkHandlerBuilder} instead. */ - public NetworkHandler(Thing thing, boolean isTCPServiceDevice, NetworkBindingConfiguration configuration) { + public NetworkHandler(Thing thing, ScheduledExecutorService executor, boolean isTCPServiceDevice, + NetworkBindingConfiguration configuration) { super(thing); + this.executor = executor; this.isTCPServiceDevice = isTCPServiceDevice; this.configuration = configuration; this.configuration.addNetworkBindingConfigurationListener(this); @@ -202,7 +206,7 @@ void initialize(PresenceDetection presenceDetection) { updateStatus(ThingStatus.ONLINE); if (handlerConfiguration.refreshInterval > 0) { - refreshJob = scheduler.scheduleWithFixedDelay(presenceDetection::refresh, 0, + refreshJob = executor.scheduleWithFixedDelay(presenceDetection::refresh, 0, handlerConfiguration.refreshInterval, TimeUnit.MILLISECONDS); } } diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java index 24e08c1dcd5d1..23954240fc9fb 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java @@ -71,7 +71,7 @@ public void setUp() { @Test public void checkAllConfigurations() { NetworkBindingConfiguration config = new NetworkBindingConfiguration(); - NetworkHandler handler = spy(new NetworkHandler(thing, true, config)); + NetworkHandler handler = spy(new NetworkHandler(thing, scheduledExecutorService, true, config)); handler.setCallback(callback); // Provide all possible configuration when(thing.getConfiguration()).thenAnswer(a -> { @@ -94,7 +94,7 @@ public void checkAllConfigurations() { @Test public void tcpDeviceInitTests() { NetworkBindingConfiguration config = new NetworkBindingConfiguration(); - NetworkHandler handler = spy(new NetworkHandler(thing, true, config)); + NetworkHandler handler = spy(new NetworkHandler(thing, scheduledExecutorService, true, config)); assertThat(handler.isTCPServiceDevice(), is(true)); handler.setCallback(callback); // Port is missing, should make the device OFFLINE @@ -114,7 +114,7 @@ public void tcpDeviceInitTests() { @Test public void pingDeviceInitTests() { NetworkBindingConfiguration config = new NetworkBindingConfiguration(); - NetworkHandler handler = spy(new NetworkHandler(thing, false, config)); + NetworkHandler handler = spy(new NetworkHandler(thing, scheduledExecutorService, false, config)); handler.setCallback(callback); // Provide minimal configuration when(thing.getConfiguration()).thenAnswer(a -> { From ee8060875e4bb9ce343eec2db99d09393c7b5317 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sat, 30 Aug 2025 13:49:42 +0200 Subject: [PATCH 15/35] Fix review comment Signed-off-by: Leo Siepel --- .../network/internal/discovery/NetworkDiscoveryService.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index b8eae1c69996c..9564d838b07e3 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -188,9 +188,13 @@ protected void startScan() { try { pd.getValue(); - } catch (ExecutionException | InterruptedException e) { + } catch (ExecutionException e) { logger.warn("Error scanning IP {} on interface {}: {}", ip, networkInterface, e.getMessage()); // Do not stop the whole scan; just log and continue + } catch (InterruptedException e1) { + logger.trace("Scan interrupted for IP {} on interface {}", ip, networkInterface); + Thread.currentThread().interrupt(); + return; } int count = scannedIPcount.incrementAndGet(); if (count == networkIPs.size()) { From 4d2164ac67d575622226e38d5d8041143c85890a Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sat, 30 Aug 2025 14:18:03 +0200 Subject: [PATCH 16/35] Improve network address checks Signed-off-by: Leo Siepel --- .../discovery/NetworkDiscoveryService.java | 9 +-------- .../network/internal/utils/NetworkUtils.java | 20 +++++++++++++------ 2 files changed, 15 insertions(+), 14 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index 9564d838b07e3..24600e7e7af20 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -26,7 +26,6 @@ import java.util.concurrent.ThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; -import java.util.stream.Collectors; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; @@ -154,13 +153,7 @@ protected void startScan() { removeOlderResults(getTimestampOfLastScan(), null); logger.debug("Starting Network Device Discovery"); - Map> discoveryList = networkUtils.getNetworkIPsPerInterface().entrySet().stream() - .filter(e -> e.getValue() != null && !e.getValue().isEmpty()) - .filter(e -> e.getValue().stream().noneMatch(addr -> { - String hostAddress = addr.getAddress().getHostAddress(); - return hostAddress.startsWith("127.") // loopback 127.0.0.0/8 - || hostAddress.startsWith("169.254."); // link-local 169.254.0.0/16 - })).collect(Collectors.toMap(Map.Entry::getKey, Map.Entry::getValue)); + Map> discoveryList = networkUtils.getNetworkIPsPerInterface(); // Track completion for all interfaces final int totalInterfaces = discoveryList.size(); diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index 36138153efa4c..561a33a35d224 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -185,14 +185,22 @@ public Map> getNetworkIPsPerInterface() { try { for (Enumeration en = NetworkInterface.getNetworkInterfaces(); en.hasMoreElements();) { NetworkInterface networkInterface = en.nextElement(); - if (networkInterface.isUp() && !networkInterface.isLoopback()) { + if (networkInterface.isUp() || !networkInterface.isLoopback()) { + logger.trace("Network interface: {} is excluded in the search", networkInterface.getName()); + continue; + } + + Set addresses = networkInterface.getInterfaceAddresses().stream() + .map(m -> new CidrAddress(m.getAddress(), m.getNetworkPrefixLength())) + .filter(cidr -> !cidr.getAddress().isLoopbackAddress()) // (127.x.x.x, ::1) + .filter(cidr -> !cidr.getAddress().isLinkLocalAddress())// (169.254.x.x or fe80::/10) + .collect(Collectors.toSet()); + + if (!addresses.isEmpty()) { logger.trace("Network interface: {} is included in the search", networkInterface.getName()); - outputMap.put(networkInterface.getName(), - networkInterface.getInterfaceAddresses().stream() - .map(m -> new CidrAddress(m.getAddress(), m.getNetworkPrefixLength())) - .collect(Collectors.toSet())); + outputMap.put(networkInterface.getName(), addresses); } else { - logger.trace("Network interface: {} is excluded in the search", networkInterface.getName()); + logger.trace("Network interface: {} has no usable addresses", networkInterface.getName()); } } } catch (SocketException e) { From 2a6be86b9a74a6cb0ca49d6ef5bd2591148cac82 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sat, 30 Aug 2025 22:46:36 +0200 Subject: [PATCH 17/35] Improve interrupthandling Signed-off-by: Leo Siepel --- .../network/internal/PresenceDetection.java | 14 ++++++++++---- .../network/internal/utils/NetworkUtils.java | 2 +- 2 files changed, 11 insertions(+), 5 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java index 5538571c2ef96..0597c5ccec94d 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java @@ -548,7 +548,8 @@ protected void performArpPing(PresenceDetectionValue pdv, String interfaceName) } catch (IOException e) { logger.trace("Failed to execute an ARP ping for {}", hostname, e); } catch (InterruptedException e) { - // This can be ignored, the thread will end anyway + Thread.currentThread().interrupt(); + return; } }; @@ -558,7 +559,8 @@ protected void performArpPing(PresenceDetectionValue pdv, String interfaceName) networkUtils.wakeUpIOS(destinationAddress); Thread.sleep(50); } catch (InterruptedException e) { - // Ignore, thread will end + Thread.currentThread().interrupt(); + return; } catch (IOException e) { logger.trace("Failed to wake up iOS device for {}", hostname, e); } @@ -600,7 +602,8 @@ protected void performSystemPing(PresenceDetectionValue pdv) { } catch (IOException e) { logger.trace("Failed to execute a native ping for {}", hostname, e); } catch (InterruptedException e) { - // This can be ignored, the thread will end anyway + Thread.currentThread().interrupt(); + return; } }); } @@ -622,8 +625,11 @@ public void refresh() { try { logger.debug("Refreshing {} reachability state", hostname); getValue(); - } catch (InterruptedException | ExecutionException e) { + } catch (ExecutionException e) { logger.debug("Failed to refresh {} presence detection", hostname, e); + } catch (InterruptedException e) { + Thread.currentThread().interrupt(); + return; } } diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index 561a33a35d224..1b2d2eea7d5e6 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -305,7 +305,7 @@ public IpPingMethodEnum determinePingMethod() { } catch (IOException e) { logger.trace("Native ping to 127.0.0.1 failed", e); } catch (InterruptedException e) { - Thread.currentThread().interrupt(); // Reset interrupt flag + Thread.currentThread().interrupt(); } return IpPingMethodEnum.JAVA_PING; } From 7e3efa0a1faf2fb3943690833b8636810653125f Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sun, 31 Aug 2025 09:06:53 +0200 Subject: [PATCH 18/35] Revert threadlocal Signed-off-by: Leo Siepel --- .../discovery/NetworkDiscoveryService.java | 26 ++++++++----------- 1 file changed, 11 insertions(+), 15 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index 24600e7e7af20..56551ac66f789 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -166,19 +166,18 @@ protected void startScan() { final AtomicInteger scannedIPcount = new AtomicInteger(0); for (String ip : networkIPs) { - service.execute(() -> { - PresenceDetection pd = presenceDetectorThreadLocal.get(); - - // Reset per-IP fields - pd.setHostname(ip); - pd.setNetworkInterfaceNames(Set.of(networkInterface)); - pd.setIOSDevice(true); - pd.setUseDhcpSniffing(false); - pd.setTimeout(PING_TIMEOUT); - pd.setUseIcmpPing(true); - pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); - pd.setServicePorts(tcpServicePorts); + final PresenceDetection pd = new PresenceDetection(this, Duration.ofSeconds(2)); + pd.setHostname(ip); + pd.setIOSDevice(true); + pd.setUseDhcpSniffing(false); + pd.setTimeout(PING_TIMEOUT); + // Ping devices + pd.setUseIcmpPing(true); + pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); + // TCP devices + pd.setServicePorts(tcpServicePorts); + service.execute(() -> { try { pd.getValue(); } catch (ExecutionException e) { @@ -205,9 +204,6 @@ protected void startScan() { } } - private final ThreadLocal presenceDetectorThreadLocal = ThreadLocal - .withInitial(() -> new PresenceDetection(this, Duration.ofSeconds(2))); - @Override protected synchronized void stopScan() { super.stopScan(); From 5e1313f67d707ac82f69e41dd4693d2015ffe4b3 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sun, 31 Aug 2025 11:11:14 +0200 Subject: [PATCH 19/35] Refactor Presencedetection Signed-off-by: Leo Siepel --- .../network/internal/PresenceDetection.java | 70 ++--------- .../discovery/NetworkDiscoveryService.java | 2 +- .../internal/handler/NetworkHandler.java | 3 +- .../internal/PresenceDetectionTest.java | 113 ++++++++---------- .../internal/handler/NetworkHandlerTest.java | 8 +- 5 files changed, 71 insertions(+), 125 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java index 0597c5ccec94d..92fd5fdc14277 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java @@ -30,9 +30,6 @@ import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.ForkJoinPool; import java.util.concurrent.TimeUnit; import java.util.function.Consumer; @@ -92,23 +89,16 @@ public class PresenceDetection implements IPRequestReceivedCallback { private final PresenceDetectionListener updateListener; private Set networkInterfaceNames = Set.of(); - protected @Nullable ExecutorService detectionExecutorService; - protected @Nullable ExecutorService waitForResultExecutorService; private String dhcpState = "off"; int detectionChecks; private String lastReachableNetworkInterfaceName = ""; - // Executor for async tasks (e.g., iOS wakeup/arp chain) - private final Executor asyncExecutor; - - public PresenceDetection(final PresenceDetectionListener updateListener, Duration cacheDeviceStateTime) { - this(updateListener, cacheDeviceStateTime, ForkJoinPool.commonPool()); - } + private final Executor executor; public PresenceDetection(final PresenceDetectionListener updateListener, Duration cacheDeviceStateTime, - Executor asyncExecutor) { + Executor executor) { this.updateListener = updateListener; - this.asyncExecutor = asyncExecutor; + this.executor = executor; cache = new ExpiringCacheAsync<>(cacheDeviceStateTime); } @@ -317,10 +307,6 @@ public void getValue(Consumer callback) { cache.getValue(this::performPresenceDetection).thenAccept(callback); } - public ExecutorService getThreadsFor(int threadCount) { - return Executors.newFixedThreadPool(threadCount); - } - private void withDestinationAddress(Consumer consumer) { InetAddress destinationAddress = destination.getValue(); if (destinationAddress == null) { @@ -330,24 +316,8 @@ private void withDestinationAddress(Consumer consumer) { } } - private void stopDetection() { - ExecutorService detectionExecutorService = this.detectionExecutorService; - if (detectionExecutorService != null) { - logger.debug("Shutting down detectionExecutorService"); - detectionExecutorService.shutdownNow(); - this.detectionExecutorService = null; - } - ExecutorService waitForResultExecutorService = this.waitForResultExecutorService; - if (waitForResultExecutorService != null) { - logger.debug("Shutting down waitForResultExecutorService"); - waitForResultExecutorService.shutdownNow(); - this.waitForResultExecutorService = null; - } - } - /** * Perform a presence detection with ICMP-, ARP ping and TCP connection attempts simultaneously. - * A fixed thread pool will be created with as many threads as necessary to perform all tests at once. * * Please be aware of the following restrictions: *
    @@ -383,53 +353,40 @@ public CompletableFuture performPresenceDetection() { return CompletableFuture.completedFuture(pdv); } - stopDetection(); - - ExecutorService detectionExecutorService = getThreadsFor(detectionChecks); - this.detectionExecutorService = detectionExecutorService; - ExecutorService waitForResultExecutorService = getThreadsFor(1); - this.waitForResultExecutorService = waitForResultExecutorService; - List> completableFutures = new ArrayList<>(); for (Integer tcpPort : tcpPorts) { addAsyncDetection(completableFutures, () -> { - Thread.currentThread().setName("presenceDetectionTCP_" + hostname + " " + tcpPort); performServicePing(pdv, tcpPort); - }, detectionExecutorService); + }); } // ARP ping for IPv4 addresses. Use single executor for Windows tool and // each own executor for each network interface for other tools if (arpPingMethod == ArpPingUtilEnum.ELI_FULKERSON_ARP_PING_FOR_WINDOWS) { addAsyncDetection(completableFutures, () -> { - Thread.currentThread().setName("presenceDetectionARP_" + hostname + " "); - // arp-ping.exe tool capable of handling multiple interfaces by itself performArpPing(pdv, ""); - }, detectionExecutorService); + }); } else if (interfaceNames != null) { for (final String interfaceName : interfaceNames) { addAsyncDetection(completableFutures, () -> { - Thread.currentThread().setName("presenceDetectionARP_" + hostname + " " + interfaceName); performArpPing(pdv, interfaceName); - }, detectionExecutorService); + }); } } // ICMP ping if (pingMethod != IpPingMethodEnum.DISABLED) { addAsyncDetection(completableFutures, () -> { - Thread.currentThread().setName("presenceDetectionICMP_" + hostname); if (pingMethod == IpPingMethodEnum.JAVA_PING) { performJavaPing(pdv); } else { performSystemPing(pdv); } - }, detectionExecutorService); + }); } return CompletableFuture.supplyAsync(() -> { - Thread.currentThread().setName("presenceDetectionResult_" + hostname); logger.debug("Waiting for {} detection futures for {} to complete", completableFutures.size(), hostname); completableFutures.forEach(completableFuture -> { try { @@ -448,17 +405,12 @@ public CompletableFuture performPresenceDetection() { logger.debug("Sending listener final result: {}", pdv); updateListener.finalDetectionResult(pdv); - detectionExecutorService.shutdownNow(); - this.detectionExecutorService = null; - detectionChecks = 0; - return pdv; - }, waitForResultExecutorService); + }, executor); } - private void addAsyncDetection(List> completableFutures, Runnable detectionRunnable, - ExecutorService executorService) { - completableFutures.add(CompletableFuture.runAsync(detectionRunnable, executorService) + private void addAsyncDetection(List> completableFutures, Runnable detectionRunnable) { + completableFutures.add(CompletableFuture.runAsync(detectionRunnable, executor) .orTimeout(timeout.plusSeconds(3).toMillis(), TimeUnit.MILLISECONDS)); } @@ -564,7 +516,7 @@ protected void performArpPing(PresenceDetectionValue pdv, String interfaceName) } catch (IOException e) { logger.trace("Failed to wake up iOS device for {}", hostname, e); } - }, asyncExecutor).thenRunAsync(arpPingTask, asyncExecutor); + }, executor).thenRunAsync(arpPingTask, executor); } else { arpPingTask.run(); } diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index 56551ac66f789..164b0a083b4a3 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -166,7 +166,7 @@ protected void startScan() { final AtomicInteger scannedIPcount = new AtomicInteger(0); for (String ip : networkIPs) { - final PresenceDetection pd = new PresenceDetection(this, Duration.ofSeconds(2)); + final PresenceDetection pd = new PresenceDetection(this, Duration.ofSeconds(2), service); pd.setHostname(ip); pd.setIOSDevice(true); pd.setUseDhcpSniffing(false); diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java index 22e14a97ca6bb..a7757d470a3f5 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java @@ -224,7 +224,8 @@ private void updateNetworkProperties() { // Create a new network service and apply all configurations. @Override public void initialize() { - initialize(new PresenceDetection(this, Duration.ofMillis(configuration.cacheDeviceStateTimeInMS.intValue()))); + initialize(new PresenceDetection(this, Duration.ofMillis(configuration.cacheDeviceStateTimeInMS.intValue()), + executor)); } /** diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java index 754b09fc2cf5e..c38ee49461f87 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/PresenceDetectionTest.java @@ -22,10 +22,12 @@ import java.time.Duration; import java.util.Set; import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import java.util.concurrent.ScheduledExecutorService; import java.util.function.Consumer; import org.eclipse.jdt.annotation.NonNullByDefault; +import org.eclipse.jdt.annotation.Nullable; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -50,6 +52,7 @@ public class PresenceDetectionTest { private @NonNullByDefault({}) PresenceDetection subject; + private @NonNullByDefault({}) PresenceDetection asyncSubject; private @Mock @NonNullByDefault({}) Consumer callback; private @Mock @NonNullByDefault({}) ExecutorService detectionExecutorService; @@ -78,36 +81,59 @@ public void setUp() { subject.setUseArpPing(true, "arping", ArpPingUtilEnum.IPUTILS_ARPING); subject.setUseIcmpPing(true); + asyncSubject = spy(new PresenceDetection(listener, Duration.ofSeconds(2), Executors.newSingleThreadExecutor())); + + asyncSubject.networkUtils = networkUtils; + asyncSubject.setHostname("127.0.0.1"); + asyncSubject.setTimeout(Duration.ofMillis(300)); + asyncSubject.setUseDhcpSniffing(false); + asyncSubject.setIOSDevice(true); + asyncSubject.setServicePorts(Set.of(1010)); + asyncSubject.setUseArpPing(true, "arping", ArpPingUtilEnum.IPUTILS_ARPING); + asyncSubject.setUseIcmpPing(true); + assertThat(subject.pingMethod, is(IpPingMethodEnum.WINDOWS_PING)); } - // Depending on the amount of test methods an according amount of threads is spawned. - // We will check if they spawn and return in time. + // Depending on the amount of test methods an according amount of threads is used. @Test - public void threadCountTest() { - assertNull(subject.detectionExecutorService); + public void usedThreadCountTest() { + // Custom executor to count submitted tasks + class CountingExecutor implements java.util.concurrent.Executor { + int count = 0; + + @Override + public void execute(@Nullable Runnable command) { + count++; + if (command != null) { + command.run(); + } + } + } + CountingExecutor countingExecutor = new CountingExecutor(); + + // Create a new subject with the counting executor + subject = spy(new PresenceDetection(listener, Duration.ofSeconds(2), countingExecutor)); + subject.networkUtils = networkUtils; + subject.setHostname("127.0.0.1"); + subject.setTimeout(Duration.ofMillis(300)); + subject.setUseDhcpSniffing(false); + subject.setIOSDevice(true); + subject.setServicePorts(Set.of(1010)); + subject.setUseArpPing(true, "arping", ArpPingUtilEnum.IPUTILS_ARPING); + subject.setUseIcmpPing(true); doNothing().when(subject).performArpPing(any(), any()); doNothing().when(subject).performJavaPing(any()); doNothing().when(subject).performSystemPing(any()); doNothing().when(subject).performServicePing(any(), anyInt()); - doReturn(waitForResultExecutorService).when(subject).getThreadsFor(1); - subject.getValue(callback -> { + // No-op callback }); - // Thread count: ARP + ICMP + 1*TCP - assertThat(subject.detectionChecks, is(3)); - assertNotNull(subject.detectionExecutorService); - - // "Wait" for the presence detection to finish - ArgumentCaptor runnableCapture = ArgumentCaptor.forClass(Runnable.class); - verify(waitForResultExecutorService, times(1)).execute(runnableCapture.capture()); - runnableCapture.getValue().run(); - - assertThat(subject.detectionChecks, is(0)); - assertNull(subject.detectionExecutorService); + // Thread count: ARP + ICMP + 1*TCP + task completion watcher = 4 + assertThat(countingExecutor.count, is(4)); } @Test @@ -118,27 +144,11 @@ public void partialAndFinalCallbackTests() throws InterruptedException, IOExcept anyString(), any(), any()); doReturn(pingResult).when(networkUtils).servicePing(anyString(), anyInt(), any()); - doReturn(detectionExecutorService).when(subject).getThreadsFor(3); - doReturn(waitForResultExecutorService).when(subject).getThreadsFor(1); - subject.performPresenceDetection(); assertThat(subject.detectionChecks, is(3)); - // Perform the different presence detection threads now - ArgumentCaptor capture = ArgumentCaptor.forClass(Runnable.class); - verify(detectionExecutorService, times(3)).execute(capture.capture()); - for (Runnable r : capture.getAllValues()) { - r.run(); - } - - // "Wait" for the presence detection to finish - ArgumentCaptor runnableCapture = ArgumentCaptor.forClass(Runnable.class); - verify(waitForResultExecutorService, times(1)).execute(runnableCapture.capture()); - runnableCapture.getValue().run(); - - assertThat(subject.detectionChecks, is(0)); - + // All detection methods should be called (direct executor runs synchronously) verify(subject, times(0)).performJavaPing(any()); verify(subject).performSystemPing(any()); verify(subject).performArpPing(any(), any()); @@ -159,41 +169,22 @@ public void cacheTest() throws InterruptedException, IOException { anyString(), any(), any()); doReturn(pingResult).when(networkUtils).servicePing(anyString(), anyInt(), any()); - doReturn(detectionExecutorService).when(subject).getThreadsFor(3); - doReturn(waitForResultExecutorService).when(subject).getThreadsFor(1); - // We expect no valid value - assertTrue(subject.cache.isExpired()); + assertTrue(asyncSubject.cache.isExpired()); // Get value will issue a PresenceDetection internally. - subject.getValue(callback); - verify(subject).performPresenceDetection(); - assertNotNull(subject.detectionExecutorService); - // There should be no straight callback yet - verify(callback, times(0)).accept(any()); - - // Perform the different presence detection threads now - ArgumentCaptor capture = ArgumentCaptor.forClass(Runnable.class); - verify(detectionExecutorService, times(3)).execute(capture.capture()); - for (Runnable r : capture.getAllValues()) { - r.run(); - } - - // "Wait" for the presence detection to finish - capture = ArgumentCaptor.forClass(Runnable.class); - verify(waitForResultExecutorService, times(1)).execute(capture.capture()); - capture.getValue().run(); - - // Although there are multiple partial results and a final result, - // the getValue() consumers get the fastest response possible, and only once. + asyncSubject.getValue(callback); + verify(asyncSubject).performPresenceDetection(); + Thread.sleep(200); // give it some time to execute + // Callback should be called once with the result (since we use direct executor) verify(callback, times(1)).accept(any()); // As long as the cache is valid, we can get the result back again - subject.getValue(callback); + asyncSubject.getValue(callback); verify(callback, times(2)).accept(any()); // Invalidate value, we should not get a new callback immediately again - subject.cache.invalidateValue(); - subject.getValue(callback); + asyncSubject.cache.invalidateValue(); + asyncSubject.getValue(callback); verify(callback, times(2)).accept(any()); } } diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java index 23954240fc9fb..5a39777d0978d 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/handler/NetworkHandlerTest.java @@ -82,7 +82,8 @@ public void checkAllConfigurations() { conf.put(NetworkBindingConstants.PARAMETER_TIMEOUT, 1234); return conf; }); - PresenceDetection presenceDetection = spy(new PresenceDetection(handler, Duration.ofSeconds(2))); + PresenceDetection presenceDetection = spy( + new PresenceDetection(handler, Duration.ofSeconds(2), scheduledExecutorService)); handler.initialize(presenceDetection); assertThat(handler.retries, is(10)); @@ -103,7 +104,7 @@ public void tcpDeviceInitTests() { conf.put(NetworkBindingConstants.PARAMETER_HOSTNAME, "127.0.0.1"); return conf; }); - handler.initialize(new PresenceDetection(handler, Duration.ofSeconds(2))); + handler.initialize(new PresenceDetection(handler, Duration.ofSeconds(2), scheduledExecutorService)); // Check that we are offline ArgumentCaptor statusInfoCaptor = ArgumentCaptor.forClass(ThingStatusInfo.class); verify(callback).statusUpdated(eq(thing), statusInfoCaptor.capture()); @@ -123,7 +124,8 @@ public void pingDeviceInitTests() { conf.put(NetworkBindingConstants.PARAMETER_REFRESH_INTERVAL, 0); // disable auto refresh return conf; }); - PresenceDetection presenceDetection = spy(new PresenceDetection(handler, Duration.ofSeconds(2))); + PresenceDetection presenceDetection = spy( + new PresenceDetection(handler, Duration.ofSeconds(2), scheduledExecutorService)); doReturn(Instant.now()).when(presenceDetection).getLastSeen(); handler.initialize(presenceDetection); From 71508fbcf97e0f4db60c6efbfd09356042ac8692 Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Sat, 30 Aug 2025 21:43:13 +0200 Subject: [PATCH 20/35] Make LatencyParser accept Windows' <1ms syntax for minimal latency Signed-off-by: Ravi Nadahar --- .../openhab/binding/network/internal/utils/LatencyParser.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java index 11bf9d7c59ec3..e92b5cbb75e40 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java @@ -31,7 +31,7 @@ @NonNullByDefault public class LatencyParser { - private static final Pattern LATENCY_PATTERN = Pattern.compile(".*time=(.*) ?(u|m)s.*"); + private static final Pattern LATENCY_PATTERN = Pattern.compile(".*time(?:=|<)(.*) ?(u|m)s.*"); private final Logger logger = LoggerFactory.getLogger(LatencyParser.class); // This is how the input looks like on Mac and Linux: @@ -60,7 +60,7 @@ public class LatencyParser { * contain a latency value which matches the known patterns. */ public @Nullable Duration parseLatency(String inputLine) { - logger.trace("Parsing latency from input {}", inputLine); + logger.trace("Parsing latency from input \"{}\"", inputLine); Matcher m = LATENCY_PATTERN.matcher(inputLine); if (m.find() && m.groupCount() >= 2) { From b203761d5351fb1d14aaeb0e5d240ab948b9262b Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Sat, 30 Aug 2025 22:02:04 +0200 Subject: [PATCH 21/35] Remove misleading reference to non-existing NetworkHandlerBuilder Signed-off-by: Ravi Nadahar --- .../binding/network/internal/handler/NetworkHandler.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java index a7757d470a3f5..65076ffd3e222 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java @@ -80,7 +80,7 @@ public class NetworkHandler extends BaseThingHandler private ScheduledExecutorService executor; /** - * Do not call this directly, but use the {@see NetworkHandlerBuilder} instead. + * Creates a new instance using the specified parameters. */ public NetworkHandler(Thing thing, ScheduledExecutorService executor, boolean isTCPServiceDevice, NetworkBindingConfiguration configuration) { From e080baeab7f011b6125cf62d8660b5bde98eb160 Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Sat, 30 Aug 2025 23:11:39 +0200 Subject: [PATCH 22/35] Handle thread-safety of NetworkDiscoveryService.executorService Signed-off-by: Ravi Nadahar --- .../discovery/NetworkDiscoveryService.java | 40 ++++++++++++------- 1 file changed, 25 insertions(+), 15 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index 164b0a083b4a3..837657894efbc 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -67,7 +67,9 @@ public class NetworkDiscoveryService extends AbstractDiscoveryService implements // TCP port 554 (Windows share / Linux samba) // TCP port 1025 (Xbox / MS-RPC) private Set tcpServicePorts = Set.of(80, 548, 554, 1025); - private @Nullable ExecutorService executorService = null; + + /* All access must be guarded by "this" */ + private @Nullable ExecutorService executorService; private final NetworkBindingConfiguration configuration = new NetworkBindingConfiguration(); private final NetworkUtils networkUtils = new NetworkUtils(); @@ -98,8 +100,11 @@ protected void modified(@Nullable Map config) { @Override @Deactivate protected void deactivate() { - if (executorService != null) { - executorService.shutdownNow(); + synchronized (this) { + if (executorService != null) { + executorService.shutdownNow(); + executorService = null; + } } super.deactivate(); } @@ -142,10 +147,13 @@ private ExecutorService createDiscoveryExecutor() { */ @Override protected void startScan() { - if (executorService == null) { - executorService = createDiscoveryExecutor(); + final ExecutorService service; + synchronized (this) { + if (executorService == null) { + executorService = createDiscoveryExecutor(); + } + service = executorService; } - final ExecutorService service = executorService; if (service == null) { return; } @@ -205,23 +213,25 @@ protected void startScan() { } @Override - protected synchronized void stopScan() { - super.stopScan(); - final ExecutorService service = executorService; - if (service == null) { - return; + protected void stopScan() { + final ExecutorService service; + synchronized (this) { + super.stopScan(); + service = executorService; + if (service == null) { + return; + } + executorService = null; } - service.shutdown(); // initiate shutdown + service.shutdownNow(); // Initiate shutdown try { if (!service.awaitTermination(PING_TIMEOUT.toMillis(), TimeUnit.MILLISECONDS)) { - service.shutdownNow(); // force shutdown if still busy + logger.warn("Network discovery scan failed to stop within the timeout of {}", PING_TIMEOUT); } } catch (InterruptedException e) { - service.shutdownNow(); Thread.currentThread().interrupt(); } - executorService = null; } public static ThingUID createServiceUID(String ip, int tcpPort) { From 165df305864013561d3b5f34aa516491c7a784cf Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Tue, 2 Sep 2025 22:26:53 +0200 Subject: [PATCH 23/35] Fix network interface exclusion Signed-off-by: Ravi Nadahar --- .../openhab/binding/network/internal/utils/NetworkUtils.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index 1b2d2eea7d5e6..fb8ad326018eb 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -176,7 +176,7 @@ public Set getNetworkIPs(int maximumPerInterface) { /** * Retrieves a map of network interface names to their associated IP addresses. - * + * * @return A map where the key is the name of the network interface and the value is a set of CidrAddress objects * representing the IP addresses and network prefix lengths for that interface. */ @@ -185,7 +185,7 @@ public Map> getNetworkIPsPerInterface() { try { for (Enumeration en = NetworkInterface.getNetworkInterfaces(); en.hasMoreElements();) { NetworkInterface networkInterface = en.nextElement(); - if (networkInterface.isUp() || !networkInterface.isLoopback()) { + if (!networkInterface.isUp() || networkInterface.isLoopback()) { logger.trace("Network interface: {} is excluded in the search", networkInterface.getName()); continue; } From 8d099e00af3c44eacbc4f29f65651822c4c0d169 Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Tue, 2 Sep 2025 22:27:57 +0200 Subject: [PATCH 24/35] Make PresenceDetectionValue thread-safe Signed-off-by: Ravi Nadahar --- .../internal/PresenceDetectionValue.java | 52 +++++++++++-------- 1 file changed, 29 insertions(+), 23 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetectionValue.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetectionValue.java index b2a3a19c233e9..a33a2de70a14a 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetectionValue.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetectionValue.java @@ -24,9 +24,10 @@ import org.eclipse.jdt.annotation.NonNullByDefault; /** - * Contains the result or partial result of a presence detection. + * Contains the result or partial result of a presence detection. This class is thread-safe. * * @author David Graeff - Initial contribution + * @author Ravi Nadahar - Made class thread-safe */ @NonNullByDefault public class PresenceDetectionValue { @@ -34,9 +35,14 @@ public class PresenceDetectionValue { public static final Duration UNREACHABLE = Duration.ofMillis(-1); private final String hostAddress; + + /* All access must be guarded by "this" */ private Duration latency; + /* All access must be guarded by "this" */ private final Set reachableDetectionTypes = new TreeSet<>(); + + /* All access must be guarded by "this" */ private final List reachableTcpPorts = new ArrayList<>(); /** @@ -61,14 +67,14 @@ public String getHostAddress() { * Return the ping latency, {@value #UNREACHABLE} if not reachable. Can be 0 if * no specific latency is known but the device is still reachable. */ - public Duration getLowestLatency() { + public synchronized Duration getLowestLatency() { return latency; } /** * Returns true if the target is reachable by any means. */ - public boolean isReachable() { + public synchronized boolean isReachable() { return !UNREACHABLE.equals(latency); } @@ -86,9 +92,13 @@ boolean updateLatency(Duration newLatency) { "Latency must be >=0. Create a new PresenceDetectionValue for a not reachable device!"); } else if (newLatency.isZero()) { return false; - } else if (!isReachable() || latency.isZero() || newLatency.compareTo(latency) < 0) { - latency = newLatency; - return true; + } else { + synchronized (this) { + if (!isReachable() || latency.isZero() || newLatency.compareTo(latency) < 0) { + latency = newLatency; + return true; + } + } } return false; } @@ -98,14 +108,14 @@ boolean updateLatency(Duration newLatency) { * * @param type a {@link PresenceDetectionType}. */ - void addReachableDetectionType(PresenceDetectionType type) { + synchronized void addReachableDetectionType(PresenceDetectionType type) { reachableDetectionTypes.add(type); } /** * Return true if the target can be reached by ICMP or ARP pings. */ - public boolean isPingReachable() { + public synchronized boolean isPingReachable() { return reachableDetectionTypes.contains(PresenceDetectionType.ARP_PING) || reachableDetectionTypes.contains(PresenceDetectionType.ICMP_PING); } @@ -113,41 +123,37 @@ public boolean isPingReachable() { /** * Return true if the target provides open TCP ports. */ - public boolean isTcpServiceReachable() { + public synchronized boolean isTcpServiceReachable() { return reachableDetectionTypes.contains(PresenceDetectionType.TCP_CONNECTION); } /** * Return a string of comma-separated successful presence detection types. */ - public String getSuccessfulDetectionTypes() { + public synchronized String getSuccessfulDetectionTypes() { return reachableDetectionTypes.stream().map(PresenceDetectionType::name).collect(Collectors.joining(", ")); } /** * Return the reachable TCP ports of the presence detection value. - * Thread safe. */ - public List getReachableTcpPorts() { - synchronized (reachableTcpPorts) { - return new ArrayList<>(reachableTcpPorts); - } + public synchronized List getReachableTcpPorts() { + return new ArrayList<>(reachableTcpPorts); } /** * Add a reachable TCP port to this presence detection result value object. - * Thread safe. */ - void addReachableTcpPort(int tcpPort) { - synchronized (reachableTcpPorts) { - reachableTcpPorts.add(tcpPort); - } + synchronized void addReachableTcpPort(int tcpPort) { + reachableTcpPorts.add(tcpPort); } @Override public String toString() { - return "PresenceDetectionValue [hostAddress=" + hostAddress + ", latency=" + durationToMillis(latency) - + "ms, reachableDetectionTypes=" + reachableDetectionTypes + ", reachableTcpPorts=" + reachableTcpPorts - + "]"; + synchronized (this) { + return "PresenceDetectionValue [hostAddress=" + hostAddress + ", latency=" + durationToMillis(latency) + + "ms, reachableDetectionTypes=" + reachableDetectionTypes + ", reachableTcpPorts=" + + reachableTcpPorts + "]"; + } } } From 571f1a2e0f87b3b8b9bc4ced6f27790d3a767ef8 Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Tue, 2 Sep 2025 22:34:07 +0200 Subject: [PATCH 25/35] Partial refactoring of PresenceDetection Fixes: - Synchronization of lastSeen - Joining of async tasks - Minor logging improvements Addition: - Create setIcmpPingMethod() Signed-off-by: Ravi Nadahar --- .../network/internal/PresenceDetection.java | 67 +++++++++++++------ 1 file changed, 48 insertions(+), 19 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java index 92fd5fdc14277..7c2caf39e39ba 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/PresenceDetection.java @@ -30,7 +30,7 @@ import java.util.concurrent.CompletionException; import java.util.concurrent.ExecutionException; import java.util.concurrent.Executor; -import java.util.concurrent.TimeUnit; +import java.util.concurrent.TimeoutException; import java.util.function.Consumer; import org.eclipse.jdt.annotation.NonNullByDefault; @@ -48,7 +48,7 @@ import org.slf4j.LoggerFactory; /** - * The {@link PresenceDetection} handles the connection to the Device. + * The {@link PresenceDetection} handles the connection to the Device. This class is not thread-safe. * * @author Marc Mettke - Initial contribution * @author David Gräff, 2017 - Rewritten @@ -68,13 +68,15 @@ public class PresenceDetection implements IPRequestReceivedCallback { private String ipPingState = "Disabled"; protected String arpPingUtilPath = ""; private ArpPingUtilEnum arpPingMethod = ArpPingUtilEnum.DISABLED; - protected @Nullable IpPingMethodEnum pingMethod = IpPingMethodEnum.DISABLED; + protected IpPingMethodEnum pingMethod = IpPingMethodEnum.DISABLED; private boolean iosDevice; private boolean useArpPing; private boolean useIcmpPing; private Set tcpPorts = new HashSet<>(); private Duration timeout = Duration.ofSeconds(5); + + /** All access must be guarded by "this" */ private @Nullable Instant lastSeen; private @NonNullByDefault({}) String hostname; @@ -187,6 +189,27 @@ public void setUseIcmpPing(@Nullable Boolean useSystemPing) { } } + /** + * Sets the ping method directly. The exact ping method must be known, or things will fail. + * Use {@link NetworkUtils#determinePingMethod()} to find a supported method. + * + * @param pingMethod the {@link IpPingMethodEnum}. + */ + public void setIcmpPingMethod(IpPingMethodEnum pingMethod) { + this.pingMethod = pingMethod; + switch (pingMethod) { + case DISABLED: + ipPingState = "Disabled"; + break; + case JAVA_PING: + ipPingState = "Java ping"; + break; + default: + ipPingState = pingMethod.name(); + break; + } + } + /** * Enables or disables ARP pings. Will be automatically disabled if the destination * is not an IPv4 address. If the feature test for the native arping utility fails, @@ -281,7 +304,7 @@ public void setUseIcmPing(boolean useIcmpPing) { /** * Return the last seen value as an {@link Instant} or null if not yet seen. */ - public @Nullable Instant getLastSeen() { + public @Nullable synchronized Instant getLastSeen() { return lastSeen; } @@ -388,17 +411,22 @@ public CompletableFuture performPresenceDetection() { return CompletableFuture.supplyAsync(() -> { logger.debug("Waiting for {} detection futures for {} to complete", completableFutures.size(), hostname); - completableFutures.forEach(completableFuture -> { - try { - completableFuture.join(); - } catch (CancellationException | CompletionException e) { + try { + CompletableFuture.allOf(completableFutures.toArray(CompletableFuture[]::new)).join(); + logger.debug("All {} detection futures for {} have completed", completableFutures.size(), hostname); + } catch (CancellationException e) { + logger.debug("Detection future for {} was cancelled", hostname); + } catch (CompletionException e) { + if (e.getCause() instanceof TimeoutException) { + logger.debug("Detection future for {} timed out", hostname); + } else { logger.debug("Detection future failed to complete {}", e.getMessage()); + logger.trace("", e); } - }); - logger.debug("All {} detection futures for {} have completed", completableFutures.size(), hostname); + } if (!pdv.isReachable()) { - logger.debug("{} is unreachable, invalidating destination value", hostname); + logger.trace("{} is unreachable, invalidating cached destination value", hostname); destination.invalidateValue(); } @@ -410,8 +438,7 @@ public CompletableFuture performPresenceDetection() { } private void addAsyncDetection(List> completableFutures, Runnable detectionRunnable) { - completableFutures.add(CompletableFuture.runAsync(detectionRunnable, executor) - .orTimeout(timeout.plusSeconds(3).toMillis(), TimeUnit.MILLISECONDS)); + completableFutures.add(CompletableFuture.runAsync(detectionRunnable, executor)); } /** @@ -423,7 +450,7 @@ private void addAsyncDetection(List> completableFutures, * @param type the detection type * @param latency the latency */ - synchronized PresenceDetectionValue updateReachable(PresenceDetectionType type, Duration latency) { + PresenceDetectionValue updateReachable(PresenceDetectionType type, Duration latency) { PresenceDetectionValue pdv = new PresenceDetectionValue(hostname, latency); updateReachable(pdv, type, latency); return pdv; @@ -439,20 +466,22 @@ synchronized PresenceDetectionValue updateReachable(PresenceDetectionType type, * @param type the detection type * @param latency the latency */ - synchronized void updateReachable(PresenceDetectionValue pdv, PresenceDetectionType type, Duration latency) { + void updateReachable(PresenceDetectionValue pdv, PresenceDetectionType type, Duration latency) { updateReachable(pdv, type, latency, -1); } - synchronized void updateReachable(PresenceDetectionValue pdv, PresenceDetectionType type, Duration latency, - int tcpPort) { - lastSeen = Instant.now(); + void updateReachable(PresenceDetectionValue pdv, PresenceDetectionType type, Duration latency, int tcpPort) { + Instant now = Instant.now(); pdv.addReachableDetectionType(type); pdv.updateLatency(latency); if (0 <= tcpPort) { pdv.addReachableTcpPort(tcpPort); } logger.debug("Sending listener partial result: {}", pdv); - updateListener.partialDetectionResult(pdv); + synchronized (this) { + lastSeen = now; + updateListener.partialDetectionResult(pdv); + } } protected void performServicePing(PresenceDetectionValue pdv, int tcpPort) { From 42a03daa542b2f878c3154b51fef2b75b1f1dfc7 Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Tue, 2 Sep 2025 22:39:14 +0200 Subject: [PATCH 26/35] Partial refactoring of NetworkDiscoveryService Fixes: - Correct the number of addresses in a /24 subnet. - Restructure task creation so that one less thread is needed per scanned address, and so that startScan() is guaranteed to return quickly. - Create independent threads for posting discovery results, as this can take a long time and might not finish before the executor shuts down. Signed-off-by: Ravi Nadahar --- .../discovery/NetworkDiscoveryService.java | 102 +++++++++--------- .../internal/discovery/DiscoveryTest.java | 6 +- 2 files changed, 58 insertions(+), 50 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index 837657894efbc..66e5d42b7c21f 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -18,9 +18,8 @@ import java.time.Duration; import java.util.List; import java.util.Map; -import java.util.Objects; +import java.util.Map.Entry; import java.util.Set; -import java.util.concurrent.ExecutionException; import java.util.concurrent.ExecutorService; import java.util.concurrent.LinkedBlockingQueue; import java.util.concurrent.ThreadPoolExecutor; @@ -34,6 +33,7 @@ import org.openhab.binding.network.internal.PresenceDetectionListener; import org.openhab.binding.network.internal.PresenceDetectionValue; import org.openhab.binding.network.internal.utils.NetworkUtils; +import org.openhab.binding.network.internal.utils.NetworkUtils.IpPingMethodEnum; import org.openhab.core.config.core.Configuration; import org.openhab.core.config.discovery.AbstractDiscoveryService; import org.openhab.core.config.discovery.DiscoveryResultBuilder; @@ -59,7 +59,7 @@ @Component(service = DiscoveryService.class, configurationPid = "discovery.network") public class NetworkDiscoveryService extends AbstractDiscoveryService implements PresenceDetectionListener { static final Duration PING_TIMEOUT = Duration.ofMillis(500); - static final int MAXIMUM_IPS_PER_INTERFACE = 255; + static final int MAXIMUM_IPS_PER_INTERFACE = 254; private static final long DISCOVERY_RESULT_TTL = TimeUnit.MINUTES.toSeconds(10); private final Logger logger = LoggerFactory.getLogger(NetworkDiscoveryService.class); @@ -135,7 +135,7 @@ private ExecutorService createDiscoveryExecutor() { 60L, TimeUnit.SECONDS, // keep-alive for idle threads new LinkedBlockingQueue<>(cores * 50), // bounded queue r -> { - Thread t = new Thread(r, "NetworkDiscovery-" + count.getAndIncrement()); + Thread t = new Thread(r, "OH-binding-network-discoveryWorker-" + count.getAndIncrement()); t.setDaemon(true); return t; }, new ThreadPoolExecutor.CallerRunsPolicy() // backpressure when saturated @@ -167,49 +167,44 @@ protected void startScan() { final int totalInterfaces = discoveryList.size(); final AtomicInteger completedInterfaces = new AtomicInteger(0); - for (String networkInterface : discoveryList.keySet()) { - final Set networkIPs = networkUtils.getNetworkIPs( - Objects.requireNonNull(discoveryList.get(networkInterface)), MAXIMUM_IPS_PER_INTERFACE); - logger.debug("Scanning {} IPs on interface {} ", networkIPs.size(), networkInterface); - final AtomicInteger scannedIPcount = new AtomicInteger(0); + service.execute(() -> { + Thread.currentThread().setName("OH-binding-network-discoveryCoordinator"); + IpPingMethodEnum pingMethod = networkUtils.determinePingMethod(); + for (Entry> discovery : discoveryList.entrySet()) { + final String networkInterface = discovery.getKey(); + final Set networkIPs = networkUtils.getNetworkIPs(discovery.getValue(), + MAXIMUM_IPS_PER_INTERFACE); + logger.debug("Scanning {} IPs on interface {} ", networkIPs.size(), networkInterface); + final AtomicInteger scannedIPcount = new AtomicInteger(0); + final int targetCount = networkIPs.size(); - for (String ip : networkIPs) { - final PresenceDetection pd = new PresenceDetection(this, Duration.ofSeconds(2), service); - pd.setHostname(ip); - pd.setIOSDevice(true); - pd.setUseDhcpSniffing(false); - pd.setTimeout(PING_TIMEOUT); - // Ping devices - pd.setUseIcmpPing(true); - pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); - // TCP devices - pd.setServicePorts(tcpServicePorts); - - service.execute(() -> { - try { - pd.getValue(); - } catch (ExecutionException e) { - logger.warn("Error scanning IP {} on interface {}: {}", ip, networkInterface, e.getMessage()); - // Do not stop the whole scan; just log and continue - } catch (InterruptedException e1) { - logger.trace("Scan interrupted for IP {} on interface {}", ip, networkInterface); - Thread.currentThread().interrupt(); - return; - } - int count = scannedIPcount.incrementAndGet(); - if (count == networkIPs.size()) { - logger.debug("Scan of {} IPs on interface {} completed", scannedIPcount.get(), - networkInterface); - // Only call stopScan after all interfaces are done - if (completedInterfaces.incrementAndGet() == totalInterfaces) { - logger.debug("All network interface scans completed. Stopping scan."); - stopScan(); - logger.debug("Finished Network Device Discovery"); + for (String ip : networkIPs) { + final PresenceDetection pd = new PresenceDetection(this, Duration.ofSeconds(2), service); + pd.setHostname(ip); + pd.setIOSDevice(true); + pd.setUseDhcpSniffing(false); + pd.setTimeout(PING_TIMEOUT); + // Ping devices + pd.setIcmpPingMethod(pingMethod); + pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); + // TCP devices + pd.setServicePorts(tcpServicePorts); + pd.getValue((v) -> { + int count = scannedIPcount.incrementAndGet(); + if (count >= targetCount) { + logger.debug("Scan of {} IPs on interface {} completed", scannedIPcount.get(), + networkInterface); + // Only call stopScan after all interfaces are done + if (completedInterfaces.incrementAndGet() >= totalInterfaces) { + logger.debug("All network interface scans completed. Stopping scan."); + stopScan(); + logger.debug("Finished Network Device Discovery"); + } } - } - }); + }); + } } - } + }); } @Override @@ -223,6 +218,7 @@ protected void stopScan() { } executorService = null; } + logger.debug("Stopping Network Device Discovery"); service.shutdownNow(); // Initiate shutdown try { @@ -259,9 +255,15 @@ public void newServiceDevice(String ip, int tcpPort) { default -> "Network Device"; }; label += " (" + ip + ":" + tcpPort + ")"; + final String fLabel = label; - thingDiscovered(DiscoveryResultBuilder.create(createServiceUID(ip, tcpPort)).withTTL(DISCOVERY_RESULT_TTL) - .withProperty(PARAMETER_HOSTNAME, ip).withProperty(PARAMETER_PORT, tcpPort).withLabel(label).build()); + // A thread that isn't part of the executor is needed, because registering new discoveries is slow, + // and the executor is shut down when the scan is finished or aborted. + new Thread(() -> { + thingDiscovered(DiscoveryResultBuilder.create(createServiceUID(ip, tcpPort)).withTTL(DISCOVERY_RESULT_TTL) + .withProperty(PARAMETER_HOSTNAME, ip).withProperty(PARAMETER_PORT, tcpPort).withLabel(fLabel) + .build()); + }, "OH-binding-network-discoveryResultCourier").start(); } public static ThingUID createPingUID(String ip) { @@ -277,7 +279,11 @@ public static ThingUID createPingUID(String ip) { public void newPingDevice(String ip) { logger.trace("Found pingable network device with IP address {}", ip); - thingDiscovered(DiscoveryResultBuilder.create(createPingUID(ip)).withTTL(DISCOVERY_RESULT_TTL) - .withProperty(PARAMETER_HOSTNAME, ip).withLabel("Network Device (" + ip + ")").build()); + // A thread that isn't part of the executor is needed, because registering new discoveries is slow, + // and the executor is shut down when the scan is finished or aborted. + new Thread(() -> { + thingDiscovered(DiscoveryResultBuilder.create(createPingUID(ip)).withTTL(DISCOVERY_RESULT_TTL) + .withProperty(PARAMETER_HOSTNAME, ip).withLabel("Network Device (" + ip + ")").build()); + }, "OH-binding-network-discoveryPingCourier").start(); } } diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java index 7b88838be651a..2b393131f35a4 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java @@ -57,7 +57,7 @@ public void setUp() { } @Test - public void pingDeviceDetected() { + public void pingDeviceDetected() throws InterruptedException { NetworkDiscoveryService d = new NetworkDiscoveryService(); d.addDiscoveryListener(listener); @@ -67,6 +67,7 @@ public void pingDeviceDetected() { when(value.isPingReachable()).thenReturn(true); when(value.isTcpServiceReachable()).thenReturn(false); d.partialDetectionResult(value); + Thread.sleep(200L); verify(listener).thingDiscovered(any(), result.capture()); DiscoveryResult dresult = result.getValue(); assertThat(dresult.getThingUID(), is(NetworkDiscoveryService.createPingUID(ip))); @@ -74,7 +75,7 @@ public void pingDeviceDetected() { } @Test - public void tcpDeviceDetected() { + public void tcpDeviceDetected() throws InterruptedException { NetworkDiscoveryService d = new NetworkDiscoveryService(); d.addDiscoveryListener(listener); @@ -85,6 +86,7 @@ public void tcpDeviceDetected() { when(value.isTcpServiceReachable()).thenReturn(true); when(value.getReachableTcpPorts()).thenReturn(List.of(1010)); d.partialDetectionResult(value); + Thread.sleep(200L); verify(listener).thingDiscovered(any(), result.capture()); DiscoveryResult dresult = result.getValue(); assertThat(dresult.getThingUID(), is(NetworkDiscoveryService.createServiceUID(ip, 1010))); From 370b1337f897de2442dc932488a425d9393404fa Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Wed, 3 Sep 2025 20:40:44 +0200 Subject: [PATCH 27/35] Make NetworkHandler thread-safe Signed-off-by: Ravi Nadahar --- .../internal/WakeOnLanPacketSender.java | 2 +- .../internal/handler/NetworkHandler.java | 149 ++++++++++++------ 2 files changed, 102 insertions(+), 49 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/WakeOnLanPacketSender.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/WakeOnLanPacketSender.java index 59a4530a56048..a05d3f407be18 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/WakeOnLanPacketSender.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/WakeOnLanPacketSender.java @@ -67,7 +67,7 @@ public class WakeOnLanPacketSender { public WakeOnLanPacketSender(String macAddress, @Nullable String hostname, @Nullable Integer port, Set networkInterfaceNames) { - logger.debug("initialized WOL Packet Sender (mac: {}, hostname: {}, port: {}, networkInterfaceNames: {})", + logger.trace("initialized WOL Packet Sender (mac: {}, hostname: {}, port: {}, networkInterfaceNames: {})", macAddress, hostname, port, networkInterfaceNames); this.macAddress = macAddress; this.hostname = hostname; diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java index 65076ffd3e222..e0d9cb106b5c9 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java @@ -60,24 +60,30 @@ * @author Marc Mettke - Initial contribution * @author David Graeff - Rewritten * @author Wouter Born - Add Wake-on-LAN thing action support + * @author Ravi Nadahar - Made class thread-safe */ @NonNullByDefault public class NetworkHandler extends BaseThingHandler implements PresenceDetectionListener, NetworkBindingConfigurationListener { private final Logger logger = LoggerFactory.getLogger(NetworkHandler.class); - private @NonNullByDefault({}) PresenceDetection presenceDetection; + + /* All access must be guarded by "this" */ + private @Nullable PresenceDetection presenceDetection; + + /* All access must be guarded by "this" */ private @Nullable ScheduledFuture refreshJob; - private @NonNullByDefault({}) WakeOnLanPacketSender wakeOnLanPacketSender; - private boolean isTCPServiceDevice; - private NetworkBindingConfiguration configuration; + /* All access must be guarded by "this" */ + private @Nullable WakeOnLanPacketSender wakeOnLanPacketSender; + + private final boolean isTCPServiceDevice; + private final NetworkBindingConfiguration configuration; // How many retries before a device is deemed offline - int retries; + volatile int retries; // Retry counter. Will be reset as soon as a device presence detection succeed. - private int retryCounter = 0; - private NetworkHandlerConfiguration handlerConfiguration = new NetworkHandlerConfiguration(); - private ScheduledExecutorService executor; + private volatile int retryCounter = 0; + private final ScheduledExecutorService executor; /** * Creates a new instance using the specified parameters. @@ -92,17 +98,23 @@ public NetworkHandler(Thing thing, ScheduledExecutorService executor, boolean is } private void refreshValue(ChannelUID channelUID) { + PresenceDetection pd; + ScheduledFuture rj; + synchronized (this) { + pd = presenceDetection; + rj = refreshJob; + } // We are not yet even initialized, don't do anything - if (presenceDetection == null || refreshJob == null) { + if (pd == null || rj == null) { return; } switch (channelUID.getId()) { case CHANNEL_ONLINE: - presenceDetection.getValue(value -> updateState(CHANNEL_ONLINE, OnOffType.from(value.isReachable()))); + pd.getValue(value -> updateState(CHANNEL_ONLINE, OnOffType.from(value.isReachable()))); break; case CHANNEL_LATENCY: - presenceDetection.getValue(value -> { + pd.getValue(value -> { double latencyMs = durationToMillis(value.getLowestLatency()); updateState(CHANNEL_LATENCY, new QuantityType<>(latencyMs, MetricPrefix.MILLI(Units.SECOND))); }); @@ -110,7 +122,7 @@ private void refreshValue(ChannelUID channelUID) { case CHANNEL_LASTSEEN: // We should not set the last seen state to UNDEF, it prevents restoreOnStartup from working // For reference: https://github.com/openhab/openhab-addons/issues/17404 - Instant lastSeen = presenceDetection.getLastSeen(); + Instant lastSeen = pd.getLastSeen(); if (lastSeen != null) { updateState(CHANNEL_LASTSEEN, new DateTimeType(lastSeen)); } @@ -141,7 +153,11 @@ public void partialDetectionResult(PresenceDetectionValue value) { public void finalDetectionResult(PresenceDetectionValue value) { // We do not notify the framework immediately if a device presence detection failed and // the user configured retries to be > 1. - retryCounter = value.isReachable() ? 0 : retryCounter + 1; + if (value.isReachable()) { + retryCounter = 0; + } else { + retryCounter++; + } if (retryCounter >= retries) { updateState(CHANNEL_ONLINE, OnOffType.OFF); @@ -149,7 +165,11 @@ public void finalDetectionResult(PresenceDetectionValue value) { retryCounter = 0; } - Instant lastSeen = presenceDetection.getLastSeen(); + PresenceDetection pd; + synchronized (this) { + pd = presenceDetection; + } + Instant lastSeen = pd == null ? null : pd.getLastSeen(); if (value.isReachable() && lastSeen != null) { updateState(CHANNEL_LASTSEEN, new DateTimeType(lastSeen)); } @@ -161,12 +181,14 @@ public void finalDetectionResult(PresenceDetectionValue value) { @Override public void dispose() { - ScheduledFuture refreshJob = this.refreshJob; - if (refreshJob != null) { - refreshJob.cancel(true); - this.refreshJob = null; + synchronized (this) { + ScheduledFuture refreshJob = this.refreshJob; + if (refreshJob != null) { + refreshJob.cancel(true); + this.refreshJob = null; + } + presenceDetection = null; } - presenceDetection = null; } /** @@ -174,58 +196,67 @@ public void dispose() { * Used by testing for injecting. */ void initialize(PresenceDetection presenceDetection) { - handlerConfiguration = getConfigAs(NetworkHandlerConfiguration.class); + NetworkHandlerConfiguration config = getConfigAs(NetworkHandlerConfiguration.class); - this.presenceDetection = presenceDetection; - presenceDetection.setHostname(handlerConfiguration.hostname); - presenceDetection.setNetworkInterfaceNames(handlerConfiguration.networkInterfaceNames); + presenceDetection.setHostname(config.hostname); + presenceDetection.setNetworkInterfaceNames(config.networkInterfaceNames); presenceDetection.setPreferResponseTimeAsLatency(configuration.preferResponseTimeAsLatency); if (isTCPServiceDevice) { - Integer port = handlerConfiguration.port; + Integer port = config.port; if (port == null) { updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "No port configured!"); return; } presenceDetection.setServicePorts(Set.of(port)); } else { - presenceDetection.setIOSDevice(handlerConfiguration.useIOSWakeUp); + presenceDetection.setIOSDevice(config.useIOSWakeUp); // Hand over binding configurations to the network service presenceDetection.setUseDhcpSniffing(configuration.allowDHCPlisten); - presenceDetection.setUseIcmpPing(handlerConfiguration.useIcmpPing ? configuration.allowSystemPings : null); - presenceDetection.setUseArpPing(handlerConfiguration.useArpPing, configuration.arpPingToolPath, + presenceDetection.setUseIcmpPing(config.useIcmpPing ? configuration.allowSystemPings : null); + presenceDetection.setUseArpPing(config.useArpPing, configuration.arpPingToolPath, configuration.arpPingUtilMethod); } - this.retries = handlerConfiguration.retry.intValue(); - presenceDetection.setTimeout(Duration.ofMillis(handlerConfiguration.timeout)); - - wakeOnLanPacketSender = new WakeOnLanPacketSender(handlerConfiguration.macAddress, - handlerConfiguration.hostname, handlerConfiguration.port, handlerConfiguration.networkInterfaceNames); + this.retries = config.retry.intValue(); + presenceDetection.setTimeout(Duration.ofMillis(config.timeout)); + synchronized (this) { + this.presenceDetection = presenceDetection; + wakeOnLanPacketSender = new WakeOnLanPacketSender(config.macAddress, config.hostname, config.port, + config.networkInterfaceNames); + if (config.refreshInterval > 0) { + refreshJob = executor.scheduleWithFixedDelay(presenceDetection::refresh, 0, config.refreshInterval, + TimeUnit.MILLISECONDS); + } + } updateStatus(ThingStatus.ONLINE); - - if (handlerConfiguration.refreshInterval > 0) { - refreshJob = executor.scheduleWithFixedDelay(presenceDetection::refresh, 0, - handlerConfiguration.refreshInterval, TimeUnit.MILLISECONDS); - } } private void updateNetworkProperties() { // Update properties (after startAutomaticRefresh, to get the correct dhcp state) Map properties = editProperties(); - properties.put(NetworkBindingConstants.PROPERTY_ARP_STATE, presenceDetection.getArpPingState()); - properties.put(NetworkBindingConstants.PROPERTY_ICMP_STATE, presenceDetection.getIPPingState()); - properties.put(NetworkBindingConstants.PROPERTY_PRESENCE_DETECTION_TYPE, ""); - properties.put(NetworkBindingConstants.PROPERTY_DHCP_STATE, presenceDetection.getDhcpState()); + synchronized (this) { + PresenceDetection pd = presenceDetection; + if (pd == null) { + logger.warn("Can't update network properties because presenceDetection is null"); + return; + } + properties.put(NetworkBindingConstants.PROPERTY_ARP_STATE, pd.getArpPingState()); + properties.put(NetworkBindingConstants.PROPERTY_ICMP_STATE, pd.getIPPingState()); + properties.put(NetworkBindingConstants.PROPERTY_PRESENCE_DETECTION_TYPE, ""); + properties.put(NetworkBindingConstants.PROPERTY_DHCP_STATE, pd.getDhcpState()); + } updateProperties(properties); } // Create a new network service and apply all configurations. @Override public void initialize() { - initialize(new PresenceDetection(this, Duration.ofMillis(configuration.cacheDeviceStateTimeInMS.intValue()), - executor)); + executor.submit(() -> { + initialize(new PresenceDetection(this, Duration.ofMillis(configuration.cacheDeviceStateTimeInMS.intValue()), + executor)); + }); } /** @@ -238,7 +269,12 @@ public boolean isTCPServiceDevice() { @Override public void bindingConfigurationChanged() { // Make sure that changed binding configuration is reflected - presenceDetection.setPreferResponseTimeAsLatency(configuration.preferResponseTimeAsLatency); + synchronized (this) { + PresenceDetection pd = presenceDetection; + if (pd != null) { + pd.setPreferResponseTimeAsLatency(configuration.preferResponseTimeAsLatency); + } + } } @Override @@ -247,15 +283,32 @@ public Collection> getServices() { } public void sendWakeOnLanPacketViaIp() { - // Hostname can't be null - wakeOnLanPacketSender.sendWakeOnLanPacketViaIp(); + WakeOnLanPacketSender sender; + synchronized (this) { + sender = wakeOnLanPacketSender; + } + if (sender != null) { + // Hostname can't be null + sender.sendWakeOnLanPacketViaIp(); + } else { + logger.warn("Failed to send WoL packet via IP because sender is null"); + } } public void sendWakeOnLanPacketViaMac() { - if (handlerConfiguration.macAddress.isEmpty()) { + NetworkHandlerConfiguration config = getConfigAs(NetworkHandlerConfiguration.class); + if (config.macAddress.isEmpty()) { throw new IllegalStateException( "Cannot send WoL packet because the 'macAddress' is not configured for " + thing.getUID()); } - wakeOnLanPacketSender.sendWakeOnLanPacketViaMac(); + WakeOnLanPacketSender sender; + synchronized (this) { + sender = wakeOnLanPacketSender; + } + if (sender != null) { + sender.sendWakeOnLanPacketViaMac(); + } else { + logger.warn("Failed to send WoL packet via MAC because sender is null"); + } } } From cab9542fe82f81ef5a6d02635ab848e548036783 Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Thu, 4 Sep 2025 05:06:31 +0200 Subject: [PATCH 28/35] Make SpeedTestHandler thread-safe Signed-off-by: Ravi Nadahar --- .../internal/handler/SpeedTestHandler.java | 120 +++++++++++------- 1 file changed, 75 insertions(+), 45 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/SpeedTestHandler.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/SpeedTestHandler.java index 17460cd389b59..02e1de9444faa 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/SpeedTestHandler.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/SpeedTestHandler.java @@ -18,6 +18,7 @@ import java.math.BigDecimal; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; @@ -48,15 +49,22 @@ * measurements at a given interval and for given file / size * * @author Gaël L'hopital - Initial contribution + * @author Ravi Nadahar - Made class thread-safe */ @NonNullByDefault public class SpeedTestHandler extends BaseThingHandler implements ISpeedTestListener { private final Logger logger = LoggerFactory.getLogger(SpeedTestHandler.class); + + /* All access must be guarded by "this" */ private @Nullable SpeedTestSocket speedTestSocket; - private @NonNullByDefault({}) ScheduledFuture refreshTask; - private @NonNullByDefault({}) SpeedTestConfiguration configuration; + + /* All access must be guarded by "this" */ + private @Nullable ScheduledFuture refreshTask; + + /* All access must be guarded by "this" */ private State bufferedProgress = UnDefType.UNDEF; - private int timeouts; + + private final AtomicInteger timeouts = new AtomicInteger(); public SpeedTestHandler(Thing thing) { super(thing); @@ -64,44 +72,52 @@ public SpeedTestHandler(Thing thing) { @Override public void initialize() { - configuration = getConfigAs(SpeedTestConfiguration.class); startRefreshTask(); } - private synchronized void startSpeedTest() { - String url = configuration.getDownloadURL(); - if (speedTestSocket == null && url != null) { - logger.debug("Network speedtest started"); - final SpeedTestSocket socket = new SpeedTestSocket(1500); - speedTestSocket = socket; - socket.addSpeedTestListener(this); - updateState(CHANNEL_TEST_ISRUNNING, OnOffType.ON); - updateState(CHANNEL_TEST_START, new DateTimeType()); - updateState(CHANNEL_TEST_END, UnDefType.NULL); - updateProgress(new QuantityType<>(0, Units.PERCENT)); - socket.startDownload(url); - } else { - logger.info("A speedtest is already in progress, will retry on next refresh"); + private void startSpeedTest() { + SpeedTestConfiguration config = getConfigAs(SpeedTestConfiguration.class); + String url = config.getDownloadURL(); + if (url == null || url.isBlank()) { + logger.warn("Failed to start speedtest because the URL is blank"); + return; + } + synchronized (this) { + if (speedTestSocket == null) { + logger.debug("Network speedtest started"); + final SpeedTestSocket socket = new SpeedTestSocket(1500); + speedTestSocket = socket; + socket.addSpeedTestListener(this); + updateState(CHANNEL_TEST_ISRUNNING, OnOffType.ON); + updateState(CHANNEL_TEST_START, new DateTimeType()); + updateState(CHANNEL_TEST_END, UnDefType.NULL); + updateProgress(new QuantityType<>(0, Units.PERCENT)); + socket.startDownload(url); + } else { + logger.info("A speedtest is already in progress, will retry on next refresh"); + } } } - private synchronized void stopSpeedTest() { + private void stopSpeedTest() { updateState(CHANNEL_TEST_ISRUNNING, OnOffType.OFF); updateProgress(UnDefType.NULL); updateState(CHANNEL_TEST_END, new DateTimeType()); - if (speedTestSocket != null) { - SpeedTestSocket socket = speedTestSocket; - socket.closeSocket(); - socket.removeSpeedTestListener(this); - socket = null; - speedTestSocket = null; - logger.debug("Network speedtest finished"); + synchronized (this) { + if (speedTestSocket != null) { + SpeedTestSocket socket = speedTestSocket; + socket.closeSocket(); + socket.removeSpeedTestListener(this); + speedTestSocket = null; + logger.debug("Network speedtest finished"); + } } } @Override public void onCompletion(final @Nullable SpeedTestReport testReport) { - timeouts = configuration.maxTimeout; + SpeedTestConfiguration config = getConfigAs(SpeedTestConfiguration.class); + timeouts.set(config.maxTimeout); if (testReport != null) { BigDecimal rate = testReport.getTransferRateBit(); QuantityType quantity = new QuantityType<>(rate, BIT_PER_SECOND) @@ -110,9 +126,11 @@ public void onCompletion(final @Nullable SpeedTestReport testReport) { switch (testReport.getSpeedTestMode()) { case DOWNLOAD: updateState(CHANNEL_RATE_DOWN, quantity); - String url = configuration.getUploadURL(); - if (speedTestSocket != null && url != null) { - speedTestSocket.startUpload(configuration.getUploadURL(), configuration.uploadSize); + String url = config.getUploadURL(); + synchronized (this) { + if (speedTestSocket != null && url != null) { + speedTestSocket.startUpload(config.getUploadURL(), config.uploadSize); + } } break; case UPLOAD: @@ -132,12 +150,12 @@ public void onError(final @Nullable SpeedTestError testError, final @Nullable St updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, errorMessage); freeRefreshTask(); } else if (SpeedTestError.SOCKET_TIMEOUT.equals(testError)) { - timeouts--; - if (timeouts <= 0) { + int count = timeouts.decrementAndGet(); + if (count <= 0) { updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, "Max timeout count reached"); freeRefreshTask(); } else { - logger.warn("Speedtest timed out, {} attempts left. Message '{}'", timeouts, errorMessage); + logger.warn("Speedtest timed out, {} attempts left. Message '{}'", count, errorMessage); stopSpeedTest(); } } else if (SpeedTestError.SOCKET_ERROR.equals(testError) @@ -156,9 +174,15 @@ public void onProgress(float percent, @Nullable SpeedTestReport testReport) { } private void updateProgress(State state) { - if (!state.toString().equals(bufferedProgress.toString())) { - bufferedProgress = state; - updateState(CHANNEL_TEST_PROGRESS, bufferedProgress); + boolean isNew = false; + synchronized (this) { + if (!state.toString().equals(bufferedProgress.toString())) { + bufferedProgress = state; + isNew = true; + } + } + if (isNew) { + updateState(CHANNEL_TEST_PROGRESS, state); } } @@ -187,19 +211,25 @@ public void dispose() { } private void freeRefreshTask() { - stopSpeedTest(); - if (refreshTask != null) { - refreshTask.cancel(true); - refreshTask = null; + synchronized (this) { + ScheduledFuture task = refreshTask; + if (task != null) { + task.cancel(true); + refreshTask = null; + } } + stopSpeedTest(); } private void startRefreshTask() { - logger.info("Speedtests starts in {} minutes, then refreshes every {} minutes", configuration.initialDelay, - configuration.refreshInterval); - refreshTask = scheduler.scheduleWithFixedDelay(this::startSpeedTest, configuration.initialDelay, - configuration.refreshInterval, TimeUnit.MINUTES); - timeouts = configuration.maxTimeout; + SpeedTestConfiguration config = getConfigAs(SpeedTestConfiguration.class); + logger.info("Speedtests starts in {} minutes, then refreshes every {} minutes", config.initialDelay, + config.refreshInterval); + synchronized (this) { + refreshTask = scheduler.scheduleWithFixedDelay(this::startSpeedTest, config.initialDelay, + config.refreshInterval, TimeUnit.MINUTES); + } + timeouts.set(config.maxTimeout); updateStatus(ThingStatus.ONLINE); } } From 04786e28d18ad5f7a799659b22e74f30ef954fc0 Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Fri, 5 Sep 2025 06:00:52 +0200 Subject: [PATCH 29/35] Make sure that process output is consumed Signed-off-by: Ravi Nadahar --- .../network/internal/utils/NetworkUtils.java | 84 ++++++++++++------- 1 file changed, 53 insertions(+), 31 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index fb8ad326018eb..793051e468c1d 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -28,6 +28,7 @@ import java.net.SocketException; import java.net.SocketTimeoutException; import java.net.UnknownHostException; +import java.nio.charset.StandardCharsets; import java.time.Duration; import java.time.Instant; import java.util.ArrayList; @@ -362,15 +363,15 @@ public enum IpPingMethodEnum { switch (method) { case IPUTILS_LINUX_PING: proc = new ProcessBuilder("ping", "-w", String.valueOf(timeout.toSeconds()), "-c", "1", hostname) - .start(); + .redirectErrorStream(true).start(); break; case MAC_OS_PING: proc = new ProcessBuilder("ping", "-t", String.valueOf(timeout.toSeconds()), "-c", "1", hostname) - .start(); + .redirectErrorStream(true).start(); break; case WINDOWS_PING: proc = new ProcessBuilder("ping", "-w", String.valueOf(timeout.toMillis()), "-n", "1", hostname) - .start(); + .redirectErrorStream(true).start(); break; case JAVA_PING: default: @@ -378,6 +379,17 @@ public enum IpPingMethodEnum { return null; } + // Consume the output while the process runs + List output = new ArrayList<>(); + try (InputStreamReader isr = new InputStreamReader(proc.getInputStream(), StandardCharsets.UTF_8); + BufferedReader br = new BufferedReader(isr)) { + String line; + while ((line = br.readLine()) != null) { + output.add(line); + logger.trace("Network [ping output]: '{}'", line); + } + } + // The return code is 0 for a successful ping, 1 if device didn't // respond, and 2 if there is another error like network interface // not ready. @@ -385,28 +397,25 @@ public enum IpPingMethodEnum { // see https://superuser.com/questions/403905/ping-from-windows-7-get-no-reply-but-sets-errorlevel-to-0 int result = proc.waitFor(); + Instant execStopTime = Instant.now(); if (result != 0) { - return new PingResult(false, Duration.between(execStartTime, Instant.now())); + return new PingResult(false, Duration.between(execStartTime, execStopTime)); } - try (BufferedReader r = new BufferedReader(new InputStreamReader(proc.getInputStream()))) { - String line = r.readLine(); - if (line == null) { - throw new IOException("Received no output from ping process."); - } - do { - // Because of the Windows issue, we need to check this. We assume that the ping was successful whenever - // this specific string is contained in the output - if (line.contains("TTL=") || line.contains("ttl=")) { - PingResult pingResult = new PingResult(true, Duration.between(execStartTime, Instant.now())); - pingResult.setResponseTime(latencyParser.parseLatency(line)); - return pingResult; - } - line = r.readLine(); - } while (line != null); + if (output.isEmpty()) { + throw new IOException("Received no output from ping process."); + } - return new PingResult(false, Duration.between(execStartTime, Instant.now())); + for (String line : output) { + // Because of the Windows issue, we need to check this. We assume that the ping was successful whenever + // this specific string is contained in the output + if (line.contains("TTL=") || line.contains("ttl=")) { + PingResult pingResult = new PingResult(true, Duration.between(execStartTime, execStopTime)); + pingResult.setResponseTime(latencyParser.parseLatency(line)); + return pingResult; + } } + return new PingResult(false, Duration.between(execStartTime, execStopTime)); } public enum ArpPingUtilEnum { @@ -451,34 +460,47 @@ public enum ArpPingUtilEnum { Instant execStartTime = Instant.now(); Process proc; if (arpingTool == ArpPingUtilEnum.THOMAS_HABERT_ARPING_WITHOUT_TIMEOUT) { - proc = new ProcessBuilder(arpUtilPath, "-c", "1", "-i", interfaceName, ipV4address).start(); + proc = new ProcessBuilder(arpUtilPath, "-c", "1", "-i", interfaceName, ipV4address) + .redirectErrorStream(true).start(); } else if (arpingTool == ArpPingUtilEnum.THOMAS_HABERT_ARPING) { proc = new ProcessBuilder(arpUtilPath, "-w", String.valueOf(timeout.toSeconds()), "-C", "1", "-i", - interfaceName, ipV4address).start(); + interfaceName, ipV4address).redirectErrorStream(true).start(); } else if (arpingTool == ArpPingUtilEnum.ELI_FULKERSON_ARP_PING_FOR_WINDOWS) { - proc = new ProcessBuilder(arpUtilPath, "-w", String.valueOf(timeout.toMillis()), "-x", ipV4address).start(); + proc = new ProcessBuilder(arpUtilPath, "-w", String.valueOf(timeout.toMillis()), "-x", ipV4address) + .redirectErrorStream(true).start(); } else { proc = new ProcessBuilder(arpUtilPath, "-w", String.valueOf(timeout.toSeconds()), "-c", "1", "-I", - interfaceName, ipV4address).start(); + interfaceName, ipV4address).redirectErrorStream(true).start(); + } + + // Consume the output while the process runs + List output = new ArrayList<>(); + try (InputStreamReader isr = new InputStreamReader(proc.getInputStream(), StandardCharsets.UTF_8); + BufferedReader br = new BufferedReader(isr)) { + String line; + while ((line = br.readLine()) != null) { + output.add(line); + logger.trace("Network [arping output]: '{}'", line); + } } // The return code is 0 for a successful ping. 1 if device didn't respond and 2 if there is another error like // network interface not ready. int result = proc.waitFor(); + Instant execStopTime = Instant.now(); if (result != 0) { - return new PingResult(false, Duration.between(execStartTime, Instant.now())); + return new PingResult(false, Duration.between(execStartTime, execStopTime)); } - PingResult pingResult = new PingResult(true, Duration.between(execStartTime, Instant.now())); - try (BufferedReader r = new BufferedReader(new InputStreamReader(proc.getInputStream()))) { - String line = r.readLine(); - while (line != null) { - Duration responseTime = latencyParser.parseLatency(line); + PingResult pingResult = new PingResult(true, Duration.between(execStartTime, execStopTime)); + Duration responseTime; + for (String line : output) { + if (!line.isBlank()) { + responseTime = latencyParser.parseLatency(line); if (responseTime != null) { pingResult.setResponseTime(responseTime); return pingResult; } - line = r.readLine(); } } From 14a0d39f1030647bf912a22bd2a368153ac3cd47 Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Fri, 5 Sep 2025 06:04:58 +0200 Subject: [PATCH 30/35] Implement tool-specific latency parsing Signed-off-by: Ravi Nadahar --- .../network/internal/utils/LatencyParser.java | 31 +++++++++++++++++-- .../network/internal/utils/NetworkUtils.java | 4 +-- .../internal/utils/LatencyParserTest.java | 11 +++++-- 3 files changed, 39 insertions(+), 7 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java index e92b5cbb75e40..732281e032ebb 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/LatencyParser.java @@ -32,6 +32,12 @@ public class LatencyParser { private static final Pattern LATENCY_PATTERN = Pattern.compile(".*time(?:=|<)(.*) ?(u|m)s.*"); + private static final Pattern THOMAS_HABERT_ARPING_PATTERN = Pattern + .compile("^[\\w ]+from[\\w:()\\.= ]+?time=([0-9,\\.]+)\\s?(m|u)sec$"); + private static final Pattern IPUTILS_ARPING_PATTERN = Pattern + .compile("^Unicast[\\w ]+from[\\w:()\\.= \\[\\]]+?\\s*([0-9,\\.]+)\\s?(m|u)s$"); + private static final Pattern ELI_FULKERSON_ARP_PING_PATTERN = Pattern + .compile("^Reply that[\\w:\\. ]+?\\sin\\s([0-9,\\.]+)\\s?(m|u)s$"); private final Logger logger = LoggerFactory.getLogger(LatencyParser.class); // This is how the input looks like on Mac and Linux: @@ -56,13 +62,34 @@ public class LatencyParser { * Examine a single ping or arping command output line and try to extract the latency value if it is contained. * * @param inputLine Single output line of the ping or arping command. + * @param type the syntax to expect. Use {@code null} for generic ping syntax. * @return Latency value provided by the ping or arping command. null if the provided line did not * contain a latency value which matches the known patterns. */ - public @Nullable Duration parseLatency(String inputLine) { + public @Nullable Duration parseLatency(String inputLine, @Nullable ArpPingUtilEnum type) { logger.trace("Parsing latency from input \"{}\"", inputLine); - Matcher m = LATENCY_PATTERN.matcher(inputLine); + Pattern pattern; + if (type == null) { + pattern = LATENCY_PATTERN; + } else { + switch (type) { + case ELI_FULKERSON_ARP_PING_FOR_WINDOWS: + pattern = ELI_FULKERSON_ARP_PING_PATTERN; + break; + case IPUTILS_ARPING: + pattern = IPUTILS_ARPING_PATTERN; + break; + case THOMAS_HABERT_ARPING: + case THOMAS_HABERT_ARPING_WITHOUT_TIMEOUT: + pattern = THOMAS_HABERT_ARPING_PATTERN; + break; + default: + pattern = LATENCY_PATTERN; + break; + } + } + Matcher m = pattern.matcher(inputLine); if (m.find() && m.groupCount() >= 2) { if ("u".equals(m.group(2))) { return microsToDuration(Double.parseDouble(m.group(1).replace(",", "."))); diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java index 793051e468c1d..f2c5b5a0b0a93 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/utils/NetworkUtils.java @@ -411,7 +411,7 @@ public enum IpPingMethodEnum { // this specific string is contained in the output if (line.contains("TTL=") || line.contains("ttl=")) { PingResult pingResult = new PingResult(true, Duration.between(execStartTime, execStopTime)); - pingResult.setResponseTime(latencyParser.parseLatency(line)); + pingResult.setResponseTime(latencyParser.parseLatency(line, null)); return pingResult; } } @@ -496,7 +496,7 @@ public enum ArpPingUtilEnum { Duration responseTime; for (String line : output) { if (!line.isBlank()) { - responseTime = latencyParser.parseLatency(line); + responseTime = latencyParser.parseLatency(line, arpingTool); if (responseTime != null) { pingResult.setResponseTime(responseTime); return pingResult; diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/utils/LatencyParserTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/utils/LatencyParserTest.java index 7e1414bb0b008..1a938714ed625 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/utils/LatencyParserTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/utils/LatencyParserTest.java @@ -35,7 +35,7 @@ public void parseLinuxAndMacResultFoundTest() { String input = "64 bytes from 192.168.1.1: icmp_seq=0 ttl=64 time=1.225 ms"; // Act - Duration resultLatency = latencyParser.parseLatency(input); + Duration resultLatency = latencyParser.parseLatency(input, null); // Assert assertNotNull(resultLatency); @@ -55,7 +55,7 @@ public void parseLinuxAndMacResultNotFoundTest() { for (String inputLine : inputLines) { // Act - Duration resultLatency = latencyParser.parseLatency(inputLine); + Duration resultLatency = latencyParser.parseLatency(inputLine, null); // Assert assertNull(resultLatency); @@ -69,10 +69,15 @@ public void parseWindows10ResultFoundTest() { String input = "Reply from 192.168.178.207: bytes=32 time=2ms TTL=64"; // Act - Duration resultLatency = latencyParser.parseLatency(input); + Duration resultLatency = latencyParser.parseLatency(input, null); // Assert assertNotNull(resultLatency); assertEquals(2, durationToMillis(resultLatency), 0); + + input = "Reply from 10.80.5.2: bytes=32 time<1ms TTL=64"; + resultLatency = latencyParser.parseLatency(input, null); + assertNotNull(resultLatency); + assertEquals(1, durationToMillis(resultLatency), 0.0); } } From 5fcd5a916bfc2e43760fc611c12a6e9f8374bddf Mon Sep 17 00:00:00 2001 From: Ravi Nadahar Date: Fri, 5 Sep 2025 21:03:59 +0200 Subject: [PATCH 31/35] Fix NetworkDiscoveryService configuration and make the thread pool size configurable Signed-off-by: Ravi Nadahar --- .../internal/NetworkBindingConfiguration.java | 11 ++- .../internal/NetworkBindingConstants.java | 1 + .../internal/NetworkHandlerFactory.java | 13 +-- .../discovery/NetworkDiscoveryService.java | 97 +++++++++++-------- .../src/main/resources/OH-INF/addon/addon.xml | 7 ++ .../internal/discovery/DiscoveryTest.java | 4 +- 6 files changed, 84 insertions(+), 49 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkBindingConfiguration.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkBindingConfiguration.java index b9604e6e435be..22195524b10d7 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkBindingConfiguration.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkBindingConfiguration.java @@ -29,13 +29,17 @@ @NonNullByDefault public class NetworkBindingConfiguration { + public final static int DEFAULT_DISCOVERY_THREADS = 100; + public final static String DEFAULT_ARPING_TOOL_PATH = "arping"; + public final static ArpPingUtilEnum DEFAULT_ARPING_METHOD = ArpPingUtilEnum.DISABLED; public boolean allowSystemPings = true; public boolean allowDHCPlisten = true; public BigDecimal cacheDeviceStateTimeInMS = BigDecimal.valueOf(2000); - public String arpPingToolPath = "arping"; - public ArpPingUtilEnum arpPingUtilMethod = ArpPingUtilEnum.DISABLED; + public String arpPingToolPath = DEFAULT_ARPING_TOOL_PATH; + public ArpPingUtilEnum arpPingUtilMethod = DEFAULT_ARPING_METHOD; // For backwards compatibility reasons, the default is to use the ping method execution time as latency value public boolean preferResponseTimeAsLatency = false; + public int numberOfDiscoveryThreads = DEFAULT_DISCOVERY_THREADS; private List listeners = new ArrayList<>(); @@ -45,6 +49,7 @@ public void update(NetworkBindingConfiguration newConfiguration) { this.cacheDeviceStateTimeInMS = newConfiguration.cacheDeviceStateTimeInMS; this.arpPingToolPath = newConfiguration.arpPingToolPath; this.preferResponseTimeAsLatency = newConfiguration.preferResponseTimeAsLatency; + this.numberOfDiscoveryThreads = newConfiguration.numberOfDiscoveryThreads; NetworkUtils networkUtils = new NetworkUtils(); this.arpPingUtilMethod = networkUtils.determineNativeArpPingMethod(arpPingToolPath); @@ -65,6 +70,6 @@ public String toString() { return "NetworkBindingConfiguration{" + "allowSystemPings=" + allowSystemPings + ", allowDHCPlisten=" + allowDHCPlisten + ", cacheDeviceStateTimeInMS=" + cacheDeviceStateTimeInMS + ", arpPingToolPath='" + arpPingToolPath + '\'' + ", arpPingUtilMethod=" + arpPingUtilMethod + ", preferResponseTimeAsLatency=" - + preferResponseTimeAsLatency + '}'; + + preferResponseTimeAsLatency + ", numberOfDiscoveryThreads=" + numberOfDiscoveryThreads + '}'; } } diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkBindingConstants.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkBindingConstants.java index 492f430816d15..ccb41e46c2f01 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkBindingConstants.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkBindingConstants.java @@ -28,6 +28,7 @@ public class NetworkBindingConstants { public static final String BINDING_ID = "network"; + public static final String BINDING_CONFIGURATION_PID = "binding.network"; // List of all Thing Type UIDs public static final ThingTypeUID BACKWARDS_COMPATIBLE_DEVICE = new ThingTypeUID(BINDING_ID, "device"); diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkHandlerFactory.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkHandlerFactory.java index afd5179edf1f2..36f123e98e39c 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkHandlerFactory.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/NetworkHandlerFactory.java @@ -12,6 +12,8 @@ */ package org.openhab.binding.network.internal; +import static org.openhab.binding.network.internal.NetworkBindingConstants.*; + import java.util.Map; import java.util.concurrent.ScheduledExecutorService; @@ -41,7 +43,7 @@ * @author David Graeff - Initial contribution */ @NonNullByDefault -@Component(service = ThingHandlerFactory.class, configurationPid = "binding.network") +@Component(service = ThingHandlerFactory.class, configurationPid = BINDING_CONFIGURATION_PID) public class NetworkHandlerFactory extends BaseThingHandlerFactory { final NetworkBindingConfiguration configuration = new NetworkBindingConfiguration(); private static final String NETWORK_HANDLER_THREADPOOL_NAME = "networkBinding"; @@ -51,7 +53,7 @@ public class NetworkHandlerFactory extends BaseThingHandlerFactory { @Override public boolean supportsThingType(ThingTypeUID thingTypeUID) { - return NetworkBindingConstants.SUPPORTED_THING_TYPES_UIDS.contains(thingTypeUID); + return SUPPORTED_THING_TYPES_UIDS.contains(thingTypeUID); } // The activate component call is used to access the bindings configuration @@ -80,12 +82,11 @@ protected void modified(Map config) { protected @Nullable ThingHandler createHandler(Thing thing) { ThingTypeUID thingTypeUID = thing.getThingTypeUID(); - if (thingTypeUID.equals(NetworkBindingConstants.PING_DEVICE) - || thingTypeUID.equals(NetworkBindingConstants.BACKWARDS_COMPATIBLE_DEVICE)) { + if (thingTypeUID.equals(PING_DEVICE) || thingTypeUID.equals(BACKWARDS_COMPATIBLE_DEVICE)) { return new NetworkHandler(thing, executor, false, configuration); - } else if (thingTypeUID.equals(NetworkBindingConstants.SERVICE_DEVICE)) { + } else if (thingTypeUID.equals(SERVICE_DEVICE)) { return new NetworkHandler(thing, executor, true, configuration); - } else if (thingTypeUID.equals(NetworkBindingConstants.SPEEDTEST_DEVICE)) { + } else if (thingTypeUID.equals(SPEEDTEST_DEVICE)) { return new SpeedTestHandler(thing); } return null; diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index 66e5d42b7c21f..e7433e9b8c0e9 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -15,16 +15,20 @@ import static org.openhab.binding.network.internal.NetworkBindingConstants.*; import static org.openhab.binding.network.internal.utils.NetworkUtils.durationToMillis; +import java.io.IOException; import java.time.Duration; +import java.util.Collections; +import java.util.Dictionary; import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Set; import java.util.concurrent.ExecutorService; -import java.util.concurrent.LinkedBlockingQueue; -import java.util.concurrent.ThreadPoolExecutor; +import java.util.concurrent.Executors; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.function.Function; +import java.util.stream.Collectors; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; @@ -34,16 +38,17 @@ import org.openhab.binding.network.internal.PresenceDetectionValue; import org.openhab.binding.network.internal.utils.NetworkUtils; import org.openhab.binding.network.internal.utils.NetworkUtils.IpPingMethodEnum; -import org.openhab.core.config.core.Configuration; import org.openhab.core.config.discovery.AbstractDiscoveryService; import org.openhab.core.config.discovery.DiscoveryResultBuilder; import org.openhab.core.config.discovery.DiscoveryService; import org.openhab.core.net.CidrAddress; import org.openhab.core.thing.ThingUID; +import org.osgi.service.cm.Configuration; +import org.osgi.service.cm.ConfigurationAdmin; import org.osgi.service.component.annotations.Activate; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Deactivate; -import org.osgi.service.component.annotations.Modified; +import org.osgi.service.component.annotations.Reference; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -70,31 +75,16 @@ public class NetworkDiscoveryService extends AbstractDiscoveryService implements /* All access must be guarded by "this" */ private @Nullable ExecutorService executorService; - private final NetworkBindingConfiguration configuration = new NetworkBindingConfiguration(); private final NetworkUtils networkUtils = new NetworkUtils(); + private final @Nullable ConfigurationAdmin admin; - public NetworkDiscoveryService() { + @Activate + public NetworkDiscoveryService(@Reference @Nullable ConfigurationAdmin admin) { super(SUPPORTED_THING_TYPES_UIDS, (int) Math.round(new NetworkUtils().getNetworkIPs(MAXIMUM_IPS_PER_INTERFACE).size() * (durationToMillis(PING_TIMEOUT) / 1000.0)), false); - } - - @Override - @Activate - public void activate(@Nullable Map config) { - super.activate(config); - modified(config); - } - - @Override - @Modified - protected void modified(@Nullable Map config) { - super.modified(config); - // We update instead of replace the configuration object, so that if the user updates the - // configuration, the values are automatically available in all handlers. Because they all - // share the same instance. - configuration.update(new Configuration(config).as(NetworkBindingConfiguration.class)); + this.admin = admin; } @Override @@ -126,20 +116,23 @@ public void partialDetectionResult(PresenceDetectionValue value) { public void finalDetectionResult(PresenceDetectionValue value) { } - private ExecutorService createDiscoveryExecutor() { - int cores = Runtime.getRuntime().availableProcessors(); + private ExecutorService createDiscoveryExecutor(@Nullable NetworkBindingConfiguration configuration) { AtomicInteger count = new AtomicInteger(1); - - return new ThreadPoolExecutor(cores * 2, // core pool size - cores * 8, // max pool size for bursts - 60L, TimeUnit.SECONDS, // keep-alive for idle threads - new LinkedBlockingQueue<>(cores * 50), // bounded queue - r -> { - Thread t = new Thread(r, "OH-binding-network-discoveryWorker-" + count.getAndIncrement()); - t.setDaemon(true); - return t; - }, new ThreadPoolExecutor.CallerRunsPolicy() // backpressure when saturated - ); + int numThreads = configuration == null ? NetworkBindingConfiguration.DEFAULT_DISCOVERY_THREADS + : configuration.numberOfDiscoveryThreads; + if (numThreads > 0) { + return Executors.newFixedThreadPool(numThreads, r -> { + Thread t = new Thread(r, "OH-binding-network-discoveryWorker-" + count.getAndIncrement()); + t.setDaemon(true); + return t; + }); + } else { + return Executors.newCachedThreadPool(r -> { + Thread t = new Thread(r, "OH-binding-network-discoveryWorker-" + count.getAndIncrement()); + t.setDaemon(true); + return t; + }); + } } /** @@ -147,10 +140,11 @@ private ExecutorService createDiscoveryExecutor() { */ @Override protected void startScan() { + NetworkBindingConfiguration configuration = getConfig(); final ExecutorService service; synchronized (this) { if (executorService == null) { - executorService = createDiscoveryExecutor(); + executorService = createDiscoveryExecutor(configuration); } service = executorService; } @@ -186,7 +180,12 @@ protected void startScan() { pd.setTimeout(PING_TIMEOUT); // Ping devices pd.setIcmpPingMethod(pingMethod); - pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); + if (configuration == null) { + pd.setUseArpPing(true, NetworkBindingConfiguration.DEFAULT_ARPING_TOOL_PATH, + NetworkBindingConfiguration.DEFAULT_ARPING_METHOD); + } else { + pd.setUseArpPing(true, configuration.arpPingToolPath, configuration.arpPingUtilMethod); + } // TCP devices pd.setServicePorts(tcpServicePorts); pd.getValue((v) -> { @@ -286,4 +285,26 @@ public void newPingDevice(String ip) { .withProperty(PARAMETER_HOSTNAME, ip).withLabel("Network Device (" + ip + ")").build()); }, "OH-binding-network-discoveryPingCourier").start(); } + + private @Nullable NetworkBindingConfiguration getConfig() { + ConfigurationAdmin admin = this.admin; + if (admin == null) { + return null; + } + try { + Configuration configOnline = admin.getConfiguration(BINDING_CONFIGURATION_PID, null); + if (configOnline != null) { + Dictionary props = configOnline.getProperties(); + if (props != null) { + Map propMap = Collections.list(props.keys()).stream() + .collect(Collectors.toMap(Function.identity(), props::get)); + return new org.openhab.core.config.core.Configuration(propMap) + .as(NetworkBindingConfiguration.class); + } + } + } catch (IOException e) { + logger.warn("Unable to read configuration: {}", e.getMessage()); + } + return null; + } } diff --git a/bundles/org.openhab.binding.network/src/main/resources/OH-INF/addon/addon.xml b/bundles/org.openhab.binding.network/src/main/resources/OH-INF/addon/addon.xml index 6e3f9bd271b15..840dd0f87de23 100644 --- a/bundles/org.openhab.binding.network/src/main/resources/OH-INF/addon/addon.xml +++ b/bundles/org.openhab.binding.network/src/main/resources/OH-INF/addon/addon.xml @@ -43,5 +43,12 @@ such latency value is found in the ping command output, the time to execute the ping command is used as fallback latency. If disabled, the time to execute the ping command is always used as latency value. + + 100 + + The number of threads to use when scanning for network devices. Fewer threads, results in lower memory + consumption but a slower operation. Use 0 for unlimited. + true + diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java index 2b393131f35a4..0da3639d2a8b6 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java @@ -58,7 +58,7 @@ public void setUp() { @Test public void pingDeviceDetected() throws InterruptedException { - NetworkDiscoveryService d = new NetworkDiscoveryService(); + NetworkDiscoveryService d = new NetworkDiscoveryService(null); d.addDiscoveryListener(listener); ArgumentCaptor result = ArgumentCaptor.forClass(DiscoveryResult.class); @@ -76,7 +76,7 @@ public void pingDeviceDetected() throws InterruptedException { @Test public void tcpDeviceDetected() throws InterruptedException { - NetworkDiscoveryService d = new NetworkDiscoveryService(); + NetworkDiscoveryService d = new NetworkDiscoveryService(null); d.addDiscoveryListener(listener); ArgumentCaptor result = ArgumentCaptor.forClass(DiscoveryResult.class); From 75c1e07337576aff91a791eaae7e9873b34878af Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sat, 6 Sep 2025 08:55:41 +0200 Subject: [PATCH 32/35] i18n Signed-off-by: Leo Siepel --- .../src/main/resources/OH-INF/i18n/network.properties | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bundles/org.openhab.binding.network/src/main/resources/OH-INF/i18n/network.properties b/bundles/org.openhab.binding.network/src/main/resources/OH-INF/i18n/network.properties index 787f3f959311f..e8e55efece46a 100644 --- a/bundles/org.openhab.binding.network/src/main/resources/OH-INF/i18n/network.properties +++ b/bundles/org.openhab.binding.network/src/main/resources/OH-INF/i18n/network.properties @@ -13,6 +13,8 @@ addon.config.network.arpPingToolPath.label = ARP Ping Tool Path addon.config.network.arpPingToolPath.description = If your arp ping tool is not called arping and cannot be found in the PATH environment, you can configure the absolute path / tool name here. addon.config.network.cacheDeviceStateTimeInMS.label = Cache Time addon.config.network.cacheDeviceStateTimeInMS.description = The result of a device presence detection is cached for a small amount of time. Be aware that no new pings will be issued within this time frame, even if explicitly requested. +addon.config.network.numberOfDiscoveryThreads.label = Number of Discovery Threads +addon.config.network.numberOfDiscoveryThreads.description = The number of threads to use when scanning for network devices. Fewer threads, results in lower memory consumption but a slower operation. Use 0 for unlimited. addon.config.network.preferResponseTimeAsLatency.label = Use Response Time as Latency addon.config.network.preferResponseTimeAsLatency.description = If enabled, an attempt will be made to extract the latency from the output of the ping command. If no such latency value is found in the ping command output, the time to execute the ping command is used as fallback latency. If disabled, the time to execute the ping command is always used as latency value. From e36ff671df0a74af3267745bcc6fa787115dede9 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sat, 6 Sep 2025 10:36:05 +0200 Subject: [PATCH 33/35] Review comments Signed-off-by: Leo Siepel --- bundles/org.openhab.binding.network/README.md | 2 ++ .../discovery/NetworkDiscoveryService.java | 14 ++++++++------ .../network/internal/handler/NetworkHandler.java | 1 + 3 files changed, 11 insertions(+), 6 deletions(-) diff --git a/bundles/org.openhab.binding.network/README.md b/bundles/org.openhab.binding.network/README.md index a742e80c95c69..10a44f042c6c7 100644 --- a/bundles/org.openhab.binding.network/README.md +++ b/bundles/org.openhab.binding.network/README.md @@ -14,6 +14,7 @@ The binding has the following configuration options: - **arpPingToolPath:** If the ARP ping tool is not called `arping` and cannot be found in the PATH environment variable, the absolute path can be configured here. Default is `arping`. - **cacheDeviceStateTimeInMS:** The result of a device presence detection is cached for a small amount of time. Set this time here in milliseconds. Be aware that no new pings will be issued within this time frame, even if explicitly requested. Default is 2000. - **preferResponseTimeAsLatency:** If enabled, an attempt will be made to extract the latency from the output of the ping command. If no such latency value is found in the ping command output, the time to execute the ping command is used as fallback latency. If disabled, the time to execute the ping command is always used as latency value. This is disabled by default to be backwards-compatible and to not break statistics and monitoring which existed before this feature. +- **numberOfDiscoveryThreads:** Specifies the number of threads to be used during the discovery process. Increasing this value may speed up the discovery of devices on large networks but could also increase the load on the system. Default is `100`. _note:_The binding needs a restart for this configuration parameter to have effect. Create a `/services/network.cfg` file and use the above options like this: @@ -22,6 +23,7 @@ binding.network:allowSystemPings=true binding.network:allowDHCPlisten=false binding.network:arpPingToolPath=arping binding.network:cacheDeviceStateTimeInMS=2000 +binding.network:numberOfDiscoveryThreads=100 ``` ## Supported Things diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java index e7433e9b8c0e9..3edf0eac729bf 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/discovery/NetworkDiscoveryService.java @@ -12,7 +12,12 @@ */ package org.openhab.binding.network.internal.discovery; -import static org.openhab.binding.network.internal.NetworkBindingConstants.*; +import static org.openhab.binding.network.internal.NetworkBindingConstants.BINDING_CONFIGURATION_PID; +import static org.openhab.binding.network.internal.NetworkBindingConstants.PARAMETER_HOSTNAME; +import static org.openhab.binding.network.internal.NetworkBindingConstants.PARAMETER_PORT; +import static org.openhab.binding.network.internal.NetworkBindingConstants.PING_DEVICE; +import static org.openhab.binding.network.internal.NetworkBindingConstants.SERVICE_DEVICE; +import static org.openhab.binding.network.internal.NetworkBindingConstants.SUPPORTED_THING_TYPES_UIDS; import static org.openhab.binding.network.internal.utils.NetworkUtils.durationToMillis; import java.io.IOException; @@ -76,10 +81,10 @@ public class NetworkDiscoveryService extends AbstractDiscoveryService implements /* All access must be guarded by "this" */ private @Nullable ExecutorService executorService; private final NetworkUtils networkUtils = new NetworkUtils(); - private final @Nullable ConfigurationAdmin admin; + private final ConfigurationAdmin admin; @Activate - public NetworkDiscoveryService(@Reference @Nullable ConfigurationAdmin admin) { + public NetworkDiscoveryService(@Reference ConfigurationAdmin admin) { super(SUPPORTED_THING_TYPES_UIDS, (int) Math.round(new NetworkUtils().getNetworkIPs(MAXIMUM_IPS_PER_INTERFACE).size() * (durationToMillis(PING_TIMEOUT) / 1000.0)), @@ -288,9 +293,6 @@ public void newPingDevice(String ip) { private @Nullable NetworkBindingConfiguration getConfig() { ConfigurationAdmin admin = this.admin; - if (admin == null) { - return null; - } try { Configuration configOnline = admin.getConfiguration(BINDING_CONFIGURATION_PID, null); if (configOnline != null) { diff --git a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java index e0d9cb106b5c9..39d8a1e009e19 100644 --- a/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java +++ b/bundles/org.openhab.binding.network/src/main/java/org/openhab/binding/network/internal/handler/NetworkHandler.java @@ -253,6 +253,7 @@ private void updateNetworkProperties() { // Create a new network service and apply all configurations. @Override public void initialize() { + updateStatus(ThingStatus.UNKNOWN); executor.submit(() -> { initialize(new PresenceDetection(this, Duration.ofMillis(configuration.cacheDeviceStateTimeInMS.intValue()), executor)); From b4a70da89ec188887c0601c5350c319463ed9b22 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sat, 6 Sep 2025 10:56:51 +0200 Subject: [PATCH 34/35] Fix test Signed-off-by: Leo Siepel --- .../binding/network/internal/discovery/DiscoveryTest.java | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java index 0da3639d2a8b6..762ee46f6f510 100644 --- a/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java +++ b/bundles/org.openhab.binding.network/src/test/java/org/openhab/binding/network/internal/discovery/DiscoveryTest.java @@ -33,6 +33,7 @@ import org.openhab.binding.network.internal.PresenceDetectionValue; import org.openhab.core.config.discovery.DiscoveryListener; import org.openhab.core.config.discovery.DiscoveryResult; +import org.osgi.service.cm.ConfigurationAdmin; /** * Tests cases for {@see PresenceDetectionValue} @@ -58,7 +59,8 @@ public void setUp() { @Test public void pingDeviceDetected() throws InterruptedException { - NetworkDiscoveryService d = new NetworkDiscoveryService(null); + ConfigurationAdmin configAdmin = mock(ConfigurationAdmin.class); + NetworkDiscoveryService d = new NetworkDiscoveryService(configAdmin); d.addDiscoveryListener(listener); ArgumentCaptor result = ArgumentCaptor.forClass(DiscoveryResult.class); @@ -76,7 +78,8 @@ public void pingDeviceDetected() throws InterruptedException { @Test public void tcpDeviceDetected() throws InterruptedException { - NetworkDiscoveryService d = new NetworkDiscoveryService(null); + ConfigurationAdmin configAdmin = mock(ConfigurationAdmin.class); + NetworkDiscoveryService d = new NetworkDiscoveryService(configAdmin); d.addDiscoveryListener(listener); ArgumentCaptor result = ArgumentCaptor.forClass(DiscoveryResult.class); From 8664e053fdc714678a05ecc9150f4a32a58db450 Mon Sep 17 00:00:00 2001 From: Leo Siepel Date: Sat, 6 Sep 2025 12:41:00 +0200 Subject: [PATCH 35/35] Remove restart from doc Signed-off-by: Leo Siepel --- bundles/org.openhab.binding.network/README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bundles/org.openhab.binding.network/README.md b/bundles/org.openhab.binding.network/README.md index 10a44f042c6c7..0679257d5f9ce 100644 --- a/bundles/org.openhab.binding.network/README.md +++ b/bundles/org.openhab.binding.network/README.md @@ -14,7 +14,7 @@ The binding has the following configuration options: - **arpPingToolPath:** If the ARP ping tool is not called `arping` and cannot be found in the PATH environment variable, the absolute path can be configured here. Default is `arping`. - **cacheDeviceStateTimeInMS:** The result of a device presence detection is cached for a small amount of time. Set this time here in milliseconds. Be aware that no new pings will be issued within this time frame, even if explicitly requested. Default is 2000. - **preferResponseTimeAsLatency:** If enabled, an attempt will be made to extract the latency from the output of the ping command. If no such latency value is found in the ping command output, the time to execute the ping command is used as fallback latency. If disabled, the time to execute the ping command is always used as latency value. This is disabled by default to be backwards-compatible and to not break statistics and monitoring which existed before this feature. -- **numberOfDiscoveryThreads:** Specifies the number of threads to be used during the discovery process. Increasing this value may speed up the discovery of devices on large networks but could also increase the load on the system. Default is `100`. _note:_The binding needs a restart for this configuration parameter to have effect. +- **numberOfDiscoveryThreads:** Specifies the number of threads to be used during the discovery process. Increasing this value may speed up the discovery of devices on large networks but could also increase the load on the system. Default is `100`. Create a `/services/network.cfg` file and use the above options like this: