Commit ee567fa8 authored by Andy Wilkinson's avatar Andy Wilkinson

Make MetricRegistryMetricReader thread-safe

MetricRegistryMetricReader’s fields where neither final, nor volatile
but could be accessed on multiple threads. This lead to visibility
problems where the value of a field would unexpectedly be null, causing
an NPE.

This commit updates all of the fields to declare them as final, thereby
ensuring that their values are guaranteed to be visible across different
threads.

Fixes gh-2590
parent 493d7a36
/* /*
* Copyright 2013-2104 the original author or authors. * Copyright 2013-2015 the original author or authors.
* *
* Licensed under the Apache License, Version 2.0 (the "License"); * Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License. * you may not use this file except in compliance with the License.
...@@ -48,17 +48,17 @@ import com.codahale.metrics.Timer; ...@@ -48,17 +48,17 @@ import com.codahale.metrics.Timer;
* of type Number. * of type Number.
* *
* @author Dave Syer * @author Dave Syer
* * @author Andy Wilkinson
*/ */
public class MetricRegistryMetricReader implements MetricReader, MetricRegistryListener { public class MetricRegistryMetricReader implements MetricReader, MetricRegistryListener {
private static Map<Class<?>, Set<String>> numberKeys = new ConcurrentHashMap<Class<?>, Set<String>>(); private static final Map<Class<?>, Set<String>> NUMBER_KEYS = new ConcurrentHashMap<Class<?>, Set<String>>();
private MetricRegistry registry; private final MetricRegistry registry;
private Map<String, String> names = new HashMap<String, String>(); private final Map<String, String> names = new HashMap<String, String>();
private MultiValueMap<String, String> reverse = new LinkedMultiValueMap<String, String>(); private final MultiValueMap<String, String> reverse = new LinkedMultiValueMap<String, String>();
public MetricRegistryMetricReader(MetricRegistry registry) { public MetricRegistryMetricReader(MetricRegistry registry) {
this.registry = registry; this.registry = registry;
...@@ -67,11 +67,11 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -67,11 +67,11 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
@Override @Override
public Metric<?> findOne(String metricName) { public Metric<?> findOne(String metricName) {
if (!names.containsKey(metricName)) { if (!this.names.containsKey(metricName)) {
return null; return null;
} }
com.codahale.metrics.Metric metric = registry.getMetrics().get( com.codahale.metrics.Metric metric = this.registry.getMetrics().get(
names.get(metricName)); this.names.get(metricName));
if (metric instanceof Counter) { if (metric instanceof Counter) {
Counter counter = (Counter) metric; Counter counter = (Counter) metric;
return new Metric<Number>(metricName, counter.getCount()); return new Metric<Number>(metricName, counter.getCount());
...@@ -107,13 +107,13 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -107,13 +107,13 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
@Override @Override
public long count() { public long count() {
return names.size(); return this.names.size();
} }
@Override @Override
public void onGaugeAdded(String name, Gauge<?> gauge) { public void onGaugeAdded(String name, Gauge<?> gauge) {
names.put(name, name); this.names.put(name, name);
reverse.add(name, name); this.reverse.add(name, name);
} }
@Override @Override
...@@ -123,8 +123,8 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -123,8 +123,8 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
@Override @Override
public void onCounterAdded(String name, Counter counter) { public void onCounterAdded(String name, Counter counter) {
names.put(name, name); this.names.put(name, name);
reverse.add(name, name); this.reverse.add(name, name);
} }
@Override @Override
...@@ -136,13 +136,13 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -136,13 +136,13 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
public void onHistogramAdded(String name, Histogram histogram) { public void onHistogramAdded(String name, Histogram histogram) {
for (String key : getNumberKeys(histogram)) { for (String key : getNumberKeys(histogram)) {
String metricName = name + "." + key; String metricName = name + "." + key;
names.put(metricName, name); this.names.put(metricName, name);
reverse.add(name, metricName); this.reverse.add(name, metricName);
} }
for (String key : getNumberKeys(histogram.getSnapshot())) { for (String key : getNumberKeys(histogram.getSnapshot())) {
String metricName = name + ".snapshot." + key; String metricName = name + ".snapshot." + key;
names.put(metricName, name); this.names.put(metricName, name);
reverse.add(name, metricName); this.reverse.add(name, metricName);
} }
} }
...@@ -155,8 +155,8 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -155,8 +155,8 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
public void onMeterAdded(String name, Meter meter) { public void onMeterAdded(String name, Meter meter) {
for (String key : getNumberKeys(meter)) { for (String key : getNumberKeys(meter)) {
String metricName = name + "." + key; String metricName = name + "." + key;
names.put(metricName, name); this.names.put(metricName, name);
reverse.add(name, metricName); this.reverse.add(name, metricName);
} }
} }
...@@ -169,13 +169,13 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -169,13 +169,13 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
public void onTimerAdded(String name, Timer timer) { public void onTimerAdded(String name, Timer timer) {
for (String key : getNumberKeys(timer)) { for (String key : getNumberKeys(timer)) {
String metricName = name + "." + key; String metricName = name + "." + key;
names.put(metricName, name); this.names.put(metricName, name);
reverse.add(name, metricName); this.reverse.add(name, metricName);
} }
for (String key : getNumberKeys(timer.getSnapshot())) { for (String key : getNumberKeys(timer.getSnapshot())) {
String metricName = name + ".snapshot." + key; String metricName = name + ".snapshot." + key;
names.put(metricName, name); this.names.put(metricName, name);
reverse.add(name, metricName); this.reverse.add(name, metricName);
} }
} }
...@@ -185,10 +185,10 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -185,10 +185,10 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
} }
private void remove(String name) { private void remove(String name) {
for (String key : reverse.get(name)) { for (String key : this.reverse.get(name)) {
names.remove(name + "." + key); this.names.remove(name + "." + key);
} }
reverse.remove(name); this.reverse.remove(name);
} }
private class MetricRegistryIterator implements Iterator<Metric<?>> { private class MetricRegistryIterator implements Iterator<Metric<?>> {
...@@ -202,12 +202,12 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -202,12 +202,12 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
@Override @Override
public boolean hasNext() { public boolean hasNext() {
return iterator.hasNext(); return this.iterator.hasNext();
} }
@Override @Override
public Metric<?> next() { public Metric<?> next() {
String name = iterator.next(); String name = this.iterator.next();
return MetricRegistryMetricReader.this.findOne(name); return MetricRegistryMetricReader.this.findOne(name);
} }
...@@ -220,7 +220,7 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -220,7 +220,7 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
} }
private static Set<String> getNumberKeys(Object metric) { private static Set<String> getNumberKeys(Object metric) {
Set<String> result = numberKeys.containsKey(metric.getClass()) ? numberKeys Set<String> result = NUMBER_KEYS.containsKey(metric.getClass()) ? NUMBER_KEYS
.get(metric.getClass()) : new HashSet<String>(); .get(metric.getClass()) : new HashSet<String>();
if (result.isEmpty()) { if (result.isEmpty()) {
for (PropertyDescriptor descriptor : BeanUtils.getPropertyDescriptors(metric for (PropertyDescriptor descriptor : BeanUtils.getPropertyDescriptors(metric
...@@ -229,7 +229,7 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL ...@@ -229,7 +229,7 @@ public class MetricRegistryMetricReader implements MetricReader, MetricRegistryL
result.add(descriptor.getName()); result.add(descriptor.getName());
} }
} }
numberKeys.put(metric.getClass(), result); NUMBER_KEYS.put(metric.getClass(), result);
} }
return result; return result;
} }
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment