Skip to content

Commit 7499f08

Browse files
jimtngjimtng
authored
Replace remote‑addon ExpiringCache with explicit refresh to avoid expiry‑induced UI stalls (#5710)
The automatic 15‑minute cache expiry occasionally caused slow or stalled marketplace addon loads. Users can now request a refresh on demand, ensuring predictable behaviour and consistently responsive UI performance. Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
1 parent 0d0bf94 commit 7499f08

6 files changed

Lines changed: 43 additions & 64 deletions

File tree

‎bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/AbstractRemoteAddonService.java‎

Lines changed: 19 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -15,17 +15,16 @@
1515
import static org.openhab.core.common.ThreadPoolManager.THREAD_POOL_NAME_COMMON;
1616

1717
import java.io.IOException;
18-
import java.time.Duration;
1918
import java.util.ArrayList;
2019
import java.util.Comparator;
2120
import java.util.Dictionary;
2221
import java.util.HashMap;
23-
import java.util.HashSet;
2422
import java.util.List;
2523
import java.util.Locale;
2624
import java.util.Map;
2725
import java.util.Objects;
2826
import java.util.Set;
27+
import java.util.concurrent.CopyOnWriteArraySet;
2928
import java.util.concurrent.ScheduledExecutorService;
3029

3130
import org.eclipse.jdt.annotation.NonNullByDefault;
@@ -37,7 +36,6 @@
3736
import org.openhab.core.addon.AddonInfoRegistry;
3837
import org.openhab.core.addon.AddonService;
3938
import org.openhab.core.addon.AddonType;
40-
import org.openhab.core.cache.ExpiringCache;
4139
import org.openhab.core.common.ThreadPoolManager;
4240
import org.openhab.core.config.core.ConfigParser;
4341
import org.openhab.core.events.Event;
@@ -86,15 +84,14 @@ public abstract class AbstractRemoteAddonService implements AddonService {
8684
protected final BundleVersion coreVersion;
8785

8886
protected final Gson gson = new GsonBuilder().setDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSS'Z'").create();
89-
protected final Set<MarketplaceAddonHandler> addonHandlers = new HashSet<>();
87+
protected final Set<MarketplaceAddonHandler> addonHandlers = new CopyOnWriteArraySet<>();
9088
protected final Storage<String> installedAddonStorage;
9189
protected final EventPublisher eventPublisher;
9290
protected final ConfigurationAdmin configurationAdmin;
93-
protected final ExpiringCache<List<Addon>> cachedRemoteAddons = new ExpiringCache<>(Duration.ofMinutes(15),
94-
this::getRemoteAddons);
91+
protected volatile List<Addon> cachedRemoteAddons = List.of();
9592
protected final AddonInfoRegistry addonInfoRegistry;
96-
protected List<Addon> cachedAddons = List.of();
97-
protected List<String> installedAddonIds = List.of();
93+
protected volatile List<Addon> cachedAddons = List.of();
94+
protected volatile List<String> installedAddonIds = List.of();
9895

9996
private final Logger logger = LoggerFactory.getLogger(AbstractRemoteAddonService.class);
10097
private final ScheduledExecutorService scheduler = ThreadPoolManager.getScheduledPool(THREAD_POOL_NAME_COMMON);
@@ -124,10 +121,10 @@ private Addon convertFromStorage(Map.Entry<String, @Nullable String> entry) {
124121

125122
@Override
126123
public void refreshSource() {
127-
refreshSource(false);
124+
refreshSource(true);
128125
}
129126

130-
private void refreshSource(boolean installedOnly) {
127+
private synchronized void refreshSource(boolean fetchRemoteAddons) {
131128
if (!addonHandlers.stream().allMatch(MarketplaceAddonHandler::isReady)) {
132129
logger.debug("Add-on service '{}' tried to refresh source before add-on handlers ready. Exiting.",
133130
getClass());
@@ -149,7 +146,7 @@ private void refreshSource(boolean installedOnly) {
149146
logger.error(
150147
"Failed to read JSON database, trying to purge it. You might need to re-install {} from the '{}' service.",
151148
installedAddonStorage.getKeys(), getId());
152-
refreshSource(installedOnly);
149+
refreshSource(fetchRemoteAddons);
153150
return;
154151
}
155152

@@ -162,12 +159,16 @@ private void refreshSource(boolean installedOnly) {
162159
List<String> currentAddonIds = addons.stream().map(Addon::getUid).toList();
163160

164161
// get the remote addons
165-
if (!installedOnly && remoteEnabled()) {
166-
List<Addon> remoteAddons = Objects.requireNonNullElse(cachedRemoteAddons.getValue(), List.of());
167-
remoteAddons.stream().filter(a -> !currentAddonIds.contains(a.getUid())).forEach(addon -> {
162+
if (remoteEnabled()) {
163+
if (fetchRemoteAddons || cachedRemoteAddons.isEmpty()) {
164+
cachedRemoteAddons = List.copyOf(getRemoteAddons());
165+
}
166+
cachedRemoteAddons.stream().filter(a -> !currentAddonIds.contains(a.getUid())).forEach(addon -> {
168167
setInstalled(addon);
169168
addons.add(addon);
170169
});
170+
} else if (!cachedRemoteAddons.isEmpty()) {
171+
cachedRemoteAddons = List.of();
171172
}
172173

173174
// remove incompatible add-ons if not enabled
@@ -183,10 +184,10 @@ private void refreshSource(boolean installedOnly) {
183184
}
184185
}
185186

186-
cachedAddons = addons;
187+
cachedAddons = List.copyOf(addons);
187188
this.installedAddonIds = currentAddonIds;
188189

189-
if (!installedOnly && !missingAddons.isEmpty()) {
190+
if (!missingAddons.isEmpty()) {
190191
logger.info("Re-installing missing add-ons from remote repository: {}", missingAddons);
191192
scheduler.execute(() -> missingAddons.forEach(this::install));
192193
}
@@ -231,12 +232,6 @@ public List<Addon> getAddons(@Nullable Locale locale) {
231232
return cachedAddons;
232233
}
233234

234-
@Override
235-
public List<Addon> getAddons(@Nullable Locale locale, boolean installedOnly) {
236-
refreshSource(installedOnly);
237-
return cachedAddons;
238-
}
239-
240235
@Override
241236
public List<AddonType> getTypes(@Nullable Locale locale) {
242237
return AddonType.DEFAULT_TYPES;
@@ -256,8 +251,7 @@ public void install(String id) {
256251
handler.install(addon);
257252
addon.setInstalled(true);
258253
installedAddonStorage.put(id, gson.toJson(addon));
259-
cachedRemoteAddons.invalidateValue();
260-
refreshSource();
254+
refreshSource(false);
261255
postInstalledEvent(addon.getUid());
262256
} catch (MarketplaceHandlerException e) {
263257
postFailureEvent(addon.getUid(), e.getMessage());
@@ -284,8 +278,7 @@ public void uninstall(String id) {
284278
try {
285279
handler.uninstall(addon);
286280
installedAddonStorage.remove(id);
287-
cachedRemoteAddons.invalidateValue();
288-
refreshSource();
281+
refreshSource(false);
289282
postUninstalledEvent(addon.getUid());
290283
} catch (MarketplaceHandlerException e) {
291284
postFailureEvent(addon.getUid(), e.getMessage());

‎bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/internal/community/CommunityMarketplaceAddonService.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,6 @@ public void modified(@Nullable Map<String, Object> config) {
126126
this.showUnpublished = ConfigParser.valueAsOrElse(config.get(CONFIG_SHOW_UNPUBLISHED_ENTRIES_KEY),
127127
Boolean.class, false);
128128
this.enabled = ConfigParser.valueAsOrElse(config.get(CONFIG_ENABLED_KEY), Boolean.class, true);
129-
cachedRemoteAddons.invalidateValue();
130129
refreshSource();
131130
}
132131
}

‎bundles/org.openhab.core.addon.marketplace/src/main/java/org/openhab/core/addon/marketplace/internal/json/JsonAddonService.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,6 @@ public void modified(@Nullable Map<String, Object> config) {
9595
String urls = ConfigParser.valueAsOrElse(config.get(CONFIG_URLS), String.class, "");
9696
addonServiceUrls = Arrays.asList(urls.split("\\|")).stream().filter(this::isValidUrl).toList();
9797
showUnstable = ConfigParser.valueAsOrElse(config.get(CONFIG_SHOW_UNSTABLE), Boolean.class, false);
98-
cachedRemoteAddons.invalidateValue();
9998
refreshSource();
10099
}
101100
}

‎bundles/org.openhab.core.addon.marketplace/src/test/java/org/openhab/core/addon/marketplace/AbstractRemoteAddonServiceTest.java‎

Lines changed: 4 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,7 @@ public void testAddonIsReportedAsInstalledIfStorageEntryMissing() {
128128
}
129129

130130
@Test
131-
public void testInstalledAddonIsStillPresentAfterRemoteIsDisabledOrMissing() {
131+
public void testInstalledAddonIsStillPresentAfterRemoteIsDisabledOrMissingAfterRefresh() {
132132
addonService.setInstalled(TEST_ADDON);
133133
addonService.addToStorage(TEST_ADDON);
134134

@@ -139,24 +139,15 @@ public void testInstalledAddonIsStillPresentAfterRemoteIsDisabledOrMissing() {
139139
// disable remote repo
140140
properties.put(CONFIG_REMOTE_ENABLED, false);
141141

142+
// force refresh after changing settings
143+
addonService.refreshSource();
144+
142145
// check only the installed addon is present
143146
addons = addonService.getAddons(null);
144147
assertThat(addons, hasSize(1));
145148
assertThat(addons.getFirst().getUid(), is(getFullAddonId(TEST_ADDON)));
146149
}
147150

148-
@Test
149-
public void testInstalledOnlyAddonsDoNotTriggerRemoteLookup() {
150-
addonService.setInstalled(TEST_ADDON);
151-
addonService.addToStorage(TEST_ADDON);
152-
153-
List<Addon> addons = addonService.getAddons(null, true);
154-
assertThat(addons, hasSize(1));
155-
assertThat(addons.getFirst().getUid(), is(getFullAddonId(TEST_ADDON)));
156-
assertThat(addons.getFirst().isInstalled(), is(true));
157-
assertThat(addonService.getRemoteCalls(), is(0));
158-
}
159-
160151
@Test
161152
public void testIncompatibleAddonsNotIncludedByDefault() {
162153
assertThat(addonService.getAddons(null), hasSize(COMPATIBLE_ADDON_COUNT));

‎bundles/org.openhab.core.addon/src/main/java/org/openhab/core/addon/AddonService.java‎

Lines changed: 0 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -63,22 +63,6 @@ public interface AddonService {
6363
*/
6464
List<Addon> getAddons(@Nullable Locale locale);
6565

66-
/**
67-
* Retrieves add-ons, optionally restricted to installed ones only.
68-
*
69-
* Implementations that can avoid remote lookups when {@code installedOnly} is {@code true} should do so.
70-
*
71-
* @param locale the locale to use for the result
72-
* @param installedOnly {@code true} to return only installed add-ons
73-
* @return the localized add-ons
74-
*/
75-
default List<Addon> getAddons(@Nullable Locale locale, boolean installedOnly) {
76-
if (installedOnly) {
77-
return getAddons(locale).stream().filter(Addon::isInstalled).toList();
78-
}
79-
return getAddons(locale);
80-
}
81-
8266
/**
8367
* Retrieves the add-on for the given id.
8468
*

‎bundles/org.openhab.core.io.rest.core/src/main/java/org/openhab/core/io/rest/core/internal/addons/AddonResource.java‎

Lines changed: 20 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -156,22 +156,35 @@ protected void removeAddonService(AddonService featureService) {
156156
public Response getAddon(
157157
@HeaderParam("Accept-Language") @Parameter(description = "language") @Nullable String language,
158158
@QueryParam("serviceId") @Parameter(description = "service ID") @Nullable String serviceId,
159-
@QueryParam("installedOnly") @Parameter(description = "true to return only installed add-ons") @Nullable Boolean installedOnly) {
159+
@QueryParam("installedOnly") @Parameter(description = "true to return only installed add-ons") @Nullable Boolean installedOnly,
160+
@QueryParam("refresh") @Parameter(description = "true to refresh add-ons before returning them") @Nullable Boolean refresh) {
160161
logger.debug("Received HTTP GET request at '{}'", uriInfo.getPath());
161162

162163
final Locale locale = localeService.getLocale(language);
164+
boolean forceRefresh = Boolean.TRUE.equals(refresh);
163165

166+
Stream<Addon> addons;
164167
if ("all".equals(serviceId)) {
165-
return Response.ok(new Stream2JSONInputStream(getAllAddons(locale, Boolean.TRUE.equals(installedOnly))))
166-
.build();
168+
if (forceRefresh) {
169+
addonServices.forEach(AddonService::refreshSource);
170+
}
171+
addons = getAllAddons(locale);
167172
} else {
168173
AddonService addonService = (serviceId != null) ? getServiceById(serviceId) : getDefaultService();
169174
if (addonService == null) {
170175
return Response.status(HttpStatus.NOT_FOUND_404).build();
171176
}
172-
return Response.ok(new Stream2JSONInputStream(
173-
addonService.getAddons(locale, Boolean.TRUE.equals(installedOnly)).stream())).build();
177+
if (forceRefresh) {
178+
addonService.refreshSource();
179+
}
180+
addons = addonService.getAddons(locale).stream();
174181
}
182+
183+
if (Boolean.TRUE.equals(installedOnly)) {
184+
addons = addons.filter(Addon::isInstalled);
185+
}
186+
187+
return Response.ok(new Stream2JSONInputStream(addons)).build();
175188
}
176189

177190
@GET
@@ -416,8 +429,8 @@ private void postFailureEvent(String addonId, @Nullable String msg) {
416429
.findFirst().orElse(addonServices.stream().findFirst().orElse(null));
417430
}
418431

419-
private Stream<Addon> getAllAddons(Locale locale, boolean installedOnly) {
420-
return addonServices.stream().map(s -> s.getAddons(locale, installedOnly)).flatMap(Collection::stream);
432+
private Stream<Addon> getAllAddons(Locale locale) {
433+
return addonServices.stream().map(s -> s.getAddons(locale)).flatMap(Collection::stream);
421434
}
422435

423436
private Set<AddonType> getAllAddonTypes(Locale locale) {

0 commit comments

Comments
 (0)