[shelly] Refactor ShellyManagerCache cleanup (#19969)

* Do not create a thread to cleanup the ShellyManagerCache

Signed-off-by: Markus Michels <markus7017@gmail.com>
Co-authored-by: Ravi Nadahar <nadahar@rediffmail.com>

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/discovery/ShellyBasicDiscoveryService.java

@@ -151,6 +151,8 @@ public class ShellyBasicDiscoveryService extends AbstractDiscoveryService {
             if (result.isHttpAccessUnauthorized()) {
                 // create shellyunknown thing - will be changed during thing initialization with valid credentials
                 thingUID = ShellyThingCreator.getThingUIDForUnknown(name, model, mode);
+            } else {
+                logger.debug("{}: Unable to discover device: {}", name, e.getMessage());
             }
         } catch (IllegalArgumentException | IOException e) { // maybe some format description was buggy
             logger.debug("Discovery: Unable to discover thing", e);

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManager.java

@@ -13,7 +13,9 @@
 package org.openhab.binding.shelly.internal.manager;
 
 import static org.openhab.binding.shelly.internal.manager.ShellyManagerConstants.*;
+import static org.openhab.binding.shelly.internal.util.ShellyUtils.getString;
 
+import java.util.Collections;
 import java.util.LinkedHashMap;
 import java.util.Map;
 
@@ -22,9 +24,21 @@ import org.eclipse.jetty.client.HttpClient;
 import org.eclipse.jetty.http.HttpStatus;
 import org.openhab.binding.shelly.internal.ShellyHandlerFactory;
 import org.openhab.binding.shelly.internal.api.ShellyApiException;
+import org.openhab.binding.shelly.internal.manager.ShellyManagerPage.FwArchList;
+import org.openhab.binding.shelly.internal.manager.ShellyManagerPage.FwRepoEntry;
 import org.openhab.binding.shelly.internal.manager.ShellyManagerPage.ShellyMgrResponse;
 import org.openhab.binding.shelly.internal.provider.ShellyTranslationProvider;
+import org.openhab.core.io.net.http.HttpClientFactory;
+import org.openhab.core.net.HttpServiceUtil;
+import org.openhab.core.net.NetworkAddressService;
 import org.osgi.service.cm.ConfigurationAdmin;
+import org.osgi.service.component.ComponentContext;
+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.Reference;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 /**
  * {@link ShellyManager} implements the Shelly Manager
@@ -32,23 +46,47 @@ import org.osgi.service.cm.ConfigurationAdmin;
  * @author Markus Michels - Initial contribution
  */
 @NonNullByDefault
+@Component(service = ShellyManager.class)
 public class ShellyManager {
-    private final Map<String, ShellyManagerPage> pages = new LinkedHashMap<>();
+    private final Map<String, ShellyManagerPage> pages;
+    private final Logger logger = LoggerFactory.getLogger(ShellyManager.class);
+    private final ShellyManagerCache<String, FwRepoEntry> firmwareRepo;
+    private final ShellyManagerCache<String, FwArchList> firmwareArch;
+
+    @Activate
+    public ShellyManager(@Reference ConfigurationAdmin configurationAdmin,
+            @Reference NetworkAddressService networkAddressService, @Reference HttpClientFactory httpClientFactory,
+            @Reference ShellyHandlerFactory handlerFactory, @Reference ShellyTranslationProvider translationProvider,
+            ComponentContext componentContext) {
+        String localIp = getString(networkAddressService.getPrimaryIpv4HostAddress());
+        Integer localPort = HttpServiceUtil.getHttpServicePort(componentContext.getBundleContext());
+        HttpClient httpClient = httpClientFactory.getCommonHttpClient();
+        Map<String, ShellyManagerPage> pages = new LinkedHashMap<>();
+        firmwareRepo = new ShellyManagerCache<>();
+        firmwareArch = new ShellyManagerCache<>();
 
-    public ShellyManager(ConfigurationAdmin configurationAdmin, ShellyTranslationProvider translationProvider,
-            HttpClient httpClient, String localIp, int localPort, ShellyHandlerFactory handlerFactory) {
         pages.put(SHELLY_MGR_OVERVIEW_URI, new ShellyManagerOverviewPage(configurationAdmin, translationProvider,
-                httpClient, localIp, localPort, handlerFactory));
+                httpClient, localIp, localPort, handlerFactory, firmwareRepo, firmwareArch));
         pages.put(SHELLY_MGR_ACTION_URI, new ShellyManagerActionPage(configurationAdmin, translationProvider,
-                httpClient, localIp, localPort, handlerFactory));
+                httpClient, localIp, localPort, handlerFactory, firmwareRepo, firmwareArch));
         pages.put(SHELLY_MGR_FWUPDATE_URI, new ShellyManagerOtaPage(configurationAdmin, translationProvider, httpClient,
-                localIp, localPort, handlerFactory));
+                localIp, localPort, handlerFactory, firmwareRepo, firmwareArch));
         pages.put(SHELLY_MGR_OTA_URI, new ShellyManagerOtaPage(configurationAdmin, translationProvider, httpClient,
-                localIp, localPort, handlerFactory));
+                localIp, localPort, handlerFactory, firmwareRepo, firmwareArch));
         pages.put(SHELLY_MGR_IMAGES_URI, new ShellyManagerImageLoader(configurationAdmin, translationProvider,
-                httpClient, localIp, localPort, handlerFactory));
+                httpClient, localIp, localPort, handlerFactory, firmwareRepo, firmwareArch));
         pages.put(SHELLY_MANAGER_URI, new ShellyManagerOverviewPage(configurationAdmin, translationProvider, httpClient,
-                localIp, localPort, handlerFactory));
+                localIp, localPort, handlerFactory, firmwareRepo, firmwareArch));
+        this.pages = Collections.unmodifiableMap(pages);
+
+        // Promote Shelly Manager usage
+        logger.info("{}", translationProvider.get("status.managerstarted", localIp, localPort.toString()));
+    }
+
+    @Deactivate
+    protected void deactivate() {
+        firmwareRepo.dispose();
+        firmwareArch.dispose();
     }
 
     public ShellyMgrResponse generateContent(String path, Map<String, String[]> parameters) throws ShellyApiException {

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManagerActionPage.java

@@ -51,8 +51,10 @@ public class ShellyManagerActionPage extends ShellyManagerPage {
     private final Logger logger = LoggerFactory.getLogger(ShellyManagerActionPage.class);
 
     public ShellyManagerActionPage(ConfigurationAdmin configurationAdmin, ShellyTranslationProvider translationProvider,
-            HttpClient httpClient, String localIp, int localPort, ShellyHandlerFactory handlerFactory) {
-        super(configurationAdmin, translationProvider, httpClient, localIp, localPort, handlerFactory);
+            HttpClient httpClient, String localIp, int localPort, ShellyHandlerFactory handlerFactory,
+            ShellyManagerCache<String, FwRepoEntry> firmwareRepo, ShellyManagerCache<String, FwArchList> firmwareArch) {
+        super(configurationAdmin, translationProvider, httpClient, localIp, localPort, handlerFactory, firmwareRepo,
+                firmwareArch);
     }
 
     @Override

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManagerCache.java

@@ -12,13 +12,17 @@
  */
 package org.openhab.binding.shelly.internal.manager;
 
-import java.util.Date;
+import java.util.HashMap;
+import java.util.Iterator;
 import java.util.Map;
-import java.util.concurrent.ConcurrentHashMap;
+import java.util.Map.Entry;
+import java.util.concurrent.ScheduledExecutorService;
+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.shelly.internal.ShellyBindingConstants;
+import org.openhab.core.common.ThreadPoolManager;
 
 /**
  * {@link ShellyManagerCache} implements a cache with expiring times of the entries
@@ -26,82 +30,70 @@ import org.openhab.binding.shelly.internal.ShellyBindingConstants;
  * @author Markus Michels - Initial contribution
  */
 @NonNullByDefault
-public class ShellyManagerCache<K, V> extends ConcurrentHashMap<K, V> {
+public class ShellyManagerCache<K, V> {
+    protected final ScheduledExecutorService scheduler = ThreadPoolManager.getScheduledPool("ShellyManagerThreadpool");
+    private static final long EXPIRY_IN_MILLIS = 15 * 60 * 1000; // 15min
 
-    private static final long serialVersionUID = 1L;
+    private record CacheEntry<V> (Long created, V value) {
+    }
 
-    private Map<K, Long> timeMap = new ConcurrentHashMap<>();
-    private long expiryInMillis = ShellyManagerConstants.CACHE_TIMEOUT_DEF_MIN * 60 * 1000; // Default 1h
+    // Non-thread-safe HashMap: all access to 'storage' is synchronized on this instance
+    private final @NonNullByDefault({}) Map<K, CacheEntry<V>> storage = new HashMap<>();
+
+    // All access must be guarded by "this"
+    private @Nullable ScheduledFuture<?> cleanupJob;
 
     public ShellyManagerCache() {
-        initialize();
     }
 
-    public ShellyManagerCache(long expiryInMillis) {
-        this.expiryInMillis = expiryInMillis;
-        initialize();
+    @Nullable
+    public synchronized V get(K key) {
+        CacheEntry<V> entry = storage.get(key);
+        return entry == null ? null : entry.value;
     }
 
-    void initialize() {
-        new CleanerThread().start();
-    }
-
-    @Override
     public V put(K key, V value) {
-        Date date = new Date();
-        timeMap.put(key, date.getTime());
-        return super.put(key, value);
+        CacheEntry<V> entry = new CacheEntry<>(System.currentTimeMillis(), value);
+        synchronized (this) {
+            entry = storage.put(key, entry);
+            startJob(); // start background cleanup
+        }
+        return entry == null ? null : value;
     }
 
-    @Override
-    public void putAll(@Nullable Map<? extends K, ? extends V> m) {
-        if (m == null) {
-            throw new IllegalArgumentException();
-        }
-        for (K key : m.keySet()) {
-            @Nullable
-            V value = m.get(key);
-            if (key != null && value != null) { // don't allow null values
-                put(key, value);
-            }
+    private synchronized void startJob() {
+        if (cleanupJob == null) {
+            cleanupJob = scheduler.scheduleWithFixedDelay(this::cleanupMap, EXPIRY_IN_MILLIS, EXPIRY_IN_MILLIS,
+                    TimeUnit.MILLISECONDS);
         }
     }
 
-    @Override
-    public V putIfAbsent(K key, V value) {
-        if (!containsKey(key)) {
-            return put(key, value);
-        } else {
-            return get(key);
-        }
-    }
-
-    class CleanerThread extends Thread {
-
-        public CleanerThread() {
-            super(String.format("OH-binding-%s-%s", ShellyBindingConstants.BINDING_ID, "Cleaner"));
-        }
-
-        @Override
-        public void run() {
-            while (true) {
-                cleanMap();
-                try {
-                    Thread.sleep(expiryInMillis / 2);
-                } catch (InterruptedException e) {
+    private void cleanupMap() {
+        long currentTime = System.currentTimeMillis();
+        Entry<K, CacheEntry<V>> entry;
+        synchronized (this) {
+            for (Iterator<Entry<K, CacheEntry<V>>> iterator = storage.entrySet().iterator(); iterator.hasNext();) {
+                entry = iterator.next();
+                if (currentTime > (entry.getValue().created.longValue() + EXPIRY_IN_MILLIS)) {
+                    iterator.remove();
                 }
             }
-        }
 
-        private void cleanMap() {
-            long currentTime = new Date().getTime();
-            for (K key : timeMap.keySet()) {
-                Long timeValue = timeMap.get(key);
-                if (key != null && (timeValue == null || currentTime > (timeValue + expiryInMillis))) {
-                    remove(key);
-                    timeMap.remove(key);
-                }
+            if (storage.isEmpty()) {
+                cancelJob(); // stop background cleanup
             }
         }
     }
+
+    private synchronized void cancelJob() {
+        if (cleanupJob != null) {
+            cleanupJob.cancel(true);
+            cleanupJob = null;
+        }
+    }
+
+    public synchronized void dispose() {
+        cancelJob();
+        storage.clear();
+    }
 }

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManagerConstants.java

@@ -150,7 +150,4 @@ public class ShellyManagerConstants {
     public static final String FWREPO_TEST_URL = "https://repo.shelly.cloud/files/firmware/";
     public static final String FWREPO_ARCH_URL = "http://archive.shelly-tools.de/archive.php";
     public static final String FWREPO_ARCFILE_URL = "http://archive.shelly-tools.de/version/";
-
-    public static final int CACHE_TIMEOUT_DEF_MIN = 60; // Default timeout for cache entries
-    public static final int CACHE_TIMEOUT_FW_MIN = 15; // Cache entries for the firmware list 15min
 }

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManagerImageLoader.java

@@ -41,8 +41,10 @@ public class ShellyManagerImageLoader extends ShellyManagerPage {
 
     public ShellyManagerImageLoader(ConfigurationAdmin configurationAdmin,
             ShellyTranslationProvider translationProvider, HttpClient httpClient, String localIp, int localPort,
-            ShellyHandlerFactory handlerFactory) {
-        super(configurationAdmin, translationProvider, httpClient, localIp, localPort, handlerFactory);
+            ShellyHandlerFactory handlerFactory, ShellyManagerCache<String, FwRepoEntry> firmwareRepo,
+            ShellyManagerCache<String, FwArchList> firmwareArch) {
+        super(configurationAdmin, translationProvider, httpClient, localIp, localPort, handlerFactory, firmwareRepo,
+                firmwareArch);
     }
 
     @Override

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManagerOtaPage.java

@@ -55,8 +55,10 @@ public class ShellyManagerOtaPage extends ShellyManagerPage {
     protected final Logger logger = LoggerFactory.getLogger(ShellyManagerOtaPage.class);
 
     public ShellyManagerOtaPage(ConfigurationAdmin configurationAdmin, ShellyTranslationProvider translationProvider,
-            HttpClient httpClient, String localIp, int localPort, ShellyHandlerFactory handlerFactory) {
-        super(configurationAdmin, translationProvider, httpClient, localIp, localPort, handlerFactory);
+            HttpClient httpClient, String localIp, int localPort, ShellyHandlerFactory handlerFactory,
+            ShellyManagerCache<String, FwRepoEntry> firmwareRepo, ShellyManagerCache<String, FwArchList> firmwareArch) {
+        super(configurationAdmin, translationProvider, httpClient, localIp, localPort, handlerFactory, firmwareRepo,
+                firmwareArch);
     }
 
     @Override

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManagerOverviewPage.java

@@ -54,8 +54,10 @@ public class ShellyManagerOverviewPage extends ShellyManagerPage {
 
     public ShellyManagerOverviewPage(ConfigurationAdmin configurationAdmin,
             ShellyTranslationProvider translationProvider, HttpClient httpClient, String localIp, int localPort,
-            ShellyHandlerFactory handlerFactory) {
-        super(configurationAdmin, translationProvider, httpClient, localIp, localPort, handlerFactory);
+            ShellyHandlerFactory handlerFactory, ShellyManagerCache<String, FwRepoEntry> firmwareRepo,
+            ShellyManagerCache<String, FwArchList> firmwareArch) {
+        super(configurationAdmin, translationProvider, httpClient, localIp, localPort, handlerFactory, firmwareRepo,
+                firmwareArch);
     }
 
     @Override

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManagerPage.java

@@ -86,8 +86,8 @@ public class ShellyManagerPage {
     protected final Map<String, String> htmlTemplates = new HashMap<>();
     protected final Gson gson = new Gson();
 
-    protected final ShellyManagerCache<String, FwRepoEntry> firmwareRepo = new ShellyManagerCache<>(15 * 60 * 1000);
-    protected final ShellyManagerCache<String, FwArchList> firmwareArch = new ShellyManagerCache<>(15 * 60 * 1000);
+    protected final ShellyManagerCache<String, FwRepoEntry> firmwareRepo;
+    protected final ShellyManagerCache<String, FwArchList> firmwareArch;
 
     public static class ShellyMgrResponse {
         public @Nullable Object data = "";
@@ -145,13 +145,16 @@ public class ShellyManagerPage {
     }
 
     public ShellyManagerPage(ConfigurationAdmin configurationAdmin, ShellyTranslationProvider translationProvider,
-            HttpClient httpClient, String localIp, int localPort, ShellyHandlerFactory handlerFactory) {
+            HttpClient httpClient, String localIp, int localPort, ShellyHandlerFactory handlerFactory,
+            ShellyManagerCache<String, FwRepoEntry> firmwareRepo, ShellyManagerCache<String, FwArchList> firmwareArch) {
         this.configurationAdmin = configurationAdmin;
         this.resources = translationProvider;
         this.handlerFactory = handlerFactory;
         this.httpClient = httpClient;
         this.localIp = localIp;
         this.localPort = localPort;
+        this.firmwareRepo = firmwareRepo;
+        this.firmwareArch = firmwareArch;
     }
 
     public ShellyMgrResponse generateContent(String path, Map<String, String[]> parameters) throws ShellyApiException {
@@ -393,10 +396,11 @@ public class ShellyManagerPage {
 
     protected FwRepoEntry getFirmwareRepoEntry(String deviceType, String mode) throws ShellyApiException {
         logger.debug("ShellyManager: Load firmware list from {}", FWREPO_PROD_URL);
-        FwRepoEntry fw = null;
-        if (firmwareRepo.containsKey(deviceType)) {
-            fw = firmwareRepo.get(deviceType);
+        FwRepoEntry fw = firmwareRepo.get(deviceType);
+        if (fw != null) {
+            return fw;
         }
+
         String json = httpGet(FWREPO_PROD_URL); // returns a strange JSON format so we are parsing this manually
         String entry = substringBetween(json, "\"" + deviceType + "\":{", "}");
         if (!entry.isEmpty()) {
@@ -434,14 +438,10 @@ public class ShellyManagerPage {
     }
 
     protected FwArchList getFirmwareArchiveList(String deviceType) throws ShellyApiException {
-        FwArchList list;
         String json = "";
-
-        if (firmwareArch.contains(deviceType)) {
-            list = firmwareArch.get(deviceType); // return from cache
-            if (list != null) {
-                return list;
-            }
+        FwArchList list = firmwareArch.get(deviceType); // return from cache
+        if (list != null) {
+            return list;
         }
 
         try {

bundles/org.openhab.binding.shelly/src/main/java/org/openhab/binding/shelly/internal/manager/ShellyManagerServlet.java

@@ -13,7 +13,7 @@
 package org.openhab.binding.shelly.internal.manager;
 
 import static org.openhab.binding.shelly.internal.manager.ShellyManagerConstants.*;
-import static org.openhab.binding.shelly.internal.util.ShellyUtils.*;
+import static org.openhab.binding.shelly.internal.util.ShellyUtils.getString;
 
 import java.io.IOException;
 import java.io.OutputStream;
@@ -28,15 +28,8 @@ import javax.servlet.http.HttpServletResponse;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
-import org.openhab.binding.shelly.internal.ShellyHandlerFactory;
 import org.openhab.binding.shelly.internal.api.ShellyApiException;
 import org.openhab.binding.shelly.internal.manager.ShellyManagerPage.ShellyMgrResponse;
-import org.openhab.binding.shelly.internal.provider.ShellyTranslationProvider;
-import org.openhab.core.io.net.http.HttpClientFactory;
-import org.openhab.core.net.HttpServiceUtil;
-import org.openhab.core.net.NetworkAddressService;
-import org.osgi.service.cm.ConfigurationAdmin;
-import org.osgi.service.component.ComponentContext;
 import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
 import org.osgi.service.component.annotations.ConfigurationPolicy;
@@ -65,18 +58,10 @@ public class ShellyManagerServlet extends HttpServlet {
     private final String className;
 
     @Activate
-    public ShellyManagerServlet(@Reference ConfigurationAdmin configurationAdmin,
-            @Reference NetworkAddressService networkAddressService, @Reference HttpClientFactory httpClientFactory,
-            @Reference ShellyHandlerFactory handlerFactory, @Reference ShellyTranslationProvider translationProvider,
-            ComponentContext componentContext, Map<String, Object> config) {
-        className = substringAfterLast(getClass().toString(), ".");
-        String localIp = getString(networkAddressService.getPrimaryIpv4HostAddress());
-        Integer localPort = HttpServiceUtil.getHttpServicePort(componentContext.getBundleContext());
-        this.manager = new ShellyManager(configurationAdmin, translationProvider,
-                httpClientFactory.getCommonHttpClient(), localIp, localPort, handlerFactory);
-
-        // Promote Shelly Manager usage
-        logger.info("{}", translationProvider.get("status.managerstarted", localIp, localPort.toString()));
+    public ShellyManagerServlet(@Reference ShellyManager shellyManager) {
+        className = getClass().getSimpleName();
+        this.manager = shellyManager;
+        logger.debug("{} started", className);
     }
 
     @Deactivate