vy commented on code in PR #2517:
URL: https://github.com/apache/logging-log4j2/pull/2517#discussion_r1579538084
##########
log4j-api/src/main/java/org/apache/logging/log4j/spi/Provider.java:
##########
Review Comment:
Can you explain how the thread-local toggling property is taken into account
in the new implementation?
##########
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()
Review Comment:
1. I think we should link to docs, not to a ticket
2. Narrowing the scope to only pooling is not a good idea. Pooling is
already configurable in some places. This flag should be the TLA kill switch,
regardless of where it is used. Is there a particular reason you took this
route?
##########
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>
Review Comment:
```suggestion
* <li>It disables the usage of {@link ThreadLocal}s for object
pooling (unless the user has explicitly provided a {@link #ENABLE_THREADLOCALS}
property)</li>
```
##########
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.
Review Comment:
I'd be in favor of using `enableThreadLocals` (just like many other
camelcase property names we have) and consistently using it everywhere in the
code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
