From d78843cfc3d2b3d686395cde73ed24e9f1f4e281 Mon Sep 17 00:00:00 2001 From: J-N-K Date: Wed, 17 Jan 2024 21:21:30 +0100 Subject: [PATCH] [http] Fix refresh time check and calculation (#16288) Signed-off-by: Jan N. Klug Signed-off-by: Ciprian Pascu --- .../http/internal/HttpThingHandler.java | 27 ++++++++------- .../internal/http/RefreshingUrlCache.java | 34 ++++++++++++------- .../http/RateLimitedHttpClientTest.java | 4 +-- .../binding/http/RefreshingUrlCacheTest.java | 6 ++-- 4 files changed, 40 insertions(+), 31 deletions(-) diff --git a/bundles/org.openhab.binding.http/src/main/java/org/openhab/binding/http/internal/HttpThingHandler.java b/bundles/org.openhab.binding.http/src/main/java/org/openhab/binding/http/internal/HttpThingHandler.java index 4ed71b5a727..8947852e110 100644 --- a/bundles/org.openhab.binding.http/src/main/java/org/openhab/binding/http/internal/HttpThingHandler.java +++ b/bundles/org.openhab.binding.http/src/main/java/org/openhab/binding/http/internal/HttpThingHandler.java @@ -168,15 +168,6 @@ public class HttpThingHandler extends BaseThingHandler implements HttpStatusList } rateLimitedHttpClient.setDelay(config.delay); - int urlHandlerCount = urlHandlers.size(); - if (urlHandlerCount * config.delay > config.refresh * 1000) { - // this should prevent the rate limit queue from filling up - config.refresh = (urlHandlerCount * config.delay) / 1000 + 1; - logger.warn( - "{} channels in thing {} with a delay of {} incompatible with the configured refresh time. Refresh-Time increased to the minimum of {}", - urlHandlerCount, thing.getUID(), config.delay, config.refresh); - } - // remove empty headers config.headers.removeIf(String::isBlank); @@ -236,6 +227,17 @@ public class HttpThingHandler extends BaseThingHandler implements HttpStatusList // create channels thing.getChannels().forEach(this::createChannel); + int urlHandlerCount = urlHandlers.size(); + if (urlHandlerCount * config.delay > config.refresh * 1000) { + // this should prevent the rate limit queue from filling up + config.refresh = (urlHandlerCount * config.delay) / 1000 + 1; + logger.warn( + "{} channels in thing {} with a delay of {} incompatible with the configured refresh time. Refresh-Time increased to the minimum of {}", + urlHandlerCount, thing.getUID(), config.delay, config.refresh); + } + + urlHandlers.values().forEach(urlHandler -> urlHandler.start(scheduler, config.refresh)); + updateStatus(ThingStatus.UNKNOWN); } @@ -330,9 +332,10 @@ public class HttpThingHandler extends BaseThingHandler implements HttpStatusList // we need a key consisting of stateContent and URL, only if both are equal, we can use the same cache String key = channelConfig.stateContent + "$" + stateUrl; channelUrls.put(channelUID, key); - Objects.requireNonNull(urlHandlers.computeIfAbsent(key, - k -> new RefreshingUrlCache(scheduler, rateLimitedHttpClient, stateUrl, config, - channelConfig.stateContent, config.contentType, this))) + Objects.requireNonNull( + urlHandlers.computeIfAbsent(key, + k -> new RefreshingUrlCache(rateLimitedHttpClient, stateUrl, config, + channelConfig.stateContent, config.contentType, this))) .addConsumer(itemValueConverter::process); } diff --git a/bundles/org.openhab.binding.http/src/main/java/org/openhab/binding/http/internal/http/RefreshingUrlCache.java b/bundles/org.openhab.binding.http/src/main/java/org/openhab/binding/http/internal/http/RefreshingUrlCache.java index d86fa1d798c..2045f34cdfd 100644 --- a/bundles/org.openhab.binding.http/src/main/java/org/openhab/binding/http/internal/http/RefreshingUrlCache.java +++ b/bundles/org.openhab.binding.http/src/main/java/org/openhab/binding/http/internal/http/RefreshingUrlCache.java @@ -59,12 +59,11 @@ public class RefreshingUrlCache { private final @Nullable String httpContentType; private final HttpStatusListener httpStatusListener; - private final ScheduledFuture future; + private @Nullable ScheduledFuture future; private @Nullable ChannelHandlerContent lastContent; - public RefreshingUrlCache(ScheduledExecutorService executor, RateLimitedHttpClient httpClient, String url, - HttpThingConfig thingConfig, String httpContent, @Nullable String httpContentType, - HttpStatusListener httpStatusListener) { + public RefreshingUrlCache(RateLimitedHttpClient httpClient, String url, HttpThingConfig thingConfig, + String httpContent, @Nullable String httpContentType, HttpStatusListener httpStatusListener) { this.httpClient = httpClient; this.url = url; this.strictErrorHandling = thingConfig.strictErrorHandling; @@ -76,9 +75,25 @@ public class RefreshingUrlCache { this.httpContentType = httpContentType; this.httpStatusListener = httpStatusListener; fallbackEncoding = thingConfig.encoding; + } - future = executor.scheduleWithFixedDelay(this::refresh, 1, thingConfig.refresh, TimeUnit.SECONDS); - logger.trace("Started refresh task for URL '{}' with interval {}s", url, thingConfig.refresh); + public void start(ScheduledExecutorService executor, int refreshTime) { + if (future != null) { + logger.warn("Starting refresh task requested but it is already started. This is bug."); + return; + } + future = executor.scheduleWithFixedDelay(this::refresh, 1, refreshTime, TimeUnit.SECONDS); + logger.trace("Started refresh task for URL '{}' with interval {}s", url, refreshTime); + } + + public void stop() { + // clearing all listeners to prevent further updates + consumers.clear(); + ScheduledFuture future = this.future; + if (future != null) { + future.cancel(true); + logger.trace("Stopped refresh task for URL '{}'", url); + } } private void refresh() { @@ -132,13 +147,6 @@ public class RefreshingUrlCache { } } - public void stop() { - // clearing all listeners to prevent further updates - consumers.clear(); - future.cancel(false); - logger.trace("Stopped refresh task for URL '{}'", url); - } - public void addConsumer(Consumer<@Nullable ChannelHandlerContent> consumer) { consumers.add(consumer); } diff --git a/bundles/org.openhab.binding.http/src/test/java/org/openhab/binding/http/RateLimitedHttpClientTest.java b/bundles/org.openhab.binding.http/src/test/java/org/openhab/binding/http/RateLimitedHttpClientTest.java index 4c73389df2a..659f4f33f85 100644 --- a/bundles/org.openhab.binding.http/src/test/java/org/openhab/binding/http/RateLimitedHttpClientTest.java +++ b/bundles/org.openhab.binding.http/src/test/java/org/openhab/binding/http/RateLimitedHttpClientTest.java @@ -99,7 +99,7 @@ public class RateLimitedHttpClientTest extends AbstractWireMockTest { assertThat((int) msBetween, allOf(greaterThanOrEqualTo(1000), lessThan(1100))); } - private List doLimitTest(int setDelay, List config) { + private void doLimitTest(int setDelay, List config) { stubFor(get(urlEqualTo(TEST_LOCATION)).willReturn(aResponse().withBody(TEST_CONTENT))); RateLimitedHttpClient rateLimitedHttpClient = new RateLimitedHttpClient(httpClient, scheduler); @@ -129,8 +129,6 @@ public class RateLimitedHttpClientTest extends AbstractWireMockTest { // wait until we got all results waitForAssert(() -> assertEquals(config.size(), responses.size())); rateLimitedHttpClient.shutdown(); - - return responses; } private static class Response { diff --git a/bundles/org.openhab.binding.http/src/test/java/org/openhab/binding/http/RefreshingUrlCacheTest.java b/bundles/org.openhab.binding.http/src/test/java/org/openhab/binding/http/RefreshingUrlCacheTest.java index e8aa8c165ab..98549d482bd 100644 --- a/bundles/org.openhab.binding.http/src/test/java/org/openhab/binding/http/RefreshingUrlCacheTest.java +++ b/bundles/org.openhab.binding.http/src/test/java/org/openhab/binding/http/RefreshingUrlCacheTest.java @@ -252,10 +252,10 @@ public class RefreshingUrlCacheTest extends AbstractWireMockTest { * @return the cache object */ private RefreshingUrlCache getUrlCache(String content) { - RefreshingUrlCache urlCache = new RefreshingUrlCache(scheduler, rateLimitedHttpClient, url, thingConfig, - content, null, statusListener); + RefreshingUrlCache urlCache = new RefreshingUrlCache(rateLimitedHttpClient, url, thingConfig, content, null, + statusListener); urlCache.addConsumer(contentWrappers::add); - + urlCache.start(scheduler, thingConfig.refresh); return urlCache; } }