Merge branch 'apache:2.x' into refactor-1.2-api-junit5

Author: ninetteadhikari

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

@@ -19,7 +19,9 @@
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertNull;
 
+import java.util.Collections;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -244,4 +246,70 @@ public void pushAllWillPushAllValues() {
         }
         assertEquals("", ThreadContext.peek());
     }
+
+    /**
+     * User provided test stressing nesting using {@link CloseableThreadContext#put(String, String)}.
+     *
+     * @see <a href="https://github.com/apache/logging-log4j2/issues/2946#issuecomment-2382935426">#2946</a>
+     */
+    @Test
+    void testAutoCloseableThreadContextPut() {
+        try (final CloseableThreadContext.Instance ctc1 = CloseableThreadContext.put("outer", "one")) {
+            try (final CloseableThreadContext.Instance ctc2 = CloseableThreadContext.put("outer", "two")) {
+                assertEquals("two", ThreadContext.get("outer"));
+
+                try (final CloseableThreadContext.Instance ctc3 = CloseableThreadContext.put("inner", "one")) {
+                    assertEquals("one", ThreadContext.get("inner"));
+
+                    ThreadContext.put(
+                            "not-in-closeable", "true"); // Remove this line, and closing context behaves as expected
+                    assertEquals("two", ThreadContext.get("outer"));
+                }
+
+                assertEquals("two", ThreadContext.get("outer"));
+                assertNull(ThreadContext.get("inner")); // Test fails here
+            }
+
+            assertEquals("one", ThreadContext.get("outer"));
+            assertNull(ThreadContext.get("inner"));
+        }
+        assertEquals("true", ThreadContext.get("not-in-closeable"));
+
+        assertNull(ThreadContext.get("inner"));
+        assertNull(ThreadContext.get("outer"));
+    }
+
+    /**
+     * User provided test stressing nesting using {@link CloseableThreadContext#putAll(Map)}.
+     *
+     * @see <a href="https://github.com/apache/logging-log4j2/issues/2946#issuecomment-2382935426">#2946</a>
+     */
+    @Test
+    void testAutoCloseableThreadContextPutAll() {
+        try (final CloseableThreadContext.Instance ctc1 = CloseableThreadContext.put("outer", "one")) {
+            try (final CloseableThreadContext.Instance ctc2 = CloseableThreadContext.put("outer", "two")) {
+                assertEquals("two", ThreadContext.get("outer"));
+
+                try (final CloseableThreadContext.Instance ctc3 = CloseableThreadContext.put("inner", "one")) {
+                    assertEquals("one", ThreadContext.get("inner"));
+
+                    ThreadContext.put(
+                            "not-in-closeable", "true"); // Remove this line, and closing context behaves as expected
+                    ThreadContext.putAll(Collections.singletonMap("inner", "two")); // But this is not a problem
+                    assertEquals("two", ThreadContext.get("inner"));
+                    assertEquals("two", ThreadContext.get("outer"));
+                }
+
+                assertEquals("two", ThreadContext.get("outer"));
+                assertNull(ThreadContext.get("inner")); // This is where the test fails
+            }
+
+            assertEquals("one", ThreadContext.get("outer"));
+            assertNull(ThreadContext.get("inner"));
+        }
+        assertEquals("true", ThreadContext.get("not-in-closeable"));
+
+        assertNull(ThreadContext.get("inner"));
+        assertNull(ThreadContext.get("outer"));
+    }
 }

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

@@ -34,6 +34,7 @@
 import java.util.Set;
 import org.apache.logging.log4j.util.ReadOnlyStringMap;
 import org.apache.logging.log4j.util.TriConsumer;
+import org.assertj.core.api.Assertions;
 import org.junit.jupiter.api.Test;
 
 public class UnmodifiableArrayBackedMapTest {
@@ -373,4 +374,23 @@ public void testToMap() {
         // verify same instance, not just equals()
         assertTrue(map == map.toMap());
     }
+
+    @Test
+    void copyAndRemoveAll_should_work() {
+
+        // Create the actual map
+        UnmodifiableArrayBackedMap actualMap = UnmodifiableArrayBackedMap.EMPTY_MAP;
+        actualMap = actualMap.copyAndPut("outer", "two");
+        actualMap = actualMap.copyAndPut("inner", "one");
+        actualMap = actualMap.copyAndPut("not-in-closeable", "true");
+
+        // Create the expected map
+        UnmodifiableArrayBackedMap expectedMap = UnmodifiableArrayBackedMap.EMPTY_MAP;
+        expectedMap = expectedMap.copyAndPut("outer", "two");
+        expectedMap = expectedMap.copyAndPut("not-in-closeable", "true");
+
+        // Remove the key and verify
+        actualMap = actualMap.copyAndRemoveAll(Collections.singleton("inner"));
+        Assertions.assertThat(actualMap).isEqualTo(expectedMap);
+    }
 }

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

