Skip to content

Commit 81e1f06

Browse files
ppkarwaszvy
ppkarwasz
and
vy
authored
Fix key removal issues in Thread Context (#3210)
This backports #3050 to the 2.24.x branch. Co-authored-by: Volkan Yazıcı <[email protected]>
1 parent a1dfa85 commit 81e1f06

File tree

4 files changed

+137
-59
lines changed

4 files changed

+137
-59
lines changed

‎log4j-api-test/src/test/java/org/apache/logging/log4j/CloseableThreadContextTest.java‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,9 @@
1919
import static org.junit.jupiter.api.Assertions.assertEquals;
2020
import static org.junit.jupiter.api.Assertions.assertFalse;
2121
import static org.junit.jupiter.api.Assertions.assertNotNull;
22+
import static org.junit.jupiter.api.Assertions.assertNull;
2223

24+
import java.util.Collections;
2325
import java.util.HashMap;
2426
import java.util.List;
2527
import java.util.Map;
@@ -244,4 +246,70 @@ public void pushAllWillPushAllValues() {
244246
}
245247
assertEquals("", ThreadContext.peek());
246248
}
249+
250+
/**
251+
* User provided test stressing nesting using {@link CloseableThreadContext#put(String, String)}.
252+
*
253+
* @see <a href="https://github.com/apache/logging-log4j2/issues/2946#issuecomment-2382935426">#2946</a>
254+
*/
255+
@Test
256+
void testAutoCloseableThreadContextPut() {
257+
try (final CloseableThreadContext.Instance ctc1 = CloseableThreadContext.put("outer", "one")) {
258+
try (final CloseableThreadContext.Instance ctc2 = CloseableThreadContext.put("outer", "two")) {
259+
assertEquals("two", ThreadContext.get("outer"));
260+
261+
try (final CloseableThreadContext.Instance ctc3 = CloseableThreadContext.put("inner", "one")) {
262+
assertEquals("one", ThreadContext.get("inner"));
263+
264+
ThreadContext.put(
265+
"not-in-closeable", "true"); // Remove this line, and closing context behaves as expected
266+
assertEquals("two", ThreadContext.get("outer"));
267+
}
268+
269+
assertEquals("two", ThreadContext.get("outer"));
270+
assertNull(ThreadContext.get("inner")); // Test fails here
271+
}
272+
273+
assertEquals("one", ThreadContext.get("outer"));
274+
assertNull(ThreadContext.get("inner"));
275+
}
276+
assertEquals("true", ThreadContext.get("not-in-closeable"));
277+
278+
assertNull(ThreadContext.get("inner"));
279+
assertNull(ThreadContext.get("outer"));
280+
}
281+
282+
/**
283+
* User provided test stressing nesting using {@link CloseableThreadContext#putAll(Map)}.
284+
*
285+
* @see <a href="https://github.com/apache/logging-log4j2/issues/2946#issuecomment-2382935426">#2946</a>
286+
*/
287+
@Test
288+
void testAutoCloseableThreadContextPutAll() {
289+
try (final CloseableThreadContext.Instance ctc1 = CloseableThreadContext.put("outer", "one")) {
290+
try (final CloseableThreadContext.Instance ctc2 = CloseableThreadContext.put("outer", "two")) {
291+
assertEquals("two", ThreadContext.get("outer"));
292+
293+
try (final CloseableThreadContext.Instance ctc3 = CloseableThreadContext.put("inner", "one")) {
294+
assertEquals("one", ThreadContext.get("inner"));
295+
296+
ThreadContext.put(
297+
"not-in-closeable", "true"); // Remove this line, and closing context behaves as expected
298+
ThreadContext.putAll(Collections.singletonMap("inner", "two")); // But this is not a problem
299+
assertEquals("two", ThreadContext.get("inner"));
300+
assertEquals("two", ThreadContext.get("outer"));
301+
}
302+
303+
assertEquals("two", ThreadContext.get("outer"));
304+
assertNull(ThreadContext.get("inner")); // This is where the test fails
305+
}
306+
307+
assertEquals("one", ThreadContext.get("outer"));
308+
assertNull(ThreadContext.get("inner"));
309+
}
310+
assertEquals("true", ThreadContext.get("not-in-closeable"));
311+
312+
assertNull(ThreadContext.get("inner"));
313+
assertNull(ThreadContext.get("outer"));
314+
}
247315
}

