JavaLogFactory Thread Safety #286 (#470)

* JavaLogFactory Concurrency Test

Test case showing that in multi-threaded requests for a single reference,
multiple Logger references may be created.

* Logic & Test Updates JavaLogFactory

Extending test to also create Logs by module name.

Updating logic in class to have class delegate to the module name with the
class name reference and synchronizing on the map while the reference is
being retrieved and/or created.

Author: jeremiahjstacey

src/main/java/org/owasp/esapi/reference/JavaLogFactory.java

@@ -49,30 +49,24 @@ public JavaLogFactory() {}
 	* {@inheritDoc}
 	*/
 	public Logger getLogger(Class clazz) {
-    	
-    	// If a logger for this class already exists, we return the same one, otherwise we create a new one.
-    	Logger classLogger = (Logger) loggersMap.get(clazz);
-    	
-    	if (classLogger == null) {
-    		classLogger = new JavaLogger(clazz.getName());
-    		loggersMap.put(clazz, classLogger);
-    	}
-		return classLogger;
+	    return getLogger(clazz.getName());
     }
 
     /**
 	* {@inheritDoc}
 	*/
     public Logger getLogger(String moduleName) {
 
-    	// If a logger for this module already exists, we return the same one, otherwise we create a new one.
-    	Logger moduleLogger = (Logger) loggersMap.get(moduleName);
-    	
-    	if (moduleLogger == null) {
-    		moduleLogger = new JavaLogger(moduleName);
-    		loggersMap.put(moduleName, moduleLogger);    		
-    	}
-		return moduleLogger;
+        synchronized (loggersMap) {
+            // If a logger for this module already exists, we return the same one, otherwise we create a new one.
+            Logger moduleLogger = (Logger) loggersMap.get(moduleName);
+
+            if (moduleLogger == null) {
+                moduleLogger = new JavaLogger(moduleName);
+                loggersMap.put(moduleName, moduleLogger);    		
+            }
+            return moduleLogger;
+        }
     }
 
 

src/test/java/org/owasp/esapi/reference/JavaLogFactoryTest.java

@@ -0,0 +1,73 @@
+package org.owasp.esapi.reference;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.CountDownLatch;
+
+import org.junit.Assert;
+import org.junit.Test;
+import org.owasp.esapi.Logger;
+
+
+public class JavaLogFactoryTest {
+
+    @Test
+    public void testConcurrentLogRequest() throws InterruptedException {
+        final ConcurrentHashMap<Integer, Logger> logCapture = new ConcurrentHashMap<>();
+        List<Thread> threads = new ArrayList<>();
+        final CountDownLatch forceConcurrency = new CountDownLatch(1);
+        for (int x = 0 ; x < 10; x ++) {
+            final int requestIndex = x;
+            Runnable requestLogByClass = new Runnable() {
+                @Override
+                public void run() {
+                   try {
+                    forceConcurrency.await();
+                } catch (InterruptedException e) {
+                    // TODO Auto-generated catch block
+                    e.printStackTrace();
+                }
+                   logCapture.put(requestIndex, JavaLogFactory.getInstance().getLogger(JavaLogFactoryTest.class));
+                }
+            };
+            
+            Runnable requestLogByModule = new Runnable() {
+                @Override
+                public void run() {
+                   try {
+                    forceConcurrency.await();
+                } catch (InterruptedException e) {
+                    // TODO Auto-generated catch block
+                    e.printStackTrace();
+                }
+                   logCapture.put(requestIndex, JavaLogFactory.getInstance().getLogger(JavaLogFactoryTest.class.getName()));
+                }
+            };
+            
+            threads.add(new Thread(requestLogByClass, "Request Log By Class " + x));
+            
+            threads.add(new Thread(requestLogByModule, "Request Log By Name " + x));
+            
+            
+        }
+        
+        for (Thread thread : threads) {
+            thread.start();
+        }
+        
+        forceConcurrency.countDown();
+        
+        for (Thread thread: threads) {
+            thread.join();
+        }
+        
+        
+        Set<Logger> uniqueLoggers = new HashSet<>(logCapture.values());
+        
+        Assert.assertEquals(1, uniqueLoggers.size());
+        
+    }
+}