remkop commented on a change in pull request #208:
URL: https://github.com/apache/logging-log4j2/pull/208#discussion_r461938239
##########
File path:
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableParameterizedMessage.java
##########
@@ -355,4 +356,41 @@ public void clear() { // LOG4J2-1583
messagePattern = null;
throwable = null;
}
+
+ @Override
+ public MessageContentFormatter getMessageContentFormatter() {
+ return Formatter.INSTANCE;
+ }
+
+ private enum Formatter implements MessageContentFormatter {
+ INSTANCE;
+
+ private static final ThreadLocal<int[]> localIndices = new
ThreadLocal<int[]>() {
+ @Override
Review comment:
This pattern (subclassing ThreadLocal to provide an initial value) is
problematic for frameworks that use class loaders. If I remember correctly,
Tomcat reports this as a potential memory leak. The reason is that the
ThreadLocal map now has a reference to a Log4j class (the anonymous ThreadLocal
subclass) that is never removed, preventing the garbage collector from GC-ing a
web app that was stopped or restarted.
A better way is to simply declare the threadlocal as
```java
private static final ThreadLocal<int[]> localIndices = new
ThreadLocal<int[]>();
```
... and providing an accessor (getter) method where the initial value is
set. The code using this threadlocal must always use the accessor. This is also
not great, but prevents the memory leak.
##########
File path:
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableParameterizedMessage.java
##########
@@ -355,4 +356,41 @@ public void clear() { // LOG4J2-1583
messagePattern = null;
throwable = null;
}
+
+ @Override
+ public MessageContentFormatter getMessageContentFormatter() {
+ return Formatter.INSTANCE;
+ }
+
+ private enum Formatter implements MessageContentFormatter {
+ INSTANCE;
+
+ private static final ThreadLocal<int[]> localIndices = new
ThreadLocal<int[]>() {
+ @Override
Review comment:
This pattern (subclassing ThreadLocal to provide an initial value) is
problematic for frameworks that use class loaders. If I remember correctly,
Tomcat reports this as a potential memory leak. The reason is that the
ThreadLocal map now has a reference to a Log4j class (the anonymous ThreadLocal
subclass) that is never removed, preventing the garbage collector from GC-ing a
web app that was stopped or restarted.
A better way is to simply declare the threadlocal as
```java
private static final ThreadLocal<int[]> localIndices = new
ThreadLocal<int[]>();
```
... and providing an accessor (getter) method where the initial value is
set. The code using this threadlocal must always use the accessor. This is also
not great, but prevents the memory leak.
For the same reason, in Log4j we only store values of JDK classes in
ThreadLocals (we never store the value of a Log4j class in a ThreadLocal). In
this case the value is an `int[]`, so there is no problem, but just for
reference... :-)
##########
File path:
log4j-api/src/main/java/org/apache/logging/log4j/message/ReusableParameterizedMessage.java
##########
@@ -355,4 +356,41 @@ public void clear() { // LOG4J2-1583
messagePattern = null;
throwable = null;
}
+
+ @Override
+ public MessageContentFormatter getMessageContentFormatter() {
+ return Formatter.INSTANCE;
+ }
+
+ private enum Formatter implements MessageContentFormatter {
+ INSTANCE;
+
+ private static final ThreadLocal<int[]> localIndices = new
ThreadLocal<int[]>() {
+ @Override
Review comment:
This pattern (subclassing ThreadLocal to provide an initial value) is
problematic for frameworks that use class loaders. If I remember correctly,
Tomcat reports this as a potential memory leak. The reason is that the
ThreadLocal map now has a reference to a Log4j class (the anonymous ThreadLocal
subclass) that is never removed, preventing the garbage collector from GC-ing a
web app that was stopped or restarted.
A better way is to simply declare the threadlocal as
```java
private static final ThreadLocal<int[]> localIndices = new
ThreadLocal<int[]>();
```
... and providing an accessor (getter) method where the initial value is
set. The code using this threadlocal must always use the accessor. This is also
not great, but prevents the memory leak.
(Side note: For the same reason, in Log4j we only store values of JDK
classes in ThreadLocals (we never store the value of a Log4j class in a
ThreadLocal). In this case the value is an `int[]`, so there is no problem, but
just for reference... :-) )
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
