Merge pull request #99 from lukecwik/master

Fixed BasicMDCAdapter leaks MDC information to non-child threads

slf4j-api/src/main/java/org/slf4j/helpers/BasicMDCAdapter.java

@@ -32,28 +32,37 @@
 /**
  * Basic MDC implementation, which can be used with logging systems that lack
  * out-of-the-box MDC support.
- * 
+ *
  * This code was initially inspired by  logback's LogbackMDCAdapter. However,
  * LogbackMDCAdapter has evolved and is now considerably more sophisticated.
  *
  * @author Ceki Gulcu
  * @author Maarten Bosteels
- * 
+ *
  * @since 1.5.0
  */
 public class BasicMDCAdapter implements MDCAdapter {
 
-    private InheritableThreadLocal<Map<String, String>> inheritableThreadLocal = new InheritableThreadLocal<Map<String, String>>();
+    private InheritableThreadLocal<Map<String, String>> inheritableThreadLocal =
+            new InheritableThreadLocal<Map<String, String>>() {
+        @Override
+        protected Map<String,String> childValue(Map<String,String> parentValue) {
+            if (parentValue == null) {
+                return null;
+            }
+            return new HashMap<String, String>(parentValue);
+        }
+    };
 
     /**
      * Put a context value (the <code>val</code> parameter) as identified with
      * the <code>key</code> parameter into the current thread's context map.
      * Note that contrary to log4j, the <code>val</code> parameter can be null.
-     * 
+     *
      * <p>
      * If the current thread does not have a context map it is created as a side
      * effect of this call.
-     * 
+     *
      * @throws IllegalArgumentException
      *                 in case the "key" parameter is null
      */
@@ -63,7 +72,7 @@ public void put(String key, String val) {
         }
         Map<String, String> map = inheritableThreadLocal.get();
         if (map == null) {
-            map = Collections.synchronizedMap(new HashMap<String, String>());
+            map = new HashMap<String, String>();
             inheritableThreadLocal.set(map);
         }
         map.put(key, val);
@@ -73,9 +82,9 @@ public void put(String key, String val) {
      * Get the context identified by the <code>key</code> parameter.
      */
     public String get(String key) {
-        Map<String, String> Map = inheritableThreadLocal.get();
-        if ((Map != null) && (key != null)) {
-            return Map.get(key);
+        Map<String, String> map = inheritableThreadLocal.get();
+        if ((map != null) && (key != null)) {
+            return map.get(key);
         } else {
             return null;
         }
@@ -105,7 +114,7 @@ public void clear() {
     /**
      * Returns the keys in the MDC as a {@link Set} of {@link String}s The
      * returned value can be null.
-     * 
+     *
      * @return the keys in the MDC
      */
     public Set<String> getKeys() {
@@ -118,26 +127,20 @@ public Set<String> getKeys() {
     }
 
     /**
-     * Return a copy of the current thread's context map. 
+     * Return a copy of the current thread's context map.
      * Returned value may be null.
-     * 
+     *
      */
     public Map<String, String> getCopyOfContextMap() {
         Map<String, String> oldMap = inheritableThreadLocal.get();
         if (oldMap != null) {
-            Map<String, String> newMap = Collections.synchronizedMap(new HashMap<String, String>());
-            synchronized (oldMap) {
-                newMap.putAll(oldMap);
-            }
-            return newMap;
+            return new HashMap<String, String>(oldMap);
         } else {
             return null;
         }
     }
 
     public void setContextMap(Map<String, String> contextMap) {
-        Map<String, String> map = Collections.synchronizedMap(new HashMap<String, String>(contextMap));
-        inheritableThreadLocal.set(map);
+        inheritableThreadLocal.set(new HashMap<String, String>(contextMap));
     }
-
 }

slf4j-api/src/test/java/org/slf4j/helpers/BasicMDCAdapterTest.java

@@ -0,0 +1,139 @@
+/**
+ * Copyright (c) 2004-2013 QOS.ch, Copyright (C) 2015 Google Inc.
+ * All rights reserved.
+ *
+ * Permission is hereby granted, free  of charge, to any person obtaining
+ * a  copy  of this  software  and  associated  documentation files  (the
+ * "Software"), to  deal in  the Software without  restriction, including
+ * without limitation  the rights to  use, copy, modify,  merge, publish,
+ * distribute,  sublicense, and/or sell  copies of  the Software,  and to
+ * permit persons to whom the Software  is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The  above  copyright  notice  and  this permission  notice  shall  be
+ * included in all copies or substantial portions of the Software.
+ *
+ * THE  SOFTWARE IS  PROVIDED  "AS  IS", WITHOUT  WARRANTY  OF ANY  KIND,
+ * EXPRESS OR  IMPLIED, INCLUDING  BUT NOT LIMITED  TO THE  WARRANTIES OF
+ * MERCHANTABILITY,    FITNESS    FOR    A   PARTICULAR    PURPOSE    AND
+ * NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE
+ * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION
+ * OF CONTRACT, TORT OR OTHERWISE,  ARISING FROM, OUT OF OR IN CONNECTION
+ * WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+package org.slf4j.helpers;
+
+import junit.framework.TestCase;
+
+import org.slf4j.spi.MDCAdapter;
+
+import java.lang.Thread.UncaughtExceptionHandler;
+import java.util.Map;
+
+/**
+ * Tests for {@link BasicMDCAdapter}
+ * 
+ * @author Lukasz Cwik
+ */
+public class BasicMDCAdapterTest extends TestCase {
+  MDCAdapter mdc = new BasicMDCAdapter();
+
+  @Override
+  protected void tearDown() throws Exception {
+    mdc.clear();
+  }
+
+  public void testSettingAndGettingWithMDC() {
+    assertNull(mdc.get("testKey"));
+    mdc.put("testKey", "testValue");
+    assertEquals(mdc.get("testKey"), "testValue");
+  }
+
+  public void testOverwritingAKeyInMDC() {
+    assertNull(mdc.get("testKey"));
+    mdc.put("testKey", "testValue");
+    mdc.put("testKey", "differentTestValue");
+    assertEquals(mdc.get("testKey"), "differentTestValue");
+  }
+
+  public void testClearingMDC() {
+    mdc.put("testKey", "testValue");
+    assertFalse(mdc.getCopyOfContextMap().isEmpty());
+    mdc.clear();
+    assertNull(mdc.getCopyOfContextMap());
+  }
+
+  public void testGetCopyOfContextMapFromMDC() {
+    mdc.put("testKey", "testValue");
+    Map<String, String> copy = mdc.getCopyOfContextMap();
+    mdc.put("anotherTestKey", "anotherTestValue");
+    assertFalse(copy.size() == mdc.getCopyOfContextMap().size());
+  }
+
+  public void testMDCInheritsValuesFromParentThread() throws Exception {
+    mdc.put("parentKey", "parentValue");
+    runAndWait(new Runnable() {
+      @Override
+      public void run() {
+        mdc.put("childKey", "childValue");
+        assertEquals("parentValue", mdc.get("parentKey"));
+      }
+    });
+  }
+
+  public void testMDCDoesntGetValuesFromChildThread() throws Exception {
+    mdc.put("parentKey", "parentValue");
+    runAndWait(new Runnable() {
+      @Override
+      public void run() {
+        mdc.put("childKey", "childValue");
+      }
+    });
+    assertEquals("parentValue", mdc.get("parentKey"));
+    assertNull(mdc.get("childKey"));
+  }
+
+  public void testMDCChildThreadCanOverwriteParentThread() throws Exception {
+    mdc.put("sharedKey", "parentValue");
+    runAndWait(new Runnable() {
+      @Override
+      public void run() {
+        assertEquals("parentValue", mdc.get("sharedKey"));
+        mdc.put("sharedKey", "childValue");
+        assertEquals("childValue", mdc.get("sharedKey"));
+      }
+    });
+    assertEquals("parentValue", mdc.get("sharedKey"));
+  }
+
+  private void runAndWait(Runnable runnable) throws Exception {
+    RecordingExceptionHandler handler = new RecordingExceptionHandler();
+    Thread thread = new Thread(runnable);
+    thread.setUncaughtExceptionHandler(handler);
+    thread.start();
+    try {
+      thread.join();
+    } catch(Throwable t) {
+      fail("Unexpected failure in child thread:" + t.getMessage());
+    }
+    assertFalse(handler.getMessage(), handler.hadException());
+  }
+
+  /** A {@link UncaughtExceptionHandler} that records whether the thread threw an exception. */
+  private static class RecordingExceptionHandler implements UncaughtExceptionHandler {
+    private Throwable exception;
+    @Override
+    public void uncaughtException(Thread t, Throwable e) {
+      exception = e;
+    }
+
+    boolean hadException() {
+      return exception != null;
+    }
+
+    String getMessage() {
+      return exception != null ? exception.getMessage() : "";
+    }
+  }
+}