From 8d442b1fbc8de809d6ba289c8b8ffd362e7a81d4 Mon Sep 17 00:00:00 2001
From: Nicholas Hagen <nicholas.hagen@znetdevelopment.com>
Date: Fri, 21 Mar 2014 17:00:00 -0400
Subject: [PATCH] Fix issue with paralellism and CodaHale metrics

Since there is no atomic remove/register operation for
Gauges, we need to synchronize.
---
 .../metrics/writer/CodahaleMetricWriter.java  | 12 +++-
 .../writer/CodahaleMetricWriterTests.java     | 70 +++++++++++++++++++
 2 files changed, 80 insertions(+), 2 deletions(-)

diff --git a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/writer/CodahaleMetricWriter.java b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/writer/CodahaleMetricWriter.java
index 20cc887ce0f..9502f6b2823 100644
--- a/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/writer/CodahaleMetricWriter.java
+++ b/spring-boot-actuator/src/main/java/org/springframework/boot/actuate/metrics/writer/CodahaleMetricWriter.java
@@ -86,8 +86,16 @@ public class CodahaleMetricWriter implements MetricWriter {
 		}
 		else {
 			final double gauge = value.getValue().doubleValue();
-			this.registry.remove(name);
-			this.registry.register(name, new SimpleGauge(gauge));
+
+			// ensure we synchronize on the registry to avoid another thread
+			// pre-empting this thread after remove causing register to be
+			// called twice causing an error in CodaHale metrics
+			// NOTE: this probably should not be synchronized, but CodaHale
+			// provides no other methods to get or add a particular gauge
+			synchronized (this.registry) {
+				this.registry.remove(name);
+				this.registry.register(name, new SimpleGauge(gauge));
+			}
 		}
 	}
 
diff --git a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/writer/CodahaleMetricWriterTests.java b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/writer/CodahaleMetricWriterTests.java
index 226f0fa676b..7772580a767 100644
--- a/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/writer/CodahaleMetricWriterTests.java
+++ b/spring-boot-actuator/src/test/java/org/springframework/boot/actuate/metrics/writer/CodahaleMetricWriterTests.java
@@ -16,6 +16,9 @@
 
 package org.springframework.boot.actuate.metrics.writer;
 
+import java.util.ArrayList;
+import java.util.List;
+
 import org.junit.Test;
 import org.springframework.boot.actuate.metrics.Metric;
 
@@ -23,6 +26,7 @@ import com.codahale.metrics.Gauge;
 import com.codahale.metrics.MetricRegistry;
 
 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 
 /**
  * @author Dave Syer
@@ -76,4 +80,70 @@ public class CodahaleMetricWriterTests {
 		assertEquals(2, this.registry.histogram("histogram.foo").getCount());
 	}
 
+	/**
+	 * Test the case where a given writer is used amongst several threads where each
+	 * thread is updating the same set of metrics. This would be an example case of the
+	 * writer being used with the MetricsFilter handling several requests/sec to the same
+	 * URL.
+	 * 
+	 * @throws Exception if an error occurs
+	 */
+	@Test
+	public void testParallism() throws Exception {
+		List<WriterThread> threads = new ArrayList<WriterThread>();
+		ThreadGroup group = new ThreadGroup("threads");
+		for (int i = 0; i < 10; i++) {
+			WriterThread thread = new WriterThread(group, i, this.writer);
+			threads.add(thread);
+			thread.start();
+		}
+
+		while (group.activeCount() > 0) {
+			Thread.sleep(1000);
+		}
+
+		for (WriterThread thread : threads) {
+			assertFalse("expected thread caused unexpected exception", thread.isFailed());
+		}
+	}
+
+	public static class WriterThread extends Thread {
+		private int index;
+		private boolean failed;
+		private CodahaleMetricWriter writer;
+
+		public WriterThread(ThreadGroup group, int index, CodahaleMetricWriter writer) {
+			super(group, "Writer-" + index);
+
+			this.index = index;
+			this.writer = writer;
+		}
+
+		public boolean isFailed() {
+			return this.failed;
+		}
+
+		@Override
+		public void run() {
+			for (int i = 0; i < 10000; i++) {
+				try {
+					Metric<Integer> metric1 = new Metric<Integer>(
+							"timer.test.service", this.index);
+					this.writer.set(metric1);
+
+					Metric<Integer> metric2 = new Metric<Integer>(
+							"histogram.test.service", this.index);
+					this.writer.set(metric2);
+
+					Metric<Integer> metric3 = new Metric<Integer>(
+							"gauge.test.service", this.index);
+					this.writer.set(metric3);
+				}
+				catch (IllegalArgumentException iae) {
+					this.failed = true;
+					throw iae;
+				}
+			}
+		}
+	}
 }