From ab634f398de1e1411e48e103f3e92b35322e03dd Sat, 12 Mar 2011 16:44:54 +0100
From: Cesar <cesar.alvarez.nunez@gmail.com>
Date: Sat, 12 Mar 2011 16:41:16 +0100
Subject: [PATCH] Added synchronization to LRUMessageCache. Now it is multithread-safe.



diff --git a/logback-classic/src/main/java/ch/qos/logback/classic/turbo/LRUMessageCache.java b/logback-classic/src/main/java/ch/qos/logback/classic/turbo/LRUMessageCache.java
index 90cd274..fcadfb8 100644
--- a/logback-classic/src/main/java/ch/qos/logback/classic/turbo/LRUMessageCache.java
+++ b/logback-classic/src/main/java/ch/qos/logback/classic/turbo/LRUMessageCache.java
@@ -15,8 +15,9 @@
 
 import java.util.LinkedHashMap;
 import java.util.Map;
+import java.util.concurrent.atomic.AtomicInteger;
 
-class LRUMessageCache extends LinkedHashMap<String, Integer> {
+class LRUMessageCache extends LinkedHashMap<String, AtomicInteger> {
 
   // LinkedHashMap permits null elements to be inserted
   
@@ -25,6 +26,7 @@
   final int cacheSize;
   
   LRUMessageCache(int cacheSize) {
+	// Access-ordered enabled.
     super((int) (cacheSize * (4.0f / 3)), 0.75f, true);
     if (cacheSize < 1) {
       throw new IllegalArgumentException("Cache size cannnot be smaller than 1");
@@ -38,16 +40,47 @@
       return 0;
     }
     
-    Integer i = super.get(msg);
-    if(i == null) {
-      i = 0;
-    } else {
-      i = new Integer(i.intValue()+1);
-    }
-    super.put(msg, i);
-    return i;
+	/*
+	 * From LinkedHashMap JavaDoc:
+	 * 
+	 * Note that this implementation is not synchronized. If multiple
+	 * threads access a linked hash map concurrently, and at least one of
+	 * the threads modifies the map structurally, it must be synchronized
+	 * externally. This is typically accomplished by synchronizing on some
+	 * object that naturally encapsulates the map. If no such object exists,
+	 * the map should be "wrapped" using the Collections.synchronizedMap
+	 * method. This is best done at creation time, to prevent accidental
+	 * unsynchronized access to the map:
+	 * 
+	 * Map m = Collections.synchronizedMap(new LinkedHashMap(...));
+	 * 
+	 * A structural modification is any operation that adds or deletes one
+	 * or more mappings or, in the case of access-ordered linked hash maps,
+	 * affects iteration order. In insertion-ordered linked hash maps,
+	 * merely changing the value associated with a key that is already
+	 * contained in the map is not a structural modification. In
+	 * access-ordered linked hash maps, merely querying the map with get is
+	 * a structural modification.)
+	 */
+    AtomicInteger i;
+
+    synchronized (this) {
+        i = super.get(msg);
+        if(i == null) {
+          i = new AtomicInteger(-1);
+          super.put(msg, i);
+        }
+	}
+    
+    return i.incrementAndGet();
   }
   
+  /**
+   * There is no need to synchronize this method because it is called as result
+   * of invoking the put or putAll methods.
+   * 
+   * @see java.util.LinkedHashMap#removeEldestEntry(java.util.Map.Entry)
+   */
   protected boolean removeEldestEntry(Map.Entry eldest) {
     return (size() > cacheSize);
   }
diff --git a/logback-classic/src/test/java/ch/qos/logback/classic/turbo/LRUMessageCacheTest.java b/logback-classic/src/test/java/ch/qos/logback/classic/turbo/LRUMessageCacheTest.java
new file mode 100644
index 0000000..6610881
--- /dev/null
+++ b/logback-classic/src/test/java/ch/qos/logback/classic/turbo/LRUMessageCacheTest.java
@@ -0,0 +1,80 @@
+package ch.qos.logback.classic.turbo;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.Callable;
+import java.util.concurrent.ExecutionException;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+import java.util.concurrent.Future;
+
+import junit.framework.Assert;
+
+import org.junit.Test;
+
+public class LRUMessageCacheTest {
+	private static final int INVOCATIONS_PER_TASK = 5*1024;
+	private static final int THREADS_NUMBER = 128;
+	
+	@Test
+	public void testEldestEntriesRemoval() {
+		final LRUMessageCache cache = new LRUMessageCache(2);
+		Assert.assertEquals(0, cache.getMessageCountAndThenIncrement("0"));
+		Assert.assertEquals(1, cache.getMessageCountAndThenIncrement("0"));
+		Assert.assertEquals(0, cache.getMessageCountAndThenIncrement("1"));
+		Assert.assertEquals(1, cache.getMessageCountAndThenIncrement("1"));
+		// 0 entry should have been removed.
+		Assert.assertEquals(0, cache.getMessageCountAndThenIncrement("2"));
+		// So it is expected a returned value of 0 instead of 2.
+		// 1 entry should have been removed.
+		Assert.assertEquals(0, cache.getMessageCountAndThenIncrement("0"));
+		// So it is expected a returned value of 0 instead of 2.
+		// 2 entry should have been removed.
+		Assert.assertEquals(0, cache.getMessageCountAndThenIncrement("1"));
+		// So it is expected a returned value of 0 instead of 2.
+		Assert.assertEquals(0, cache.getMessageCountAndThenIncrement("2"));
+	}
+
+	@Test
+	public void multiThreadsTest() throws InterruptedException, ExecutionException {
+		final LRUMessageCache cache = new LRUMessageCache(THREADS_NUMBER);
+		
+		ArrayList<TestTask> tasks = new ArrayList<TestTask>(THREADS_NUMBER);
+		for (int i=0; i<THREADS_NUMBER;i++) {
+			tasks.add(new TestTask(cache));
+		}
+		
+		ExecutorService execSrv = Executors.newFixedThreadPool(THREADS_NUMBER);
+		
+		List<Future<Boolean>> futures = execSrv.invokeAll(tasks);
+		for (Future<Boolean> future : futures) {
+			// Validate that task has successfully finished.
+			future.get();
+		}
+	}
+
+	/**
+	 * Each thread is using always the same "Message" key.
+	 */
+	private class TestTask implements Callable<Boolean> {
+		private int prevValue = -1;
+		private final LRUMessageCache cache;
+
+		public TestTask(LRUMessageCache cache) {
+			this.cache = cache;
+		}
+
+		public Boolean call() throws Exception {
+			String msg = Thread.currentThread().getName();
+			
+			for (int i = 0; i < INVOCATIONS_PER_TASK; i++) {
+				int current = cache.getMessageCountAndThenIncrement(msg);
+				// Ensure that new count is greater than previous count.
+				Assert.assertEquals(prevValue+1, current);
+				prevValue = current;
+			}
+
+			return true;
+		}
+	}
+}