‎log4j-api-test/src/test/java/org/apache/logging/log4j/internal/map/UnmodifiableArrayBackedMapTest.java‎

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@
3434
import java.util.Set;
3535
import org.apache.logging.log4j.util.ReadOnlyStringMap;
3636
import org.apache.logging.log4j.util.TriConsumer;
37+
import org.assertj.core.api.Assertions;
3738
import org.junit.jupiter.api.Test;
3839

3940
public class UnmodifiableArrayBackedMapTest {
@@ -373,4 +374,23 @@ public void testToMap() {
373374
// verify same instance, not just equals()
374375
assertTrue(map == map.toMap());
375376
}
377+
378+
@Test
379+
void copyAndRemoveAll_should_work() {
380+
381+
// Create the actual map
382+
UnmodifiableArrayBackedMap actualMap = UnmodifiableArrayBackedMap.EMPTY_MAP;
383+
actualMap = actualMap.copyAndPut("outer", "two");
384+
actualMap = actualMap.copyAndPut("inner", "one");
385+
actualMap = actualMap.copyAndPut("not-in-closeable", "true");
386+
387+
// Create the expected map
388+
UnmodifiableArrayBackedMap expectedMap = UnmodifiableArrayBackedMap.EMPTY_MAP;
389+
expectedMap = expectedMap.copyAndPut("outer", "two");
390+
expectedMap = expectedMap.copyAndPut("not-in-closeable", "true");
391+
392+
// Remove the key and verify
393+
actualMap = actualMap.copyAndRemoveAll(Collections.singleton("inner"));
394+
Assertions.assertThat(actualMap).isEqualTo(expectedMap);
395+
}
376396
}

‎log4j-api/src/main/java/org/apache/logging/log4j/internal/map/UnmodifiableArrayBackedMap.java‎

Lines changed: 41 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -350,82 +350,64 @@ public UnmodifiableArrayBackedMap copyAndRemove(String key) {
350350
}
351351

