From a8ae8b29a5057779aaa834f3bc274b5352a52405 Mon Sep 17 00:00:00 2001 From: Matt Gill Date: Fri, 5 Feb 2021 14:10:35 +0000 Subject: [PATCH] Merge pull request #5120 from MattGill98/QACI-625 QACI-625 MP Config Cache Concurrency Issues --- .../microprofile/config/spi/PayaraConfig.java | 29 ++++++++----------- .../config/spi/PayaraConfigTest.java | 29 ++++++++++++++++++- 2 files changed, 40 insertions(+), 18 deletions(-) diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfig.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfig.java index 1416013baca..d74a605f677 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfig.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfig.java @@ -52,7 +52,6 @@ import java.lang.reflect.Array; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.List; @@ -77,6 +76,7 @@ public class PayaraConfig implements Config { private static final String MP_CONFIG_CACHE_DURATION = "mp.config.cache.duration"; private static final String MP_CONFIG_EXPANSION_ENABLED_STRING = "mp.config.property.expressions.enabled"; + private static final String MP_CONFIG_PROFILE_NAME_STRING = "mp.config.profile"; private static final class CacheEntry { final ConfigValueImpl value; @@ -92,21 +92,20 @@ private static final class CacheEntry { private final Map, Converter> converters; private final long defaultCacheDurationSeconds; - private final Object cacheMapLock = new Object(); - private final Map cachedValuesByProperty = new HashMap<>(); + private final Map cachedValuesByProperty = new ConcurrentHashMap<>(); private volatile Long configuredCacheValue = null; private final Object configuredCacheValueLock = new Object(); - private String profile; + private final String profile; public PayaraConfig(List sources, Map, Converter> converters, long defaultCacheDurationSeconds) { this.sources = sources; this.converters = new ConcurrentHashMap<>(converters); this.defaultCacheDurationSeconds = defaultCacheDurationSeconds; Collections.sort(sources, new ConfigSourceComparator()); - - profile = getConfigValue("mp.config.profile").getValue(); + + profile = getConfigValue(MP_CONFIG_PROFILE_NAME_STRING).getValue(); } @SuppressWarnings("unchecked") @@ -226,14 +225,12 @@ protected ConfigValueImpl getConfigValue(String propertyName, String cacheKey, L final String entryKey = cacheKey + (defaultValue != null ? ":" + defaultValue : "") + ":" + (entryTTL / 1000) + "s"; final long now = currentTimeMillis(); - synchronized(cacheMapLock) { - return cachedValuesByProperty.compute(entryKey, (key, entry) -> { - if (entry != null && now < entry.expires) { - return entry; - } - return new CacheEntry(searchConfigSources(finalPropertyName, finalDefaultValue), entryTTL); - }).value; - } + return cachedValuesByProperty.compute(entryKey, (key, entry) -> { + if (entry != null && now < entry.expires) { + return entry; + } + return new CacheEntry(searchConfigSources(finalPropertyName, finalDefaultValue), entryTTL); + }).value; } private T convertValue(ConfigValue configValue, String defaultValue, @@ -288,9 +285,7 @@ public Optional> getConverter(Class propertyType) { } public void clearCache() { - synchronized(cacheMapLock) { - cachedValuesByProperty.clear(); - } + cachedValuesByProperty.clear(); } private Optional> createArrayConverter(Class elementType) { diff --git a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfigTest.java b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfigTest.java index bad99131f37..f152d25da29 100644 --- a/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfigTest.java +++ b/nucleus/payara-modules/nucleus-microprofile/config-service/src/test/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfigTest.java @@ -46,7 +46,7 @@ import static java.util.Collections.emptyMap; import static org.junit.Assert.assertArrayEquals; import static org.junit.Assert.assertEquals; -import static org.junit.Assert.assertTrue; +import static org.junit.Assert.assertNull; import java.lang.annotation.ElementType; import java.lang.reflect.Array; @@ -55,6 +55,10 @@ import java.util.HashSet; import java.util.NoSuchElementException; import java.util.Optional; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import org.eclipse.microprofile.config.Config; import org.eclipse.microprofile.config.ConfigValue; @@ -105,6 +109,29 @@ public void configValueTest() { assertEquals(100, value.getSourceOrdinal()); } + /** + * Introduced by QACI-625. Not reproducible on all machines, but this aims to catch + * issues caused by high concurrency in the cache map + */ + @Test + public void concurrentConfigValueTest() throws InterruptedException { + ExecutorService exec = Executors.newFixedThreadPool(1000); + AtomicReference failure = new AtomicReference<>(null); + for (int i = 0; i < 100000; i++) { + exec.submit(() -> { + try { + config.getConfigValue("mp.config.profile").getValue(); + } catch (Throwable t) { + failure.set(t); + throw (RuntimeException) t; + } + }); + } + exec.shutdown(); + exec.awaitTermination(60, TimeUnit.SECONDS); + assertNull("stress test failed", failure.get()); + } + @Test public void stringValuesAreCached() throws InterruptedException { assertCachedValue(source1, "key1", String.class, "value1", "changed1");