From 087aeaf14cb90aa1b05d3c938145dc5eab8b566a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Blanchard?= Date: Tue, 23 Dec 2025 17:48:47 +0100 Subject: [PATCH 1/7] feat: ensure that healthchecks calls can be performed concurrently and add a 1s cache for performance or DoS issues. --- .../unomi/healthcheck/HealthCheckService.java | 86 ++++++++++++------- 1 file changed, 56 insertions(+), 30 deletions(-) diff --git a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java index 9da585e14..f7da22e85 100644 --- a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java +++ b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java @@ -28,7 +28,6 @@ import javax.servlet.ServletException; import java.util.*; import java.util.concurrent.*; -import java.util.stream.Collectors; import static org.apache.unomi.healthcheck.HealthCheckConfig.CONFIG_AUTH_REALM; @@ -42,8 +41,12 @@ public class HealthCheckService { private static final Logger LOGGER = LoggerFactory.getLogger(HealthCheckService.class.getName()); private final List providers = new ArrayList<>(); + private final Object cacheLock = new Object(); + private volatile long cacheTimestamp = 0L; + private volatile List healthCache = Collections.emptyList(); + private volatile boolean initialized = false; + private volatile boolean busy = false; private ExecutorService executor; - private boolean busy = false; private boolean registered = false; @Reference @@ -58,7 +61,7 @@ public HealthCheckService() { @Activate public void activate() throws ServletException, NamespaceException { LOGGER.info("Activating healthcheck service..."); - executor = Executors.newSingleThreadExecutor(); + executor = Executors.newCachedThreadPool(); if (!registered) { setConfig(config); } @@ -114,37 +117,60 @@ protected void unbind(HealthCheckProvider provider) { providers.remove(provider); } - public List check() throws RejectedExecutionException { - if (config !=null && config.isEnabled()) { - LOGGER.debug("Health check called"); - if (busy) { - throw new RejectedExecutionException("Health check already in progress"); - } else { - try { - busy = true; - List health = new ArrayList<>(); - health.add(HealthCheckResponse.live("karaf")); - for (HealthCheckProvider provider : providers.stream().filter(p -> config.getEnabledProviders().contains(p.name())).collect(Collectors.toList())) { - Future future = executor.submit(provider::execute); - try { - HealthCheckResponse response = future.get(config.getTimeout(), TimeUnit.MILLISECONDS); - health.add(response); - } catch (TimeoutException e) { - future.cancel(true); - health.add(provider.timeout()); - } catch (Exception e) { - LOGGER.error("Error while executing health check", e); - } + public List check() { + if (config == null || !config.isEnabled()) { + LOGGER.info("Healthcheck service is disabled"); + return Collections.emptyList(); + } + if (!initialized) { + synchronized (cacheLock) { + if (!initialized) { + refreshCacheSync(); + initialized = true; + } + } + } else if (shouldRefreshCache()) { + if (!busy) { + synchronized (cacheLock) { + if (!busy) { + busy = true; + executor.submit(() -> { + try { + refreshCacheSync(); + } finally { + busy = false; + } + }); } - return health; - } finally { - busy = false; } } - } else { - LOGGER.info("Healthcheck service is disabled"); - return Collections.emptyList(); } + return healthCache; + } + + private boolean shouldRefreshCache() { + return healthCache.isEmpty() || (System.currentTimeMillis() - cacheTimestamp) > 1000; } + private void refreshCacheSync() { + try { + List health = new ArrayList<>(); + health.add(HealthCheckResponse.live("karaf")); + for (HealthCheckProvider provider : providers.stream() + .filter(p -> config.getEnabledProviders().contains(p.name())) + .toList()) { + try { + HealthCheckResponse response = provider.execute(); + health.add(response); + } catch (Exception e) { + LOGGER.error("Error while executing health check", e); + health.add(provider.timeout()); + } + } + healthCache = health; + cacheTimestamp = System.currentTimeMillis(); + } catch (Exception e) { + LOGGER.error("Error refreshing health cache", e); + } + } } From 7f828d8534bef69f4b8316e500be0d3fa26a2ef3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Blanchard?= Date: Tue, 23 Dec 2025 17:56:03 +0100 Subject: [PATCH 2/7] feat: improve refresh conditions --- .../unomi/healthcheck/HealthCheckService.java | 24 +++++++++---------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java index f7da22e85..41bdb9135 100644 --- a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java +++ b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java @@ -130,18 +130,16 @@ public List check() { } } } else if (shouldRefreshCache()) { - if (!busy) { - synchronized (cacheLock) { - if (!busy) { - busy = true; - executor.submit(() -> { - try { - refreshCacheSync(); - } finally { - busy = false; - } - }); - } + synchronized (cacheLock) { + if (!busy) { + busy = true; + executor.submit(() -> { + try { + refreshCacheSync(); + } finally { + busy = false; + } + }); } } } @@ -149,7 +147,7 @@ public List check() { } private boolean shouldRefreshCache() { - return healthCache.isEmpty() || (System.currentTimeMillis() - cacheTimestamp) > 1000; + return !busy && (System.currentTimeMillis() - cacheTimestamp) > 1000; } private void refreshCacheSync() { From d50fa685296aba3e7842768a8fabd5ddfd84176b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Blanchard?= Date: Tue, 30 Dec 2025 11:16:11 +0100 Subject: [PATCH 3/7] test: add healthcheck test for concurrency. --- .../unomi/healthcheck/HealthCheckService.java | 26 ++++++--------- .../apache/unomi/itests/HealthCheckIT.java | 32 +++++++++++++++++++ 2 files changed, 42 insertions(+), 16 deletions(-) diff --git a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java index 41bdb9135..7f6d94e58 100644 --- a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java +++ b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java @@ -26,8 +26,9 @@ import org.slf4j.LoggerFactory; import javax.servlet.ServletException; -import java.util.*; -import java.util.concurrent.*; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; import static org.apache.unomi.healthcheck.HealthCheckConfig.CONFIG_AUTH_REALM; @@ -46,7 +47,6 @@ public class HealthCheckService { private volatile List healthCache = Collections.emptyList(); private volatile boolean initialized = false; private volatile boolean busy = false; - private ExecutorService executor; private boolean registered = false; @Reference @@ -61,7 +61,6 @@ public HealthCheckService() { @Activate public void activate() throws ServletException, NamespaceException { LOGGER.info("Activating healthcheck service..."); - executor = Executors.newCachedThreadPool(); if (!registered) { setConfig(config); } @@ -101,9 +100,6 @@ public void deactivate() { httpService.unregister("/health/check"); registered = false; } - if (executor != null) { - executor.shutdown(); - } } @Reference(service = HealthCheckProvider.class, cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC, unbind = "unbind") @@ -125,7 +121,7 @@ public List check() { if (!initialized) { synchronized (cacheLock) { if (!initialized) { - refreshCacheSync(); + refreshCache(); initialized = true; } } @@ -133,13 +129,11 @@ public List check() { synchronized (cacheLock) { if (!busy) { busy = true; - executor.submit(() -> { - try { - refreshCacheSync(); - } finally { - busy = false; - } - }); + try { + refreshCache(); + } finally { + busy = false; + } } } } @@ -150,7 +144,7 @@ private boolean shouldRefreshCache() { return !busy && (System.currentTimeMillis() - cacheTimestamp) > 1000; } - private void refreshCacheSync() { + private void refreshCache() { try { List health = new ArrayList<>(); health.add(HealthCheckResponse.live("karaf")); diff --git a/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java b/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java index 755d184c2..3d62fa415 100644 --- a/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java @@ -33,8 +33,10 @@ import org.slf4j.LoggerFactory; import java.io.IOException; +import java.util.ArrayList; import java.util.List; import java.util.Map; +import java.util.concurrent.*; import static org.junit.Assert.fail; @@ -68,6 +70,36 @@ public void testHealthCheck() { } } + @Test + public void testConcurrentHealthCheck() { + final int NB_THREADS = 10; + final int NB_ITERATIONS = 20; + + try (ExecutorService executorService = Executors.newFixedThreadPool(NB_THREADS)) { + List>> futures = new ArrayList<>(); + for (int i = 0; i < NB_ITERATIONS; i++) { + for (int j = 0; j < NB_THREADS; j++) { + Future> future = executorService.submit(() -> get(HEALTHCHECK_ENDPOINT, new TypeReference<>() {})); + futures.add(future); + } + for (Future> future : futures) { + List health = future.get(10, TimeUnit.SECONDS); + Assert.assertEquals(4, health.size()); + Assert.assertTrue(health.stream().anyMatch(r -> r.getName().equals("karaf") && r.getStatus() == HealthCheckResponse.Status.LIVE)); + Assert.assertTrue(health.stream().anyMatch(r -> r.getName().equals(searchEngine) && r.getStatus() == HealthCheckResponse.Status.LIVE)); + Assert.assertTrue(health.stream().anyMatch(r -> r.getName().equals("unomi") && r.getStatus() == HealthCheckResponse.Status.LIVE)); + Assert.assertTrue(health.stream().anyMatch(r -> r.getName().equals("cluster") && r.getStatus() == HealthCheckResponse.Status.LIVE)); + } + Thread.sleep(10); + } + executorService.shutdown(); + Assert.assertTrue(executorService.awaitTermination(10, TimeUnit.SECONDS)); + } catch (Exception e) { + LOGGER.error("Error while executing concurrent health check", e); + fail("Error while executing concurrent health check: " + e.getMessage()); + } + } + protected T get(final String url, TypeReference typeReference) { CloseableHttpResponse response = null; try { From 94336a34cd3c96a1bc689180a48b03b1beb6c1cc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Blanchard?= Date: Tue, 30 Dec 2025 11:31:22 +0100 Subject: [PATCH 4/7] test: add healthcheck test for concurrency. --- .../test/java/org/apache/unomi/itests/HealthCheckIT.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java b/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java index 3d62fa415..368be7c09 100644 --- a/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java @@ -75,7 +75,9 @@ public void testConcurrentHealthCheck() { final int NB_THREADS = 10; final int NB_ITERATIONS = 20; - try (ExecutorService executorService = Executors.newFixedThreadPool(NB_THREADS)) { + ExecutorService executorService = null; + try { + executorService = Executors.newFixedThreadPool(NB_THREADS); List>> futures = new ArrayList<>(); for (int i = 0; i < NB_ITERATIONS; i++) { for (int j = 0; j < NB_THREADS; j++) { @@ -97,6 +99,10 @@ public void testConcurrentHealthCheck() { } catch (Exception e) { LOGGER.error("Error while executing concurrent health check", e); fail("Error while executing concurrent health check: " + e.getMessage()); + } finally { + if ( executorService != null ) { + executorService.shutdownNow(); + } } } From 8cdb73e73b5f103a98e29e05cfde46cdbfe3cbc4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Blanchard?= Date: Tue, 30 Dec 2025 17:19:23 +0100 Subject: [PATCH 5/7] test: fix list sort to avoid concurrent modification exception --- .../org/apache/unomi/healthcheck/HealthCheckService.java | 8 +++++--- .../unomi/healthcheck/servlet/HealthCheckServlet.java | 2 -- .../test/java/org/apache/unomi/itests/HealthCheckIT.java | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java index 7f6d94e58..08c679b4b 100644 --- a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java +++ b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/HealthCheckService.java @@ -28,6 +28,7 @@ import javax.servlet.ServletException; import java.util.ArrayList; import java.util.Collections; +import java.util.Comparator; import java.util.List; import static org.apache.unomi.healthcheck.HealthCheckConfig.CONFIG_AUTH_REALM; @@ -115,7 +116,7 @@ protected void unbind(HealthCheckProvider provider) { public List check() { if (config == null || !config.isEnabled()) { - LOGGER.info("Healthcheck service is disabled"); + LOGGER.warn("Healthcheck service is disabled"); return Collections.emptyList(); } if (!initialized) { @@ -130,7 +131,7 @@ public List check() { if (!busy) { busy = true; try { - refreshCache(); + refreshCache(); } finally { busy = false; } @@ -159,7 +160,8 @@ private void refreshCache() { health.add(provider.timeout()); } } - healthCache = health; + health.sort(Comparator.comparing(HealthCheckResponse::getName)); + healthCache = List.copyOf(health); cacheTimestamp = System.currentTimeMillis(); } catch (Exception e) { LOGGER.error("Error refreshing health cache", e); diff --git a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/servlet/HealthCheckServlet.java b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/servlet/HealthCheckServlet.java index 27dec31d4..b25deb56c 100644 --- a/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/servlet/HealthCheckServlet.java +++ b/extensions/healthcheck/src/main/java/org/apache/unomi/healthcheck/servlet/HealthCheckServlet.java @@ -32,7 +32,6 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; import java.io.IOException; -import java.util.Comparator; import java.util.List; /** @@ -66,7 +65,6 @@ protected void service(HttpServletRequest request, HttpServletResponse response) return; } List checks = service.check(); - checks.sort(Comparator.comparing(HealthCheckResponse::getName)); response.getWriter().println(mapper.writeValueAsString(checks)); response.setContentType("application/json"); response.setHeader("Cache-Control", "no-cache"); diff --git a/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java b/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java index 368be7c09..882259525 100644 --- a/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/HealthCheckIT.java @@ -102,7 +102,7 @@ public void testConcurrentHealthCheck() { } finally { if ( executorService != null ) { executorService.shutdownNow(); - } + } } } From 3f88986c6e810ea5a9574f474d502070018548b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Blanchard?= Date: Wed, 31 Dec 2025 09:51:00 +0100 Subject: [PATCH 6/7] doc: fix forbidden html tag h3 in javadoc --- .../shell/services/internal/UnomiManagementServiceImpl.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/shell-commands/src/main/java/org/apache/unomi/shell/services/internal/UnomiManagementServiceImpl.java b/tools/shell-commands/src/main/java/org/apache/unomi/shell/services/internal/UnomiManagementServiceImpl.java index 87a89c1b1..a5881c37e 100644 --- a/tools/shell-commands/src/main/java/org/apache/unomi/shell/services/internal/UnomiManagementServiceImpl.java +++ b/tools/shell-commands/src/main/java/org/apache/unomi/shell/services/internal/UnomiManagementServiceImpl.java @@ -50,7 +50,7 @@ * to dynamically adjust its behavior based on external configurations. It leverages the {@link FeaturesService} to * manage Karaf features dynamically.

* - *

Configuration

+ *

Configuration

*

The service reads its configuration from the OSGi Configuration Admin under the PID org.apache.unomi.start. * The configuration includes:

*
    @@ -58,14 +58,14 @@ * in the format persistenceImplementation:feature1,feature2. *
* - *

Usage

+ *

Usage

*

This service can be controlled programmatically through its methods:

*
    *
  • {@link #startUnomi(String, boolean)}: Installs and starts features for the specified start features configuration.
    • *
  • {@link #stopUnomi()}: Stops and uninstalls the previously started features.
    • *
* - *

Dependencies

+ *

Dependencies

*

The following dependencies are required for this service:

*
    *
  • {@link MigrationService}: Handles migration tasks during startup.
    • From 31223f320da99999689fe22eb53e4fc6b2370d12 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?J=C3=A9r=C3=B4me=20Blanchard?= Date: Wed, 31 Dec 2025 11:42:48 +0100 Subject: [PATCH 7/7] test: improve log for test in send Event --- .../apache/unomi/itests/CopyPropertiesActionIT.java | 10 +++++++--- .../java/org/apache/unomi/itests/ProfileMergeIT.java | 11 ++++++++++- .../org/apache/unomi/itests/SendEventActionIT.java | 10 +++++++++- 3 files changed, 26 insertions(+), 5 deletions(-) diff --git a/itests/src/test/java/org/apache/unomi/itests/CopyPropertiesActionIT.java b/itests/src/test/java/org/apache/unomi/itests/CopyPropertiesActionIT.java index 6acde9fe2..7d01aaf57 100644 --- a/itests/src/test/java/org/apache/unomi/itests/CopyPropertiesActionIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/CopyPropertiesActionIT.java @@ -22,6 +22,7 @@ import org.apache.unomi.api.Profile; import org.apache.unomi.api.PropertyType; import org.apache.unomi.api.rules.Rule; +import org.apache.unomi.api.services.EventService; import org.apache.unomi.persistence.spi.CustomObjectMapper; import org.junit.After; import org.junit.Assert; @@ -166,10 +167,13 @@ private Event sendCopyPropertyEvent(Map properties, String profi Event event = new Event("copyProperties", null, profile, null, null, profile, new Date()); event.setPersistent(false); - event.setProperty("urlParameters", properties); - - eventService.send(event); + int result = eventService.send(event); + LOGGER.info("Event processing result: {}", result); + if (result == EventService.ERROR) { + LOGGER.error("Event processing resulted in ERROR. Event details: {}", event); + } + Assert.assertNotEquals(EventService.ERROR, result); return event; } diff --git a/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java b/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java index be82d3109..72576a847 100644 --- a/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/ProfileMergeIT.java @@ -20,6 +20,7 @@ import org.apache.unomi.api.actions.Action; import org.apache.unomi.api.conditions.Condition; import org.apache.unomi.api.rules.Rule; +import org.apache.unomi.api.services.EventService; import org.junit.After; import org.junit.Assert; import org.junit.Test; @@ -27,6 +28,8 @@ import org.ops4j.pax.exam.junit.PaxExam; import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy; import org.ops4j.pax.exam.spi.reactors.PerSuite; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.*; @@ -36,6 +39,8 @@ @RunWith(PaxExam.class) @ExamReactorStrategy(PerSuite.class) public class ProfileMergeIT extends BaseIT { + private final static Logger LOGGER = LoggerFactory.getLogger(ProfileMergeIT.class); + public static final String PERSONALIZATION_STRATEGY_STATUS = "personalizationStrategyStatus"; public static final String PERSONALIZATION_STRATEGY_STATUS_ID = "personalizationId"; public static final String PERSONALIZATION_STRATEGY_STATUS_IN_CTRL_GROUP = "inControlGroup"; @@ -468,7 +473,11 @@ private Event sendEvent() { profile.setProperty("j:nodename", "michel"); profile.getSystemProperties().put("mergeIdentifier", "jose"); Event testEvent = new Event(TEST_EVENT_TYPE, null, profile, null, null, profile, new Date()); - eventService.send(testEvent); + int result = eventService.send(testEvent); + LOGGER.info("Event processing result: {}", result); + if (result == EventService.ERROR) { + LOGGER.error("Event processing resulted in ERROR. Event details: {}", testEvent); + } return testEvent; } diff --git a/itests/src/test/java/org/apache/unomi/itests/SendEventActionIT.java b/itests/src/test/java/org/apache/unomi/itests/SendEventActionIT.java index 821158f19..3a2086f1b 100644 --- a/itests/src/test/java/org/apache/unomi/itests/SendEventActionIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/SendEventActionIT.java @@ -23,6 +23,7 @@ import org.apache.unomi.api.actions.Action; import org.apache.unomi.api.conditions.Condition; import org.apache.unomi.api.rules.Rule; +import org.apache.unomi.api.services.EventService; import org.junit.After; import org.junit.Assert; import org.junit.Test; @@ -30,6 +31,8 @@ import org.ops4j.pax.exam.junit.PaxExam; import org.ops4j.pax.exam.spi.reactors.ExamReactorStrategy; import org.ops4j.pax.exam.spi.reactors.PerSuite; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; import java.util.Collections; import java.util.Date; @@ -38,6 +41,7 @@ @RunWith(PaxExam.class) @ExamReactorStrategy(PerSuite.class) public class SendEventActionIT extends BaseIT { + private final static Logger LOGGER = LoggerFactory.getLogger(SendEventActionIT.class); private final static String TEST_RULE_ID = "sendEventTest"; private final static String EVENT_ID = "sendEventTestId"; @@ -80,7 +84,11 @@ private Event sendEvent() { profile.setProperty("j:nodename", "michel"); Event testEvent = new Event(TEST_EVENT_TYPE, null, profile, null, null, profile, new Date()); testEvent.setItemId(EVENT_ID); - eventService.send(testEvent); + int result = eventService.send(testEvent); + LOGGER.info("Event processing result: {}", result); + if (result == EventService.ERROR) { + LOGGER.error("Event processing resulted in ERROR. Event details: {}", testEvent); + } return testEvent; }