Prevent log4j2.isWebapp from overriding other constants

ppkarwasz · ppkarwasz · commit b21ce8782a9d · 2024-04-25T13:47:23.000+02:00
Currently, if `log4j2.isWebapp` is enabled, the effective values of the
following system properties:

* `log4j2.enableThreadlocals`,
* `log4j2.garbagefreeThreadLocalMap`,
* `log4j2.shutdownHookEnabled`.

is set to `false`.

After this PR:

* the above mentioned constants are set to the value **explicitly** set
  by the user,
* if the user didn't provide any value `!log4j2.isWebapp` is used.
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/spi/Provider.java b/log4j-api/src/main/java/org/apache/logging/log4j/spi/Provider.java
@@ -326,12 +326,10 @@ public String getThreadContextMap() {
             return threadContextMapClass;
         }
         // Default based on properties
-        if (Constants.ENABLE_THREADLOCALS) {
-            return props.getBooleanProperty(GC_FREE_THREAD_CONTEXT_PROPERTY)
-                    ? GC_FREE_THREAD_CONTEXT_PROPERTY
-                    : COPY_ON_WRITE_CONTEXT_MAP;
+        if (props.getBooleanProperty(GC_FREE_THREAD_CONTEXT_PROPERTY, !Constants.IS_WEB_APP)) {
+            return GC_FREE_THREAD_CONTEXT_PROPERTY;
         }
-        return WEB_APP_CONTEXT_MAP;
+        return Constants.IS_WEB_APP ? WEB_APP_CONTEXT_MAP : COPY_ON_WRITE_CONTEXT_MAP;
     }
 
     /**
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/util/Constants.java b/log4j-api/src/main/java/org/apache/logging/log4j/util/Constants.java
@@ -16,31 +16,56 @@
  */
 package org.apache.logging.log4j.util;
 
