Merge pull request #5120 from MattGill98/QACI-625

QACI-625 MP Config Cache Concurrency Issues
payara · Feb 5, 2021 · 2e573a7 · 2e573a7
2 parents 2a2faf3 + 1b7ef10
commit 2e573a7
Show file tree

Hide file tree

Showing 2 changed files with 40 additions and 18 deletions.
diff --git a/...onfig-service/src/main/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfig.java b/...onfig-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<Class<?>, Converter<?>> converters;
     private final long defaultCacheDurationSeconds;
 
-    private final Object cacheMapLock = new Object();
-    private final Map<String, CacheEntry> cachedValuesByProperty = new HashMap<>();
+    private final Map<String, CacheEntry> cachedValuesByProperty = new ConcurrentHashMap<>();
 
     private volatile Long configuredCacheValue = null;
     private final Object configuredCacheValueLock = new Object();
 
-    private String profile;
+    private final String profile;
 
     public PayaraConfig(List<ConfigSource> sources, Map<Class<?>, 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> T convertValue(ConfigValue configValue, String defaultValue,
@@ -288,9 +285,7 @@ public <T> Optional<Converter<T>> getConverter(Class<T> propertyType) {
     }
 
     public void clearCache() {
-        synchronized(cacheMapLock) {
-            cachedValuesByProperty.clear();
-        }
+        cachedValuesByProperty.clear();
     }
 
     private <E> Optional<Converter<Object>> createArrayConverter(Class<E> elementType) {

diff --git a/...g-service/src/test/java/fish/payara/nucleus/microprofile/config/spi/PayaraConfigTest.java b/...g-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<Throwable> 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");