From 4487823ff9928fc97641981d7687c89e16ca9d16 Mon Sep 17 00:00:00 2001 From: Andy Wilkinson Date: Wed, 4 Mar 2015 15:03:03 +0000 Subject: [PATCH] Improve thread-safety of MetricRegistryMetricReader ee567fa boldy claimed that it had made MetricRegistryMetricReader thread-safe. It had not. This commit should actually make it thread safe. I hope. One notable improvement is that MetricRegistryMetricReader.findAll() will no longer contain null values if a metric is removed on another thread during iteration. names is now a ConcurrentHashMap to allow it to be safely read and written without holding a lock. reverse is a LinkedMultiValueMap which is not thread-safe. This could lead to values being lost when concurrent add calls were made. Access to reverse is now protected by synchronizing on an internal monitor object. Calls to containsKey(key) followed by get(key) have been reworked to only call get(key), this avoids the possibility of the key being removed after the contains check but before the get. Closes gh-2590 --- .../reader/MetricRegistryMetricReader.java | 98 ++++++++++--------- 1 file changed, 52 insertions(+), 46 deletions(-) diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/reader/MetricRegistryMetricReader.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/reader/MetricRegistryMetricReader.java index bd6b79df51f..5e6a6b24cf8 100644 --- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/reader/MetricRegistryMetricReader.java +++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/reader/MetricRegistryMetricReader.java @@ -13,12 +13,13 @@ * See the License for the specific language governing permissions and * limitations under the License. */ + package org.springframework.boot.actuate.metrics.reader; import java.beans.PropertyDescriptor; -import java.util.HashMap; import java.util.HashSet; import java.util.Iterator; +import java.util.List; import java.util.Map; import java.util.Set; import java.util.concurrent.ConcurrentHashMap; @@ -54,12 +55,14 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL private static final Map, Set> NUMBER_KEYS = new ConcurrentHashMap, Set>(); - private final MetricRegistry registry; + private final Object monitor = new Object(); - private final Map names = new HashMap(); + private final Map names = new ConcurrentHashMap(); private final MultiValueMap reverse = new LinkedMultiValueMap(); + private final MetricRegistry registry; + public MetricRegistryMetricReader(MetricRegistry registry) { this.registry = registry; registry.addListener(this); @@ -67,11 +70,14 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL @Override public Metric findOne(String metricName) { - if (!this.names.containsKey(metricName)) { + String name = this.names.get(metricName); + if (name == null) { + return null; + } + com.codahale.metrics.Metric metric = this.registry.getMetrics().get(name); + if (metric == null) { return null; } - com.codahale.metrics.Metric metric = this.registry.getMetrics().get( - this.names.get(metricName)); if (metric instanceof Counter) { Counter counter = (Counter) metric; return new Metric(metricName, counter.getCount()); @@ -100,7 +106,14 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL return new Iterable>() { @Override public Iterator> iterator() { - return new MetricRegistryIterator(); + Set> metrics = new HashSet>(); + for (String name : MetricRegistryMetricReader.this.names.keySet()) { + Metric metric = findOne(name); + if (metric != null) { + metrics.add(metric); + } + } + return metrics.iterator(); } }; } @@ -113,7 +126,9 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL @Override public void onGaugeAdded(String name, Gauge gauge) { this.names.put(name, name); - this.reverse.add(name, name); + synchronized (this.monitor) { + this.reverse.add(name, name); + } } @Override @@ -124,7 +139,9 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL @Override public void onCounterAdded(String name, Counter counter) { this.names.put(name, name); - this.reverse.add(name, name); + synchronized (this.monitor) { + this.reverse.add(name, name); + } } @Override @@ -137,12 +154,16 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL for (String key : getNumberKeys(histogram)) { String metricName = name + "." + key; this.names.put(metricName, name); - this.reverse.add(name, metricName); + synchronized (this.monitor) { + this.reverse.add(name, metricName); + } } for (String key : getNumberKeys(histogram.getSnapshot())) { String metricName = name + ".snapshot." + key; this.names.put(metricName, name); - this.reverse.add(name, metricName); + synchronized (this.monitor) { + this.reverse.add(name, metricName); + } } } @@ -156,7 +177,9 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL for (String key : getNumberKeys(meter)) { String metricName = name + "." + key; this.names.put(metricName, name); - this.reverse.add(name, metricName); + synchronized (this.monitor) { + this.reverse.add(name, metricName); + } } } @@ -170,12 +193,16 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL for (String key : getNumberKeys(timer)) { String metricName = name + "." + key; this.names.put(metricName, name); - this.reverse.add(name, metricName); + synchronized (this.monitor) { + this.reverse.add(name, metricName); + } } for (String key : getNumberKeys(timer.getSnapshot())) { String metricName = name + ".snapshot." + key; this.names.put(metricName, name); - this.reverse.add(name, metricName); + synchronized (this.monitor) { + this.reverse.add(name, metricName); + } } } @@ -185,43 +212,22 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL } private void remove(String name) { - for (String key : this.reverse.get(name)) { + List keys; + + synchronized (this.monitor) { + keys = this.reverse.remove(name); + } + + for (String key : keys) { this.names.remove(name + "." + key); } - this.reverse.remove(name); - } - - private class MetricRegistryIterator implements Iterator> { - - private Iterator iterator; - - public MetricRegistryIterator() { - this.iterator = new HashSet( - MetricRegistryMetricReader.this.names.keySet()).iterator(); - } - - @Override - public boolean hasNext() { - return this.iterator.hasNext(); - } - - @Override - public Metric next() { - String name = this.iterator.next(); - return MetricRegistryMetricReader.this.findOne(name); - } - - @Override - public void remove() { - throw new UnsupportedOperationException( - "You cannot remove from this iterator."); - } - } private static Set getNumberKeys(Object metric) { - Set result = NUMBER_KEYS.containsKey(metric.getClass()) ? NUMBER_KEYS - .get(metric.getClass()) : new HashSet(); + Set result = NUMBER_KEYS.get(metric.getClass()); + if (result == null) { + result = new HashSet(); + } if (result.isEmpty()) { for (PropertyDescriptor descriptor : BeanUtils.getPropertyDescriptors(metric .getClass())) {