+import org.apache.logging.log4j.spi.Provider;
+
 /**
  * Log4j API Constants.
  *
  * @since 2.6.2
  */
 public final class Constants {
     /**
-     * {@code true} if we think we are running in a web container, based on the boolean value of system property
-     * "log4j2.is.webapp", or (if this system property is not set) whether the  {@code javax.servlet.Servlet} class
-     * is present in the classpath.
+     * Specifies whether Log4j is used in a servlet container
+     * <p>
+     *     If {@code true} Log4j disables the features, which are incompatible with a typical servlet application:
+     * </p>
+     * <ol>
+     *     <li>It disables the usage of {@link ThreadLocal}s for object pooling (see {@link #ENABLE_THREADLOCALS},</li>
+     *     <li>It uses a web-application safe implementation of {@link org.apache.logging.log4j.spi.ThreadContextMap}
+     *     (see {@link Provider#getThreadContextMap()}),</li>
+     *     <li>It disables the shutdown hook,</li>
+     *     <li>It uses the caller thread to send JMX notifications.</li>
+     * </ol>
+     * <p>
+     *     The value of this constant depends upon the presence of the Servlet API on the classpath and can be
+     *     overridden using the {@code "log4j2.isWebapp"} system property.
+     * </p>
      */
     public static final boolean IS_WEB_APP = PropertiesUtil.getProperties()
             .getBooleanProperty(
                     "log4j2.is.webapp",
                     isClassAvailable("javax.servlet.Servlet") || isClassAvailable("jakarta.servlet.Servlet"));
 
     /**
-     * Kill switch for object pooling in ThreadLocals that enables much of the LOG4J2-1270 no-GC behaviour.
+     * Kill switch to disable the usage of {@link ThreadLocal}s for object pooling
+     * <p>
+     *     The value of this constant is {@code true}, unless Log4j is running in a servlet container (cf.
+     *     {@link #IS_WEB_APP}). Use the {@code "log4j2.enableThreadlocals} system property to override its value.
+     * </p>
+     * <p>
+     *     In order to enable the garbage-free behavior described in
+     *     <a href="https://issues.apache.org/jira/browse/LOG4J2-1270">LOG4J2-1270</a>, this constant must be {@code
+     *     true}.
+     * </p>
      * <p>
-     * {@code True} for non-{@link #IS_WEB_APP web apps}, disable by setting system property
-     * "log4j2.enable.threadlocals" to "false".
+     *     <strong>Warning:</strong> This setting does <strong>not</strong> disable the usage of {@code ThreadLocal}s
+     *     for other purposes than object pooling. For example the {@link org.apache.logging.log4j.ThreadContext}
+     *     API, will user {@code ThreadLocal}s even if this constant is set to {@code false}.
      * </p>
+     * @see Provider#getThreadContextMap()
      */
     public static final boolean ENABLE_THREADLOCALS =
-            !IS_WEB_APP && PropertiesUtil.getProperties().getBooleanProperty("log4j2.enable.threadlocals", true);
+            PropertiesUtil.getProperties().getBooleanProperty("log4j2.enable.threadlocals", !IS_WEB_APP);
 
     /**
      * Java major version.
diff --git a/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/GcFreeLoggingTestUtil.java b/log4j-core-test/src/main/java/org/apache/logging/log4j/core/test/GcFreeLoggingTestUtil.java
@@ -17,7 +17,6 @@
 package org.apache.logging.log4j.core.test;
 
 import static org.assertj.core.api.Assertions.assertThat;
-import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 import com.google.monitoring.runtime.instrumentation.AllocationRecorder;
@@ -47,15 +46,13 @@ public enum GcFreeLoggingTestUtil {
 
     public static void executeLogging(final String configurationFile, final Class<?> testClass) throws Exception {
 
-        System.setProperty("log4j2.enable.threadlocals", "true");
-        System.setProperty("log4j2.enable.direct.encoders", "true");
-        System.setProperty("log4j2.is.webapp", "false");
-        System.setProperty("log4j.configurationFile", configurationFile);
+        System.setProperty("log4j2.enableThreadlocals", "true");
+        System.setProperty("log4j2.enableDirectEncoders", "true");
+        System.setProperty("log4j2.configurationFile", configurationFile);
         System.setProperty("log4j2.clock", "SystemMillisClock");
 
         assertTrue(Constants.ENABLE_THREADLOCALS, "Constants.ENABLE_THREADLOCALS");
         assertTrue(Constants.ENABLE_DIRECT_ENCODERS, "Constants.ENABLE_DIRECT_ENCODERS");
-        assertFalse(Constants.IS_WEB_APP, "Constants.IS_WEB_APP");
 
         final MyCharSeq myCharSeq = new MyCharSeq();
         final Marker testGrandParent = MarkerManager.getMarker("testGrandParent");
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/EventParameterMemoryLeakTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/EventParameterMemoryLeakTest.java
@@ -39,9 +39,8 @@ public class EventParameterMemoryLeakTest {
 
     @BeforeAll
     public static void beforeClass() {
-        System.setProperty("log4j2.enable.threadlocals", "true");
-        System.setProperty("log4j2.enable.direct.encoders", "true");
-        System.setProperty("log4j2.is.webapp", "false");
+        System.setProperty("log4j2.enableThreadlocals", "true");
+        System.setProperty("log4j2.enableDirectEncoders", "true");
         System.setProperty(ConfigurationFactory.CONFIGURATION_FILE_PROPERTY, "EventParameterMemoryLeakTest.xml");
     }
 
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AbstractAsyncThreadContextTestBase.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AbstractAsyncThreadContextTestBase.java
@@ -59,9 +59,9 @@ public abstract class AbstractAsyncThreadContextTestBase {
 
     @BeforeAll
     public static void beforeClass() {
-        props.setProperty("log4j2.is.webapp", false);
-        props.setProperty("AsyncLogger.RingBufferSize", 128); // minimum ringbuffer size
-        props.setProperty("AsyncLoggerConfig.RingBufferSize", 128); // minimum ringbuffer size
+        props.setProperty("log4j2.enableThreadlocals", true);
+        props.setProperty("log4j2.asyncLoggerRingBufferSize", 128); // minimum ringbuffer size
+        props.setProperty("log4j2.asyncLoggerConfigRingBufferSize", 128); // minimum ringbuffer size
     }
 
     protected enum Mode {
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerArgumentFreedOnErrorTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerArgumentFreedOnErrorTest.java
@@ -37,10 +37,9 @@ public class AsyncLoggerArgumentFreedOnErrorTest {
 
     @BeforeClass
     public static void beforeClass() {
-        System.setProperty("log4j2.enable.threadlocals", "true");
-        System.setProperty("log4j2.enable.direct.encoders", "true");
-        System.setProperty("log4j2.is.webapp", "false");
-        System.setProperty("log4j.format.msg.async", "true");
+        System.setProperty("log4j2.enableThreadlocals", "true");
+        System.setProperty("log4j2.enableDirectEncoders", "true");
+        System.setProperty("log4j2.formatMsgAsync", "true");
         System.setProperty(Constants.LOG4J_CONTEXT_SELECTOR, AsyncLoggerContextSelector.class.getName());
     }
 
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerConfigErrorOnFormat.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerConfigErrorOnFormat.java
@@ -42,15 +42,15 @@ public class AsyncLoggerConfigErrorOnFormat {
 
     @BeforeClass
     public static void beforeClass() {
-        System.setProperty("log4j2.is.webapp", "false");
+        System.setProperty("log4j2.enableThreadlocals", "true");
         System.setProperty(ConfigurationFactory.CONFIGURATION_FILE_PROPERTY, "AsyncLoggerConfigErrorOnFormat.xml");
         // Log4jLogEvent.toString invokes message.toString
         System.setProperty("log4j2.logEventFactory", DefaultLogEventFactory.class.getName());
     }
 
     @AfterClass
     public static void afterClass() {
-        System.clearProperty("log4j2.is.webapp");
+        System.clearProperty("log4j2.enableThreadlocals");
         System.clearProperty("log4j2.logEventFactory");
     }
 
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerConfigWithAsyncEnabledTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/async/AsyncLoggerConfigWithAsyncEnabledTest.java
@@ -38,16 +38,16 @@ public class AsyncLoggerConfigWithAsyncEnabledTest {
 
     @BeforeClass
     public static void beforeClass() {
-        System.setProperty("log4j2.is.webapp", "false");
-        System.setProperty("Log4jContextSelector", AsyncLoggerContextSelector.class.getCanonicalName());
+        System.setProperty("log4j2.enableThreadlocals", "true");
+        System.setProperty("log4j2.contextSelector", AsyncLoggerContextSelector.class.getCanonicalName());
         // Reuse the configuration from AsyncLoggerConfigTest4
         System.setProperty(ConfigurationFactory.CONFIGURATION_FILE_PROPERTY, "AsyncLoggerConfigTest4.xml");
     }
 
     @AfterClass
     public static void afterClass() {
-        System.clearProperty("log4j2.is.webapp");
-        System.clearProperty("Log4jContextSelector");
+        System.clearProperty("log4j2.enableThreadlocals");
+        System.clearProperty("log4j2.contextSelector");
     }
 
     @Test
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/NestedLoggingFromThrowableMessageTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/impl/NestedLoggingFromThrowableMessageTest.java
@@ -47,7 +47,7 @@ public class NestedLoggingFromThrowableMessageTest {
     public static void beforeClass() {
         file1.delete();
         file2.delete();
-        System.setProperty("log4j2.is.webapp", "false");
+        System.setProperty("log4j2.enableThreadlocals", "true");
     }
 
     @Rule
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/DatePatternConverterWithThreadLocalsTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/pattern/DatePatternConverterWithThreadLocalsTest.java
@@ -19,8 +19,7 @@
 import org.apache.logging.log4j.test.junit.SetTestProperty;
 import org.apache.logging.log4j.test.junit.UsingTestProperties;
 
-@SetTestProperty(key = "log4j2.is.webapp", value = "false")
-@SetTestProperty(key = "log4j2.enable.threadlocals", value = "true")
+@SetTestProperty(key = "log4j2.enableThreadlocals", value = "true")
 @UsingTestProperties
 class DatePatternConverterWithThreadLocalsTest extends DatePatternConverterTestBase {
 
diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/ShutdownCallbackRegistryTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/ShutdownCallbackRegistryTest.java
@@ -39,14 +39,14 @@ public class ShutdownCallbackRegistryTest {
 
     @BeforeAll
     public static void setUpClass() {
-        System.setProperty("log4j2.is.webapp", "false");
+        System.setProperty("log4j2.shutdownHookEnabled", "true");
         System.setProperty(ShutdownCallbackRegistry.SHUTDOWN_CALLBACK_REGISTRY, Registry.class.getName());
     }
 
     @AfterAll
     public static void afterClass() {
         System.clearProperty(ShutdownCallbackRegistry.SHUTDOWN_CALLBACK_REGISTRY);
-        System.clearProperty("log4j2.is.webapp");
+        System.clearProperty("log4j2.shutdownHookEnabled");
     }
 
     @Test
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jContextFactory.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/impl/Log4jContextFactory.java
@@ -46,9 +46,8 @@
 public class Log4jContextFactory implements LoggerContextFactory, ShutdownCallbackRegistry {
 
     private static final StatusLogger LOGGER = StatusLogger.getLogger();
-    private static final boolean SHUTDOWN_HOOK_ENABLED =
-            PropertiesUtil.getProperties().getBooleanProperty(ShutdownCallbackRegistry.SHUTDOWN_HOOK_ENABLED, true)
-                    && !Constants.IS_WEB_APP;
+    private static final boolean SHUTDOWN_HOOK_ENABLED = PropertiesUtil.getProperties()
+            .getBooleanProperty(ShutdownCallbackRegistry.SHUTDOWN_HOOK_ENABLED, !Constants.IS_WEB_APP);
 
     private final ContextSelector selector;
     private final ShutdownCallbackRegistry shutdownCallbackRegistry;
diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Constants.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Constants.java
@@ -74,7 +74,7 @@ public final class Constants {
 
     /**
      * {@code true} if we think we are running in a web container, based on the boolean value of system property
-     * "log4j2.is.webapp", or (if this system property is not set) whether the  {@code javax.servlet.Servlet} class
+     * "log4j2.isWebapp", or (if this system property is not set) whether the  {@code javax.servlet.Servlet} class
      * is present in the classpath.
      */
     public static final boolean IS_WEB_APP = org.apache.logging.log4j.util.Constants.IS_WEB_APP;
@@ -94,9 +94,9 @@ public final class Constants {
      * {@link org.apache.logging.log4j.core.layout.ByteBufferDestination}s without creating intermediate temporary
      * Objects.
      * <p>
-     * {@code True} by default iff all loggers are asynchronous because system property
-     * {@code Log4jContextSelector} is set to {@code org.apache.logging.log4j.core.async.AsyncLoggerContextSelector}.
-     * Disable by setting system property "log4j2.enable.direct.encoders" to "false".
+     *     This constant is {@code true} by default, but can be disabled using the
+     *     {@code "log4j2.enableDirectEncoders"} system property.
+     * </p>
      *
      * @since 2.6
      */
diff --git a/log4j-perf-test/src/main/java/org/apache/logging/log4j/perf/jmh/ConcurrentAsyncLoggerToFileBenchmark.java b/log4j-perf-test/src/main/java/org/apache/logging/log4j/perf/jmh/ConcurrentAsyncLoggerToFileBenchmark.java
@@ -93,7 +93,7 @@ public static class BenchmarkState {
         @Setup
         public final void before() {
             new File("target/ConcurrentAsyncLoggerToFileBenchmark.log").delete();
-            System.setProperty("log4j2.is.webapp", "false");
+            System.setProperty("log4j2.enableThreadlocals", "true");
             asyncLoggerType.setProperties();
             queueFullPolicy.setProperties();
             logger = LogManager.getLogger(ConcurrentAsyncLoggerToFileBenchmark.class);
diff --git a/log4j-perf-test/src/main/java/org/apache/logging/log4j/perf/jmh/FileAppenderThrowableBenchmark.java b/log4j-perf-test/src/main/java/org/apache/logging/log4j/perf/jmh/FileAppenderThrowableBenchmark.java
@@ -57,8 +57,8 @@
 public class FileAppenderThrowableBenchmark {
     static {
         // log4j2
-        System.setProperty("log4j2.is.webapp", "false");
-        System.setProperty("log4j.configurationFile", "log4j2-perf-file-throwable.xml");
+        System.setProperty("log4j2.enableThreadlocals", "true");
+        System.setProperty("log4j2.configurationFile", "log4j2-perf-file-throwable.xml");
         // log4j 1.2
         System.setProperty("log4j.configuration", "log4j12-perf-file-throwable.xml");
         // logback
diff --git a/log4j-perf-test/src/main/java/org/apache/logging/log4j/perf/jmh/TextEncoderHelperBenchmark.java b/log4j-perf-test/src/main/java/org/apache/logging/log4j/perf/jmh/TextEncoderHelperBenchmark.java
@@ -45,9 +45,9 @@
 // ============================== HOW TO RUN THIS TEST: ====================================
 //
 // single thread:
-// java -Dfile.encoding=ISO-8859-1 -Dlog4j2.is.webapp=false -Dlog4j2.enable.threadlocals=true -jar
+// java -Dfile.encoding=ISO-8859-1 -Dlog4j2.enableThreadlocals=true -jar
 // log4j-perf/target/benchmarks.jar ".*StringBuilderEncoder.*" -f 1 -wi 5 -i 10
-// java -Dfile.encoding=UTF8 -Dlog4j2.is.webapp=false -Dlog4j2.enable.threadlocals=true -jar
+// java -Dfile.encoding=UTF8 -Dlog4j2.enableThreadlocals=true -jar
 // log4j-perf/target/benchmarks.jar ".*StringBuilderEncoder.*" -f 1 -wi 5 -i 10
 //
 // Usage help:
diff --git a/src/changelog/.2.x.x/change-is-webapp.xml b/src/changelog/.2.x.x/change-is-webapp.xml
@@ -0,0 +1,9 @@
+<?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="changed">
+  <description format="asciidoc">
+    Prioritize user-defined values of `log4j2.enableThreadLocals`, `log4j2.garbagefreeThreadContextMap` and `log4j2.shutdownHookEnabled` over the value of `log4j.isWebapp`.
+  </description>
+</entry>
diff --git a/src/site/antora/modules/ROOT/pages/manual/json-template-layout.adoc b/src/site/antora/modules/ROOT/pages/manual/json-template-layout.adoc
@@ -1085,7 +1085,7 @@ Resolves `logEvent.getMessage().getParameters()`.
 Regarding garbage footprint, `stringified` flag translates to
 `String.valueOf(value)`, hence mind not-`String`-typed values. Further,
 `logEvent.getMessage()` is expected to implement `ParameterVisitable` interface,
-which is the case if `log4j2.enableThreadLocals` property set to true.
+which is the case if `log4j2.enableThreadlocals` property set to true.
 ====
 
 ====== Examples

Original file line number	Diff line number	Diff line change
`@@ -326,12 +326,10 @@ public String getThreadContextMap() {`
`326`	`326`	`return threadContextMapClass;`
`327`	`327`	`}`
`328`	`328`	`// Default based on properties`
`329`		`- if (Constants.ENABLE_THREADLOCALS) {`
`330`		`- return props.getBooleanProperty(GC_FREE_THREAD_CONTEXT_PROPERTY)`
`331`		`- ? GC_FREE_THREAD_CONTEXT_PROPERTY`
`332`		`- : COPY_ON_WRITE_CONTEXT_MAP;`
	`329`	`+ if (props.getBooleanProperty(GC_FREE_THREAD_CONTEXT_PROPERTY, !Constants.IS_WEB_APP)) {`
	`330`	`+ return GC_FREE_THREAD_CONTEXT_PROPERTY;`
`333`	`331`	`}`
`334`		`- return WEB_APP_CONTEXT_MAP;`
	`332`	`+ return Constants.IS_WEB_APP ? WEB_APP_CONTEXT_MAP : COPY_ON_WRITE_CONTEXT_MAP;`
`335`	`333`	`}`
`336`	`334`
`337`	`335`	`/**`
Original file line number	Diff line number	Diff line change
`@@ -47,7 +47,7 @@ public class NestedLoggingFromThrowableMessageTest {`
`47`	`47`	`public static void beforeClass() {`
`48`	`48`	`file1.delete();`
`49`	`49`	`file2.delete();`
`50`		`- System.setProperty("log4j2.is.webapp", "false");`
	`50`	`+ System.setProperty("log4j2.enableThreadlocals", "true");`
`51`	`51`	`}`
`52`	`52`
`53`	`53`	`@Rule`