From ed35d1f6a0b955fb50aaadd90d2c70d921c93e44 Mon Sep 17 00:00:00 2001 From: Yongkoo Kang Date: Thu, 18 Jan 2024 16:06:31 -0800 Subject: [PATCH 1/3] Fix race condition on propertyUsageMap via AtomicReference --- .../instrumentation/AccessMonitorUtil.java | 27 +++++++++---------- 1 file changed, 13 insertions(+), 14 deletions(-) diff --git a/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java b/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java index 20713054..3b191d67 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java @@ -16,6 +16,7 @@ import java.util.concurrent.ScheduledExecutorService; import java.util.concurrent.TimeUnit; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; import java.util.function.Consumer; /** Tracks property usage data and flushes the data periodically to a sink. */ @@ -23,7 +24,7 @@ public class AccessMonitorUtil implements AutoCloseable { private static final Logger LOG = LoggerFactory.getLogger(AccessMonitorUtil.class); // Map from property id to property usage data - private final ConcurrentHashMap propertyUsageMap; + private final AtomicReference> propertyUsageMapRef; // Map from stack trace to how many times that stack trace appeared private final ConcurrentHashMap stackTrace; @@ -75,7 +76,7 @@ public static Builder builder() { private AccessMonitorUtil( Consumer dataFlushConsumer, boolean recordStackTrace) { - this.propertyUsageMap = new ConcurrentHashMap<>(); + this.propertyUsageMapRef = new AtomicReference(new ConcurrentHashMap<>()); this.stackTrace = new ConcurrentHashMap<>(); this.dataFlushConsumer = dataFlushConsumer; this.recordStackTrace = recordStackTrace; @@ -89,9 +90,9 @@ private AccessMonitorUtil( } private void startFlushing(int initialDelay, int period) { - if (!flushingEnabled()) { - LOG.info("Property usage data is being captured, but not flushed as there is no consumer specified."); - } else { + if (flushingEnabled()) { + LOG.info("Starting flushing property usage data in {} seconds and then every {} seconds after.", + initialDelay, period); executor.scheduleWithFixedDelay(this::flushUsageData, initialDelay, period, TimeUnit.SECONDS); } } @@ -108,8 +109,8 @@ private void flushUsageData() { /** Merge the results of given accessMonitorUtil into this one. */ public void merge(AccessMonitorUtil accessMonitorUtil) { - for (Map.Entry entry : accessMonitorUtil.propertyUsageMap.entrySet()) { - propertyUsageMap.putIfAbsent(entry.getKey(), entry.getValue()); + for (Map.Entry entry : accessMonitorUtil.propertyUsageMapRef.get().entrySet()) { + propertyUsageMapRef.get().putIfAbsent(entry.getKey(), entry.getValue()); } for (Map.Entry entry : accessMonitorUtil.stackTrace.entrySet()) { stackTrace.merge(entry.getKey(), entry.getValue(), Integer::sum); @@ -118,7 +119,7 @@ public void merge(AccessMonitorUtil accessMonitorUtil) { public void registerUsage(PropertyDetails propertyDetails) { // Initially, we limit the number of events we keep to one event per property id per flush. - propertyUsageMap.putIfAbsent( + propertyUsageMapRef.get().putIfAbsent( propertyDetails.getId(), new PropertyUsageData(createEventList(new PropertyUsageEvent(System.currentTimeMillis())))); @@ -138,15 +139,13 @@ private List createEventList(PropertyUsageEvent event) { } private Map getAndClearUsageMap() { - synchronized (propertyUsageMap) { - Map ret = getUsageMapImmutable(); - propertyUsageMap.clear(); - return ret; - } + Map map = + propertyUsageMapRef.getAndUpdate(unused -> new ConcurrentHashMap<>()); + return Collections.unmodifiableMap(new HashMap<>(map)); } public Map getUsageMapImmutable() { - return Collections.unmodifiableMap(new HashMap<>(propertyUsageMap)); + return Collections.unmodifiableMap(new HashMap<>(propertyUsageMapRef.get())); } public Map getStackTracesImmutable() { From 8f8bad69771ed6b5d0d51502fa6c750b8eef0b92 Mon Sep 17 00:00:00 2001 From: Yongkoo Kang Date: Thu, 18 Jan 2024 16:19:57 -0800 Subject: [PATCH 2/3] Change to getAndSet --- .../netflix/archaius/instrumentation/AccessMonitorUtil.java | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java b/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java index 3b191d67..9b1eaa82 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java @@ -139,8 +139,7 @@ private List createEventList(PropertyUsageEvent event) { } private Map getAndClearUsageMap() { - Map map = - propertyUsageMapRef.getAndUpdate(unused -> new ConcurrentHashMap<>()); + Map map = propertyUsageMapRef.getAndSet(new ConcurrentHashMap<>()); return Collections.unmodifiableMap(new HashMap<>(map)); } From 762634b97986bc34df6a84e4dff3e6b09b517fcb Mon Sep 17 00:00:00 2001 From: Yongkoo Kang Date: Fri, 19 Jan 2024 12:21:51 -0800 Subject: [PATCH 3/3] Some fixes to the usage of the usageMapRef --- .../netflix/archaius/instrumentation/AccessMonitorUtil.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java b/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java index 9b1eaa82..b799a3bc 100644 --- a/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java +++ b/archaius2-core/src/main/java/com/netflix/archaius/instrumentation/AccessMonitorUtil.java @@ -109,8 +109,9 @@ private void flushUsageData() { /** Merge the results of given accessMonitorUtil into this one. */ public void merge(AccessMonitorUtil accessMonitorUtil) { + Map myMap = propertyUsageMapRef.get(); for (Map.Entry entry : accessMonitorUtil.propertyUsageMapRef.get().entrySet()) { - propertyUsageMapRef.get().putIfAbsent(entry.getKey(), entry.getValue()); + myMap.putIfAbsent(entry.getKey(), entry.getValue()); } for (Map.Entry entry : accessMonitorUtil.stackTrace.entrySet()) { stackTrace.merge(entry.getKey(), entry.getValue(), Integer::sum); @@ -140,7 +141,7 @@ private List createEventList(PropertyUsageEvent event) { private Map getAndClearUsageMap() { Map map = propertyUsageMapRef.getAndSet(new ConcurrentHashMap<>()); - return Collections.unmodifiableMap(new HashMap<>(map)); + return Collections.unmodifiableMap(map); } public Map getUsageMapImmutable() {