352352
/**
353-
* Creates a new instance that contains the same entries as this map, minus all
354-
* of the keys passed in the arguments.
355-
*
356-
* @param key
357-
* @param value
358-
* @return
353+
* Creates a new instance where the entries of provided keys are removed.
359354
*/
360355
public UnmodifiableArrayBackedMap copyAndRemoveAll(Iterable<String> keysToRemoveIterable) {
356+
357+
// Short-circuit if the map is empty
361358
if (isEmpty()) {
362-
// shortcut: if this map is empty, the result will continue to be empty
363359
return EMPTY_MAP;
364360
}
365361

366-
// now we build a Set of keys to remove
367-
Set<String> keysToRemoveSet;
362+
// Collect distinct keys to remove
363+
final Set<String> keysToRemove;
368364
if (keysToRemoveIterable instanceof Set) {
369-
// we already have a set, let's cast it and reuse it
370-
keysToRemoveSet = (Set<String>) keysToRemoveIterable;
365+
keysToRemove = (Set<String>) keysToRemoveIterable;
371366
} else {
372-
// iterate through the keys and build a set
373-
keysToRemoveSet = new HashSet<>();
374-
for (String key : keysToRemoveIterable) {
375-
keysToRemoveSet.add(key);
367+
keysToRemove = new HashSet<>();
368+
for (final String key : keysToRemoveIterable) {
369+
keysToRemove.add(key);
376370
}
377371
}
378372

379-
int firstIndexToKeep = -1;
380-
int lastIndexToKeep = -1;
381-
int destinationIndex = 0;
382-
int numEntriesKept = 0;
383-
// build the new map
384-
UnmodifiableArrayBackedMap newMap = new UnmodifiableArrayBackedMap(numEntries);
385-
for (int indexInCurrentMap = 0; indexInCurrentMap < numEntries; indexInCurrentMap++) {
386-
// for each key in this map, check whether it's in the set we built above
387-
Object key = backingArray[getArrayIndexForKey(indexInCurrentMap)];
388-
if (!keysToRemoveSet.contains(key)) {
389-
// this key should be kept
390-
if (firstIndexToKeep == -1) {
391-
firstIndexToKeep = indexInCurrentMap;
392-
}
393-
lastIndexToKeep = indexInCurrentMap;
394-
} else if (lastIndexToKeep > 0) {
395-
// we hit a remove, copy any keys that are known ready
396-
int numEntriesToCopy = lastIndexToKeep - firstIndexToKeep + 1;
397-
System.arraycopy(
398-
backingArray,
399-
getArrayIndexForKey(firstIndexToKeep),
400-
newMap.backingArray,
401-
getArrayIndexForKey(destinationIndex),
402-
numEntriesToCopy * 2);
403-
firstIndexToKeep = -1;
404-
lastIndexToKeep = -1;
405-
destinationIndex += numEntriesToCopy;
406-
numEntriesKept += numEntriesToCopy;
407-
}
408-
}
373+
// Create the new map
374+
final UnmodifiableArrayBackedMap oldMap = this;
375+
final int oldMapEntryCount = oldMap.numEntries;
376+
final UnmodifiableArrayBackedMap newMap = new UnmodifiableArrayBackedMap(oldMapEntryCount);
409377

410-
if (lastIndexToKeep > -1) {
411-
// at least one key still requires copying
412-
int numEntriesToCopy = lastIndexToKeep - firstIndexToKeep + 1;
413-
System.arraycopy(
414-
backingArray,
415-
getArrayIndexForKey(firstIndexToKeep),
416-
newMap.backingArray,
417-
getArrayIndexForKey(destinationIndex),
418-
numEntriesToCopy * 2);
419-
numEntriesKept += numEntriesToCopy;
378+
// Short-circuit if there is nothing to remove
379+
if (keysToRemove.isEmpty()) {
380+
System.arraycopy(oldMap.backingArray, 0, newMap.backingArray, 0, oldMapEntryCount * 2);
381+
newMap.numEntries = oldMapEntryCount;
382+
return this;
420383
}
421384

422-
if (numEntriesKept == 0) {
423-
return EMPTY_MAP;
385+
// Iterate over old map entries
386+
int newMapEntryIndex = 0;
387+
for (int oldMapEntryIndex = 0; oldMapEntryIndex < oldMapEntryCount; oldMapEntryIndex++) {
388+
final int oldMapKeyIndex = getArrayIndexForKey(oldMapEntryIndex);
389+
final Object key = oldMap.backingArray[oldMapKeyIndex];
390+
391+
// Skip entries of removed keys
392+
@SuppressWarnings("SuspiciousMethodCalls")
393+
final boolean removed = keysToRemove.contains(key);
394+
if (removed) {
395+
continue;
396+
}
397+
398+
// Copy the entry
399+
final int oldMapValueIndex = getArrayIndexForValue(oldMapEntryIndex);
400+
final Object value = oldMap.backingArray[oldMapValueIndex];
401+
final int newMapKeyIndex = getArrayIndexForKey(newMapEntryIndex);
402+
final int newMapValueIndex = getArrayIndexForValue(newMapEntryIndex);
403+
newMap.backingArray[newMapKeyIndex] = key;
404+
newMap.backingArray[newMapValueIndex] = value;
405+
newMapEntryIndex++;
424406
}
425407

426-
newMap.numEntries = numEntriesKept;
408+
// Cap and return the new map
409+
newMap.numEntries = newMapEntryIndex;
427410
newMap.updateNumEntriesInArray();
428-
429411
return newMap;
430412
}
431413

‎src/changelog/.2.x.x/3048_fix_ThreadContext_remove.xml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<?xml version="1.0" encoding="UTF-8"?>
2+
<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
3+
xmlns="https://logging.apache.org/xml/ns"
4+
xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
5+
type="fixed">
6+
<issue id="3048" link="https://github.com/apache/logging-log4j2/pull/3048"/>
7+
<description format="asciidoc">Fix key removal issues in Thread Context</description>
8+
</entry>

0 commit comments

Comments
 (0)