Skip to content

Commit 3210d5e

Browse files
cekiceki
committed
- 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.
1 parent 7b5cbbd commit 3210d5e

5 files changed

Lines changed: 149 additions & 21 deletions

File tree

‎logback-classic/src/main/java/org/slf4j/impl/CopyOnInheritThreadLocal.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package org.slf4j.impl;
2+
3+
import java.util.HashMap;
4+
5+
/**
6+
* This class extends InheritableThreadLocal so that children threads get a copy
7+
* of the parent's hashmap.
8+
*
9+
* @author Ceki Gülcü
10+
*/
11+
public class CopyOnInheritThreadLocal extends
12+
InheritableThreadLocal<HashMap<String, String>> {
13+
14+
/**
15+
* Child threads should get a copy of the parent's hashmap.
16+
*/
17+
@Override
18+
protected HashMap<String, String> childValue(
19+
HashMap<String, String> parentValue) {
20+
HashMap<String, String> hm = new HashMap<String, String>(parentValue);
21+
return hm;
22+
}
23+
24+
}

‎logback-classic/src/main/java/org/slf4j/impl/LogbackMDCAdapter.java‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,9 @@
2424
*/
2525
public class LogbackMDCAdapter implements MDCAdapter {
2626

27-
private final InheritableThreadLocal<HashMap<String, String>> inheritableThreadLocal = new InheritableThreadLocal<HashMap<String, String>>();
27+
//final CopyOnInheritThreadLocal copyOnInheritThreadLocal = new CopyOnInheritThreadLocal();
28+
29+
final CopyOnInheritThreadLocal copyOnInheritThreadLocal = new CopyOnInheritThreadLocal();
2830

2931
LogbackMDCAdapter() {
3032
}
@@ -45,21 +47,21 @@ public class LogbackMDCAdapter implements MDCAdapter {
4547
* logback component to see the latest changes.
4648
*
4749
* @throws IllegalArgumentException
48-
* in case the "key" parameter is null
50+
* in case the "key" parameter is null
4951
*/
5052
public void put(String key, String val) throws IllegalArgumentException {
5153
if (key == null) {
5254
throw new IllegalArgumentException("key cannot be null");
5355
}
5456

55-
HashMap<String, String> oldMap = inheritableThreadLocal.get();
57+
HashMap<String, String> oldMap = copyOnInheritThreadLocal.get();
5658

5759
HashMap<String, String> newMap = new HashMap<String, String>();
5860
if (oldMap != null) {
5961
newMap.putAll(oldMap);
6062
}
6163
// the newMap replaces the old one for serialisation's sake
62-
inheritableThreadLocal.set(newMap);
64+
copyOnInheritThreadLocal.set(newMap);
6365
newMap.put(key, val);
6466
}
6567

@@ -70,7 +72,7 @@ public void put(String key, String val) throws IllegalArgumentException {
7072
* This method has no side effects.
7173
*/
7274
public String get(String key) {
73-
HashMap<String, String> hashMap = inheritableThreadLocal.get();
75+
HashMap<String, String> hashMap = copyOnInheritThreadLocal.get();
7476

7577
if ((hashMap != null) && (key != null)) {
7678
return hashMap.get(key);
@@ -89,26 +91,26 @@ public String get(String key) {
8991
* logback component to see the latest changes.
9092
*/
9193
public void remove(String key) {
92-
HashMap<String, String> oldMap = inheritableThreadLocal.get();
94+
HashMap<String, String> oldMap = copyOnInheritThreadLocal.get();
9395

9496
HashMap<String, String> newMap = new HashMap<String, String>();
9597
if (oldMap != null) {
9698
newMap.putAll(oldMap);
9799
}
98100
// the newMap replaces the old one for serialisation's sake
99-
inheritableThreadLocal.set(newMap);
101+
copyOnInheritThreadLocal.set(newMap);
100102
newMap.remove(key);
101103
}
102104

103105
/**
104106
* Clear all entries in the MDC.
105107
*/
106108
public void clear() {
107-
HashMap<String, String> hashMap = inheritableThreadLocal.get();
109+
HashMap<String, String> hashMap = copyOnInheritThreadLocal.get();
108110

109111
if (hashMap != null) {
110112
hashMap.clear();
111-
inheritableThreadLocal.remove();
113+
copyOnInheritThreadLocal.remove();
112114
}
113115
}
114116

@@ -117,29 +119,28 @@ public void clear() {
117119
* internally.
118120
*/
119121
public Map<String, String> getPropertyMap() {
120-
return inheritableThreadLocal.get();
122+
return copyOnInheritThreadLocal.get();
121123
}
122124

123125
/**
124-
* Return a copy of the current thread's context map.
125-
* Returned value may be null.
126+
* Return a copy of the current thread's context map. Returned value may be
127+
* null.
126128
*/
127129
public Map getCopyOfContextMap() {
128-
HashMap<String, String> hashMap = inheritableThreadLocal.get();
130+
HashMap<String, String> hashMap = copyOnInheritThreadLocal.get();
129131
if (hashMap == null) {
130132
return null;
131133
} else {
132134
return new HashMap<String, String>(hashMap);
133135
}
134136
}
135137

136-
137138
/**
138139
* Returns the keys in the MDC as a {@link Set}. The returned value can be
139140
* null.
140141
*/
141142
public Set<String> getKeys() {
142-
HashMap<String, String> hashMap = inheritableThreadLocal.get();
143+
HashMap<String, String> hashMap = copyOnInheritThreadLocal.get();
143144

144145
if (hashMap != null) {
145146
return hashMap.keySet();
@@ -148,17 +149,16 @@ public Set<String> getKeys() {
148149
}
149150
}
150151

151-
152-
@SuppressWarnings("unchecked")
152+
@SuppressWarnings("unchecked")
153153
public void setContextMap(Map contextMap) {
154-
HashMap<String, String> oldMap = inheritableThreadLocal.get();
154+
HashMap<String, String> oldMap = copyOnInheritThreadLocal.get();
155155

156156
HashMap<String, String> newMap = new HashMap<String, String>();
157157
newMap.putAll(contextMap);
158158

159159
// the newMap replaces the old one for serialisation's sake
160-
inheritableThreadLocal.set(newMap);
161-
160+
copyOnInheritThreadLocal.set(newMap);
161+
162162
// hints for the garbage collector
163163
oldMap.clear();
164164
oldMap = null;

‎logback-classic/src/test/java/ch/qos/logback/classic/AllClassicTest.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,8 @@ public class AllClassicTest extends TestCase {
1717

1818
public static Test suite() {
1919
TestSuite suite = new TestSuite();
20-
20+
21+
suite.addTest(org.slf4j.impl.PackageTest.suite());
2122
suite.addTest(ch.qos.logback.classic.PackageTest.suite());
2223
suite.addTest(ch.qos.logback.classic.control.PackageTest.suite());
2324
suite.addTest(ch.qos.logback.classic.joran.PackageTest.suite());

‎logback-classic/src/test/java/org/slf4j/impl/LogbackMDCAdapterTest.java‎

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,82 @@
1+
/**
2+
* Logback: the generic, reliable, fast and flexible logging framework.
3+
*
4+
* Copyright (C) 2000-2008, QOS.ch
5+
*
6+
* This library is free software, you can redistribute it and/or modify it under
7+
* the terms of the GNU Lesser General Public License as published by the Free
8+
* Software Foundation.
9+
*/
10+
package org.slf4j.impl;
11+
12+
import static org.junit.Assert.assertEquals;
13+
import static org.junit.Assert.assertNotNull;
14+
import static org.junit.Assert.assertNull;
15+
import static org.junit.Assert.assertTrue;
16+
17+
import java.util.HashMap;
18+
import java.util.Random;
19+
20+
import org.junit.Test;
21+
import org.slf4j.MDC;
22+
23+
public class LogbackMDCAdapterTest {
24+
25+
final static String A_SUFFIX = "A_SUFFIX";
26+
27+
int diff = new Random().nextInt();
28+
29+
/**
30+
* Test that LogbackMDCAdapter copies its hashmap when a child
31+
* thread inherits it.
32+
*
33+
* @throws InterruptedException
34+
*/
35+
@Test
36+
public void copyOnInheritence() throws InterruptedException {
37+
String mdcKey = "x" + diff;
38+
String otherMDCKey = "o" + diff;
39+
MDC.put(mdcKey, mdcKey + A_SUFFIX);
40+
41+
HashMap<String, String> parentHM = getHM();
42+
43+
MyThread myThread = new MyThread(mdcKey, otherMDCKey);
44+
myThread.start();
45+
myThread.join();
46+
47+
assertNull(MDC.get(otherMDCKey));
48+
assertTrue(myThread.successul);
49+
assertTrue(parentHM != myThread.childHM);
50+
}
51+
52+
class MyThread extends Thread {
53+
54+
String mdcKey;
55+
String otherMDCKey;
56+
boolean successul;
57+
HashMap<String, String> childHM;
58+
59+
MyThread(String mdcKey, String otherMDCKey) {
60+
this.mdcKey = mdcKey;
61+
this.otherMDCKey = otherMDCKey;
62+
}
63+
64+
@Override
65+
public void run() {
66+
childHM = getHM();
67+
68+
MDC.put(otherMDCKey, otherMDCKey + A_SUFFIX);
69+
assertNotNull(MDC.get(mdcKey));
70+
assertEquals(mdcKey + A_SUFFIX, MDC.get(mdcKey));
71+
assertEquals(otherMDCKey + A_SUFFIX, MDC.get(otherMDCKey));
72+
successul = true;
73+
}
74+
}
75+
76+
HashMap<String, String> getHM() {
77+
LogbackMDCAdapter lma = (LogbackMDCAdapter) MDC.getMDCAdapter();
78+
CopyOnInheritThreadLocal copyOnInheritThreadLocal = lma.copyOnInheritThreadLocal;
79+
return copyOnInheritThreadLocal.get();
80+
81+
}
82+
}

‎logback-classic/src/test/java/org/slf4j/impl/PackageTest.java‎

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,21 @@
1+
/**
2+
* Logback: the generic, reliable, fast and flexible logging framework.
3+
*
4+
* Copyright (C) 2000-2008, QOS.ch
5+
*
6+
* This library is free software, you can redistribute it and/or modify it under
7+
* the terms of the GNU Lesser General Public License as published by the Free
8+
* Software Foundation.
9+
*/
10+
package org.slf4j.impl;
11+
12+
import junit.framework.*;
13+
14+
public class PackageTest extends TestCase {
15+
16+
public static Test suite() {
17+
TestSuite suite = new TestSuite();
18+
suite.addTest(new JUnit4TestAdapter(LogbackMDCAdapterTest.class));
19+
return suite;
20+
}
21+
}

0 commit comments

Comments
 (0)