@@ -350,82 +350,64 @@ public UnmodifiableArrayBackedMap copyAndRemove(String key) {
     }
 
     /**
-     * Creates a new instance that contains the same entries as this map, minus all
-     * of the keys passed in the arguments.
-     *
-     * @param key
-     * @param value
-     * @return
+     * Creates a new instance where the entries of provided keys are removed.
      */
     public UnmodifiableArrayBackedMap copyAndRemoveAll(Iterable<String> keysToRemoveIterable) {
+
+        // Short-circuit if the map is empty
         if (isEmpty()) {
-            // shortcut: if this map is empty, the result will continue to be empty
             return EMPTY_MAP;
         }
 
-        // now we build a Set of keys to remove
-        Set<String> keysToRemoveSet;
+        // Collect distinct keys to remove
+        final Set<String> keysToRemove;
         if (keysToRemoveIterable instanceof Set) {
-            // we already have a set, let's cast it and reuse it
-            keysToRemoveSet = (Set<String>) keysToRemoveIterable;
+            keysToRemove = (Set<String>) keysToRemoveIterable;
         } else {
-            // iterate through the keys and build a set
-            keysToRemoveSet = new HashSet<>();
-            for (String key : keysToRemoveIterable) {
-                keysToRemoveSet.add(key);
+            keysToRemove = new HashSet<>();
+            for (final String key : keysToRemoveIterable) {
+                keysToRemove.add(key);
             }
         }
 
-        int firstIndexToKeep = -1;
-        int lastIndexToKeep = -1;
-        int destinationIndex = 0;
-        int numEntriesKept = 0;
-        // build the new map
-        UnmodifiableArrayBackedMap newMap = new UnmodifiableArrayBackedMap(numEntries);
-        for (int indexInCurrentMap = 0; indexInCurrentMap < numEntries; indexInCurrentMap++) {
-            // for each key in this map, check whether it's in the set we built above
-            Object key = backingArray[getArrayIndexForKey(indexInCurrentMap)];
-            if (!keysToRemoveSet.contains(key)) {
-                // this key should be kept
-                if (firstIndexToKeep == -1) {
-                    firstIndexToKeep = indexInCurrentMap;
-                }
-                lastIndexToKeep = indexInCurrentMap;
-            } else if (lastIndexToKeep > 0) {
-                // we hit a remove, copy any keys that are known ready
-                int numEntriesToCopy = lastIndexToKeep - firstIndexToKeep + 1;
-                System.arraycopy(
-                        backingArray,
-                        getArrayIndexForKey(firstIndexToKeep),
-                        newMap.backingArray,
-                        getArrayIndexForKey(destinationIndex),
-                        numEntriesToCopy * 2);
-                firstIndexToKeep = -1;
-                lastIndexToKeep = -1;
-                destinationIndex += numEntriesToCopy;
-                numEntriesKept += numEntriesToCopy;
-            }
-        }
+        // Create the new map
+        final UnmodifiableArrayBackedMap oldMap = this;
+        final int oldMapEntryCount = oldMap.numEntries;
+        final UnmodifiableArrayBackedMap newMap = new UnmodifiableArrayBackedMap(oldMapEntryCount);
 
-        if (lastIndexToKeep > -1) {
-            // at least one key still requires copying
-            int numEntriesToCopy = lastIndexToKeep - firstIndexToKeep + 1;
-            System.arraycopy(
-                    backingArray,
-                    getArrayIndexForKey(firstIndexToKeep),
-                    newMap.backingArray,
-                    getArrayIndexForKey(destinationIndex),
-                    numEntriesToCopy * 2);
-            numEntriesKept += numEntriesToCopy;
+        // Short-circuit if there is nothing to remove
+        if (keysToRemove.isEmpty()) {
+            System.arraycopy(oldMap.backingArray, 0, newMap.backingArray, 0, oldMapEntryCount * 2);
+            newMap.numEntries = oldMapEntryCount;
+            return this;
         }
 
-        if (numEntriesKept == 0) {
-            return EMPTY_MAP;
+        // Iterate over old map entries
+        int newMapEntryIndex = 0;
+        for (int oldMapEntryIndex = 0; oldMapEntryIndex < oldMapEntryCount; oldMapEntryIndex++) {
+            final int oldMapKeyIndex = getArrayIndexForKey(oldMapEntryIndex);
+            final Object key = oldMap.backingArray[oldMapKeyIndex];
+
+            // Skip entries of removed keys
+            @SuppressWarnings("SuspiciousMethodCalls")
+            final boolean removed = keysToRemove.contains(key);
+            if (removed) {
+                continue;
+            }
+
+            // Copy the entry
+            final int oldMapValueIndex = getArrayIndexForValue(oldMapEntryIndex);
+            final Object value = oldMap.backingArray[oldMapValueIndex];
+            final int newMapKeyIndex = getArrayIndexForKey(newMapEntryIndex);
+            final int newMapValueIndex = getArrayIndexForValue(newMapEntryIndex);
+            newMap.backingArray[newMapKeyIndex] = key;
+            newMap.backingArray[newMapValueIndex] = value;
+            newMapEntryIndex++;
         }
 
-        newMap.numEntries = numEntriesKept;
+        // Cap and return the new map
+        newMap.numEntries = newMapEntryIndex;
         newMap.updateNumEntriesInArray();
-
         return newMap;
     }
 

log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/XmlConfiguration.java

@@ -216,14 +216,16 @@ private static void setFeature(
      */
     private static void enableXInclude(final DocumentBuilderFactory factory) {
         try {
-            // Alternative: We set if a system property on the command line is set, for example:
-            // -DLog4j.XInclude=true
             factory.setXIncludeAware(true);
             // LOG4J2-3531: Xerces only checks if the feature is supported when creating a factory. To reproduce:
             // -Dorg.apache.xerces.xni.parser.XMLParserConfiguration=org.apache.xerces.parsers.XML11NonValidatingConfiguration
-            factory.newDocumentBuilder();
-        } catch (final UnsupportedOperationException | ParserConfigurationException e) {
-            factory.setXIncludeAware(false);
+            try {
+                factory.newDocumentBuilder();
+            } catch (final ParserConfigurationException e) {
+                factory.setXIncludeAware(false);
+                LOGGER.warn("The DocumentBuilderFactory [{}] does not support XInclude: {}", factory, e);
+            }
+        } catch (final UnsupportedOperationException e) {
             LOGGER.warn("The DocumentBuilderFactory [{}] does not support XInclude: {}", factory, e);
         } catch (final AbstractMethodError | NoSuchMethodError err) {
             LOGGER.warn(

log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jContextFactory.java

@@ -29,13 +29,15 @@
 import org.apache.logging.log4j.core.config.ConfigurationSource;
 import org.apache.logging.log4j.core.config.DefaultConfiguration;
 import org.apache.logging.log4j.core.config.composite.CompositeConfiguration;
+import org.apache.logging.log4j.core.selector.BasicContextSelector;
 import org.apache.logging.log4j.core.selector.ClassLoaderContextSelector;
 import org.apache.logging.log4j.core.selector.ContextSelector;
 import org.apache.logging.log4j.core.util.Cancellable;
 import org.apache.logging.log4j.core.util.Constants;
 import org.apache.logging.log4j.core.util.DefaultShutdownCallbackRegistry;
 import org.apache.logging.log4j.core.util.Loader;
 import org.apache.logging.log4j.core.util.ShutdownCallbackRegistry;
+import org.apache.logging.log4j.core.util.internal.SystemUtils;
 import org.apache.logging.log4j.spi.LoggerContextFactory;
 import org.apache.logging.log4j.status.StatusLogger;
 import org.apache.logging.log4j.util.PropertiesUtil;
@@ -104,7 +106,12 @@ private static ContextSelector createContextSelector() {
         } catch (final Exception e) {
             LOGGER.error("Unable to create custom ContextSelector. Falling back to default.", e);
         }
-        return new ClassLoaderContextSelector();
+        // StackLocator is broken on Android:
+        // 1. Android could use the StackLocator implementation for JDK 11, but does not support multi-release JARs.
+        // 2. Android does not have the `sun.reflect` classes used in the JDK 8 implementation.
+        //
+        // Therefore, we use a single logger context.
+        return SystemUtils.isOsAndroid() ? new BasicContextSelector() : new ClassLoaderContextSelector();
     }
 
     private static ShutdownCallbackRegistry createShutdownCallbackRegistry() {

log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/JmxRuntimeInputArgumentsLookup.java

@@ -17,10 +17,13 @@
 package org.apache.logging.log4j.core.lookup;
 
 import java.lang.management.ManagementFactory;
-import java.util.List;
+import java.util.Collections;
 import java.util.Map;
+import org.apache.logging.log4j.Logger;
 import org.apache.logging.log4j.core.LogEvent;
 import org.apache.logging.log4j.core.config.plugins.Plugin;
+import org.apache.logging.log4j.core.util.internal.SystemUtils;
+import org.apache.logging.log4j.status.StatusLogger;
 
 /**
  * Maps JVM input arguments (but not main arguments) using JMX to acquire JVM arguments.
@@ -31,17 +34,16 @@
 @Plugin(name = "jvmrunargs", category = StrLookup.CATEGORY)
 public class JmxRuntimeInputArgumentsLookup extends MapLookup {
 
-    static {
-        final List<String> argsList = ManagementFactory.getRuntimeMXBean().getInputArguments();
-        JMX_SINGLETON = new JmxRuntimeInputArgumentsLookup(MapLookup.toMap(argsList));
-    }
+    private static final Logger LOGGER = StatusLogger.getLogger();
 
-    public static final JmxRuntimeInputArgumentsLookup JMX_SINGLETON;
+    public static final JmxRuntimeInputArgumentsLookup JMX_SINGLETON = new JmxRuntimeInputArgumentsLookup();
 
     /**
      * Constructor when used directly as a plugin.
      */
-    public JmxRuntimeInputArgumentsLookup() {}
+    public JmxRuntimeInputArgumentsLookup() {
+        this(getMapFromJmx());
+    }
 
     public JmxRuntimeInputArgumentsLookup(final Map<String, String> map) {
         super(map);
@@ -55,4 +57,15 @@ public String lookup(final LogEvent ignored, final String key) {
         final Map<String, String> map = getMap();
         return map == null ? null : map.get(key);
     }
+
+    private static Map<String, String> getMapFromJmx() {
+        if (!SystemUtils.isOsAndroid()) {
+            try {
+                return MapLookup.toMap(ManagementFactory.getRuntimeMXBean().getInputArguments());
+            } catch (LinkageError e) {
+                LOGGER.warn("Failed to get JMX arguments from JVM.", e);
+            }
+        }
+        return Collections.emptyMap();
+    }
 }

log4j-core/src/main/java/org/apache/logging/log4j/core/util/internal/SystemUtils.java

@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to you under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *      http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.logging.log4j.core.util.internal;
+
+import org.apache.logging.log4j.Logger;
+import org.apache.logging.log4j.status.StatusLogger;
+
+public final class SystemUtils {
+
+    private static final Logger LOGGER = StatusLogger.getLogger();
+
+    private static String getJavaVendor() {
+        try {
+            return System.getProperty("java.vendor");
+        } catch (final SecurityException e) {
+            LOGGER.warn("Unable to determine Java vendor.", e);
+        }
+        return "Unknown";
+    }
+
+    public static boolean isOsAndroid() {
+        return getJavaVendor().contains("Android");
+    }
+
+    private SystemUtils() {}
+}

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

@@ -0,0 +1,8 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<entry xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+       xmlns="https://logging.apache.org/xml/ns"
+       xsi:schemaLocation="https://logging.apache.org/xml/ns https://logging.apache.org/xml/ns/log4j-changelog-0.xsd"
+       type="fixed">
+  <issue id="3048" link="https://github.com/apache/logging-log4j2/pull/3048"/>
+  <description format="asciidoc">Fix key removal issues in Thread Context</description>
+</entry>