Author: ceki Date: Mon Oct 27 15:41:52 2008 New Revision: 1876 Added: logback/trunk/logback-classic/src/main/java/org/slf4j/impl/CopyOnInheritThreadLocal.java logback/trunk/logback-classic/src/test/java/org/slf4j/ logback/trunk/logback-classic/src/test/java/org/slf4j/impl/ logback/trunk/logback-classic/src/test/java/org/slf4j/impl/LogbackMDCAdapterTest.java logback/trunk/logback-classic/src/test/java/org/slf4j/impl/PackageTest.java Modified: logback/trunk/logback-classic/src/main/java/org/slf4j/impl/LogbackMDCAdapter.java logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/AllClassicTest.java Log: - making sure that LogbackMDCAdapter actually copies its hash map. Strictly speaking, there is no need for copy-on-spawn because every change in the MDC already results in a copy of the hash map. Nevertheless, this change makes it easier for readers of the code to follow it. An external observer should not be able to tell the difference between the old and the new code. Added: logback/trunk/logback-classic/src/main/java/org/slf4j/impl/CopyOnInheritThreadLocal.java ============================================================================== --- (empty file) +++ logback/trunk/logback-classic/src/main/java/org/slf4j/impl/CopyOnInheritThreadLocal.java Mon Oct 27 15:41:52 2008 @@ -0,0 +1,24 @@ +package org.slf4j.impl; + +import java.util.HashMap; + +/** + * This class extends InheritableThreadLocal so that children threads get a copy + * of the parent's hashmap. + * + * @author Ceki Gülcü + */ +public class CopyOnInheritThreadLocal extends + InheritableThreadLocal<HashMap<String, String>> { + + /** + * Child threads should get a copy of the parent's hashmap. + */ + @Override + protected HashMap<String, String> childValue( + HashMap<String, String> parentValue) { + HashMap<String, String> hm = new HashMap<String, String>(parentValue); + return hm; + } + +} Modified: logback/trunk/logback-classic/src/main/java/org/slf4j/impl/LogbackMDCAdapter.java ============================================================================== --- logback/trunk/logback-classic/src/main/java/org/slf4j/impl/LogbackMDCAdapter.java (original) +++ logback/trunk/logback-classic/src/main/java/org/slf4j/impl/LogbackMDCAdapter.java Mon Oct 27 15:41:52 2008 @@ -24,7 +24,9 @@ */ public class LogbackMDCAdapter implements MDCAdapter { - private final InheritableThreadLocal<HashMap<String, String>> inheritableThreadLocal = new InheritableThreadLocal<HashMap<String, String>>(); + //final CopyOnInheritThreadLocal copyOnInheritThreadLocal = new CopyOnInheritThreadLocal(); + + final CopyOnInheritThreadLocal copyOnInheritThreadLocal = new CopyOnInheritThreadLocal(); LogbackMDCAdapter() { } @@ -45,21 +47,21 @@ * logback component to see the latest changes. * * @throws IllegalArgumentException - * in case the "key" parameter is null + * in case the "key" parameter is null */ public void put(String key, String val) throws IllegalArgumentException { if (key == null) { throw new IllegalArgumentException("key cannot be null"); } - HashMap<String, String> oldMap = inheritableThreadLocal.get(); + HashMap<String, String> oldMap = copyOnInheritThreadLocal.get(); HashMap<String, String> newMap = new HashMap<String, String>(); if (oldMap != null) { newMap.putAll(oldMap); } // the newMap replaces the old one for serialisation's sake - inheritableThreadLocal.set(newMap); + copyOnInheritThreadLocal.set(newMap); newMap.put(key, val); } @@ -70,7 +72,7 @@ * This method has no side effects. */ public String get(String key) { - HashMap<String, String> hashMap = inheritableThreadLocal.get(); + HashMap<String, String> hashMap = copyOnInheritThreadLocal.get(); if ((hashMap != null) && (key != null)) { return hashMap.get(key); @@ -89,14 +91,14 @@ * logback component to see the latest changes. */ public void remove(String key) { - HashMap<String, String> oldMap = inheritableThreadLocal.get(); + HashMap<String, String> oldMap = copyOnInheritThreadLocal.get(); HashMap<String, String> newMap = new HashMap<String, String>(); if (oldMap != null) { newMap.putAll(oldMap); } // the newMap replaces the old one for serialisation's sake - inheritableThreadLocal.set(newMap); + copyOnInheritThreadLocal.set(newMap); newMap.remove(key); } @@ -104,11 +106,11 @@ * Clear all entries in the MDC. */ public void clear() { - HashMap<String, String> hashMap = inheritableThreadLocal.get(); + HashMap<String, String> hashMap = copyOnInheritThreadLocal.get(); if (hashMap != null) { hashMap.clear(); - inheritableThreadLocal.remove(); + copyOnInheritThreadLocal.remove(); } } @@ -117,15 +119,15 @@ * internally. */ public Map<String, String> getPropertyMap() { - return inheritableThreadLocal.get(); + return copyOnInheritThreadLocal.get(); } /** - * Return a copy of the current thread's context map. - * Returned value may be null. + * Return a copy of the current thread's context map. Returned value may be + * null. */ public Map getCopyOfContextMap() { - HashMap<String, String> hashMap = inheritableThreadLocal.get(); + HashMap<String, String> hashMap = copyOnInheritThreadLocal.get(); if (hashMap == null) { return null; } else { @@ -133,13 +135,12 @@ } } - /** * Returns the keys in the MDC as a {@link Set}. The returned value can be * null. */ public Set<String> getKeys() { - HashMap<String, String> hashMap = inheritableThreadLocal.get(); + HashMap<String, String> hashMap = copyOnInheritThreadLocal.get(); if (hashMap != null) { return hashMap.keySet(); @@ -148,17 +149,16 @@ } } - - @SuppressWarnings("unchecked") + @SuppressWarnings("unchecked") public void setContextMap(Map contextMap) { - HashMap<String, String> oldMap = inheritableThreadLocal.get(); + HashMap<String, String> oldMap = copyOnInheritThreadLocal.get(); HashMap<String, String> newMap = new HashMap<String, String>(); newMap.putAll(contextMap); // the newMap replaces the old one for serialisation's sake - inheritableThreadLocal.set(newMap); - + copyOnInheritThreadLocal.set(newMap); + // hints for the garbage collector oldMap.clear(); oldMap = null; Modified: logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/AllClassicTest.java ============================================================================== --- logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/AllClassicTest.java (original) +++ logback/trunk/logback-classic/src/test/java/ch/qos/logback/classic/AllClassicTest.java Mon Oct 27 15:41:52 2008 @@ -17,7 +17,8 @@ public static Test suite() { TestSuite suite = new TestSuite(); - + + suite.addTest(org.slf4j.impl.PackageTest.suite()); suite.addTest(ch.qos.logback.classic.PackageTest.suite()); suite.addTest(ch.qos.logback.classic.control.PackageTest.suite()); suite.addTest(ch.qos.logback.classic.joran.PackageTest.suite()); Added: logback/trunk/logback-classic/src/test/java/org/slf4j/impl/LogbackMDCAdapterTest.java ============================================================================== --- (empty file) +++ logback/trunk/logback-classic/src/test/java/org/slf4j/impl/LogbackMDCAdapterTest.java Mon Oct 27 15:41:52 2008 @@ -0,0 +1,82 @@ +/** + * Logback: the generic, reliable, fast and flexible logging framework. + * + * Copyright (C) 2000-2008, QOS.ch + * + * This library is free software, you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation. + */ +package org.slf4j.impl; + +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; +import static org.junit.Assert.assertTrue; + +import java.util.HashMap; +import java.util.Random; + +import org.junit.Test; +import org.slf4j.MDC; + +public class LogbackMDCAdapterTest { + + final static String A_SUFFIX = "A_SUFFIX"; + + int diff = new Random().nextInt(); + + /** + * Test that LogbackMDCAdapter copies its hashmap when a child + * thread inherits it. + * + * @throws InterruptedException + */ + @Test + public void copyOnInheritence() throws InterruptedException { + String mdcKey = "x" + diff; + String otherMDCKey = "o" + diff; + MDC.put(mdcKey, mdcKey + A_SUFFIX); + + HashMap<String, String> parentHM = getHM(); + + MyThread myThread = new MyThread(mdcKey, otherMDCKey); + myThread.start(); + myThread.join(); + + assertNull(MDC.get(otherMDCKey)); + assertTrue(myThread.successul); + assertTrue(parentHM != myThread.childHM); + } + + class MyThread extends Thread { + + String mdcKey; + String otherMDCKey; + boolean successul; + HashMap<String, String> childHM; + + MyThread(String mdcKey, String otherMDCKey) { + this.mdcKey = mdcKey; + this.otherMDCKey = otherMDCKey; + } + + @Override + public void run() { + childHM = getHM(); + + MDC.put(otherMDCKey, otherMDCKey + A_SUFFIX); + assertNotNull(MDC.get(mdcKey)); + assertEquals(mdcKey + A_SUFFIX, MDC.get(mdcKey)); + assertEquals(otherMDCKey + A_SUFFIX, MDC.get(otherMDCKey)); + successul = true; + } + } + + HashMap<String, String> getHM() { + LogbackMDCAdapter lma = (LogbackMDCAdapter) MDC.getMDCAdapter(); + CopyOnInheritThreadLocal copyOnInheritThreadLocal = lma.copyOnInheritThreadLocal; + return copyOnInheritThreadLocal.get(); + + } +} Added: logback/trunk/logback-classic/src/test/java/org/slf4j/impl/PackageTest.java ============================================================================== --- (empty file) +++ logback/trunk/logback-classic/src/test/java/org/slf4j/impl/PackageTest.java Mon Oct 27 15:41:52 2008 @@ -0,0 +1,21 @@ +/** + * Logback: the generic, reliable, fast and flexible logging framework. + * + * Copyright (C) 2000-2008, QOS.ch + * + * This library is free software, you can redistribute it and/or modify it under + * the terms of the GNU Lesser General Public License as published by the Free + * Software Foundation. + */ +package org.slf4j.impl; + +import junit.framework.*; + +public class PackageTest extends TestCase { + + public static Test suite() { + TestSuite suite = new TestSuite(); + suite.addTest(new JUnit4TestAdapter(LogbackMDCAdapterTest.class)); + return suite; + } +} \ No newline at end of file