natas_hw
/
slf4j
mirror of https://github.com/qos-ch/slf4j
0
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix bug: http://bugzilla.slf4j.org/show_bug.cgi?id=325 

BasicMDCAdapter leaks MDC information to non-child threads
BasicMDCAdapter uses a ConcurrentHashMap which is passed by reference from the parent to the child. All child threads then share the same map reference and any modifications by one child is visible to the parent thread and also all other child threads.

Swapped to creating a copy of the parents map instead of storing reference. Also, since this is expected to be only accessed from within the same thread, swapped to use a hash map instead of a concurrent hash map.

Signed-off-by: Luke Cwik <lcwik@google.com>
This commit is contained in:
Luke Cwik 2015-01-27 15:41:43 -08:00
parent d18e5e9252
commit 22ef2008ec
2 changed files with 166 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

44
slf4j-api/src/main/java/org/slf4j/helpers/BasicMDCAdapter.java
View File

@ -1,5 +1,5 @@
/**
* Copyright (c) 2004-2011 QOS.ch
* Copyright (c) 2004-2011 QOS.ch, Copyright (C) 2015 Google Inc.
* All rights reserved.
*
* Permission is hereby granted, free of charge, to any person obtaining
@ -44,7 +44,15 @@ import java.util.Map;
public class BasicMDCAdapter implements MDCAdapter {
private InheritableThreadLocal<Map<String, String>> inheritableThreadLocal
= new InheritableThreadLocal<Map<String, String>>();
= 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);
}
};
static boolean isJDK14() {
try {
@ -71,13 +79,14 @@ public class BasicMDCAdapter implements MDCAdapter {
* @throws IllegalArgumentException
* in case the "key" parameter is null
*/
@Override
public void put(String key, String val) {
if (key == null) {
throw new IllegalArgumentException("key cannot be null");
}
Map<String, String> map = (Map<String, String>) inheritableThreadLocal.get();
Map<String, String> map = inheritableThreadLocal.get();
if (map == null) {
map = Collections.<String, String>synchronizedMap(new HashMap<String, String>());
map = new HashMap<String, String>();
inheritableThreadLocal.set(map);
}
map.put(key, val);
@ -86,10 +95,11 @@ public class BasicMDCAdapter implements MDCAdapter {
/**
* Get the context identified by the <code>key</code> parameter.
*/
@Override
public String get(String key) {
Map<String, String> Map = (Map<String, String>) inheritableThreadLocal.get();
Map<String, String> Map = inheritableThreadLocal.get();
if ((Map != null) && (key != null)) {
return (String) Map.get(key);
return Map.get(key);
} else {
return null;
}
@ -98,8 +108,9 @@ public class BasicMDCAdapter implements MDCAdapter {
/**
* Remove the the context identified by the <code>key</code> parameter.
*/
@Override
public void remove(String key) {
Map<String, String> map = (Map<String, String>) inheritableThreadLocal.get();
Map<String, String> map = inheritableThreadLocal.get();
if (map != null) {
map.remove(key);
}
@ -108,8 +119,9 @@ public class BasicMDCAdapter implements MDCAdapter {
/**
* Clear all entries in the MDC.
*/
@Override
public void clear() {
Map<String, String> map = (Map<String, String>) inheritableThreadLocal.get();
Map<String, String> map = inheritableThreadLocal.get();
if (map != null) {
map.clear();
// the InheritableThreadLocal.remove method was introduced in JDK 1.5
@ -129,34 +141,32 @@ public class BasicMDCAdapter implements MDCAdapter {
* @return the keys in the MDC
*/
public Set<String> getKeys() {
Map<String, String> map = (Map<String, String>) inheritableThreadLocal.get();
Map<String, String> map = inheritableThreadLocal.get();
if (map != null) {
return map.keySet();
} else {
return null;
}
}
/**
* Return a copy of the current thread's context map.
* Returned value may be null.
*
*/
@Override
public Map<String, String> getCopyOfContextMap() {
Map<String, String> oldMap = (Map<String, String>) inheritableThreadLocal.get();
Map<String, String> oldMap = inheritableThreadLocal.get();
if (oldMap != null) {
Map<String, String> newMap = Collections.<String, String>synchronizedMap(new HashMap<String, String>());
synchronized (oldMap) {
newMap.putAll(oldMap);
}
return newMap;
return new HashMap<String, String>(oldMap);
} else {
return null;
}
}
@Override
public void setContextMap(Map<String, String> contextMap) {
Map<String, String> map = Collections.<String, String>synchronizedMap(new HashMap<String, String>(contextMap));
inheritableThreadLocal.set(map);
inheritableThreadLocal.set(new HashMap<String, String>(contextMap));
}
}

139
slf4j-api/src/test/java/org/slf4j/helpers/BasicMDCAdapterTest.java Normal file
View File

@ -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() : "";
}
}
}