Thanks. I decided to change my approach. I am not using the previous patch
anymore.
I patched the ThreadContext (based on PAXLOGGING-244), reworked my code to
use the ThreadContext instead of modifying the logger name, and also made
some changes to the pax-logging-api to fix some of the leak issues and to
address inconsistencies between the various logging implementations. For my
pax-logging-api changes, some of it follows what was done in the 1.11.x
branch for PAXLOGGING-307. I no longer swap the order of the WeakHashMap
parameters back to the original <Logger, String>. My patch keeps it with
the new <String, Logger> parameters, but does not store Logger
implementations in the map if the Pax Logging Manager is already created
(as mentioned earlier, SLF4J already had this check, but Log4J1 did not).
I attached my two patches and the instructions I wrote so that my teammates
could build the new jars. Feel free to use them or modify them as needed.
Monica
On Tuesday, April 21, 2020 at 2:48:57 AM UTC-4, Grzegorz Grzybek wrote:
>
> Hello
>
> Sorry for big delay... I still remember about this issue and I think I can
> do something about it soon. Just a little bit patience please ;)
>
> regards
> Grzegorz Grzybek
>
> śr., 18 mar 2020 o 22:47 Monica Ron <[email protected] <javascript:>>
> napisał(a):
>
>> I have a test that shows my groups usage. Should I just attach it as a
>> part of a post to this forum? It definitely behaves differently with the
>> 1.10.5 vs. with my patch, with regards to how many logger instances get
>> stored in m_loggers (especially if I use Log4J1 vs. Log4J2 as my API).
>>
>> I use the Log4J2 API in my real code, as I've stated before (but
>> third-party code we use uses SLF4J or JCL, and maybe others). I tried to
>> use the ThreadContext in my code (instead of the Markers that Ralph
>> mentioned), and ran into trouble, because I ran into the problem described
>> in https://ops4j1.jira.com/browse/PAXLOGGING-244 , for which the fix was
>> not applied to the 1.10.5 branch. Once I backported that fix to the 1.10.5
>> branch (making a new pax-logging-api and new pax-logging-log4j2, the
>> ThreadContext worked, and I could re-use logger names and still see which
>> "group" my log statements were from.
>>
>> Even if I change my code to use ThreadContext, the memory behavior of
>> 1.10.5 with regards to m_loggers is still a leak compared to the old 1.6.1
>> we were using, as I have been stating all along.
>>
>> I think the inconsistencies with regard to the following two items
>> (mentioned in my previous post) is also an issue:
>> 1. storing values in the m_loggers maps when m_paxLogging is non-null (
>> *only* SLF4J API in pax-logging-api 1.10.5 does **not** store it if
>> m_paxLogging is non-null), and
>> 2. getting a new logger even if a name is reused vs. re-using the old
>> logger (*only* Log4J2 API in pax-logging-api 1.10.5 reuses the logger if
>> the name was already used--other implementations just keep creating new
>> loggers for the same name, and store all of those loggers in m_loggers)
>>
>> Because of #1 and #2, if I was using Log4J1 API in pax-logging-api
>> 1.10.5, then even if I re-used the name for a non-static logger, the
>> m_loggers just keeps growing. At least with Log4J2, if I re-use the name
>> for a non-static logger, the m_loggers does not grow.
>>
>> Thanks again,
>> Monica
>>
>> --
>> --
>> ------------------
>> OPS4J - http://www.ops4j.org - [email protected] <javascript:>
>>
>> ---
>> You received this message because you are subscribed to the Google Groups
>> "OPS4J" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to [email protected] <javascript:>.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msgid/ops4j/e6783b83-bc0c-4d98-aae3-d28e72949c2b%40googlegroups.com
>>
>> <https://groups.google.com/d/msgid/ops4j/e6783b83-bc0c-4d98-aae3-d28e72949c2b%40googlegroups.com?utm_medium=email&utm_source=footer>
>> .
>>
>
--
--
------------------
OPS4J - http://www.ops4j.org - [email protected]
---
You received this message because you are subscribed to the Google Groups
"OPS4J" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to [email protected].
To view this discussion on the web visit
https://groups.google.com/d/msgid/ops4j/60e642dd-33d3-4249-beb4-87d2b65d7944%40googlegroups.com.
## README file for building Pax Logging patch to address memory leak issues.
##
## Requirements:
## 1. You must have 'git' installed.
## 2. You must have a connection to the internet to download the original Pax
Logging branch (using 'git') before you can apply the patch.
## 3. You must have Maven installed.
##
########
## The patch is based on code from the pax-logging-1.10.x branch of Pax Logging.
##
## To download the code and apply the patches, call the following from this
directory:
git init
git remote add origin https://github.com/ops4j/org.ops4j.pax.logging.git
git fetch origin
git merge aaa0909bdd4e1fb1179ad0beabb0520fd89af060
git apply PaxLoggingThreadContext.patch
git apply PaxLoggingMemoryLeak.patch
## Then do the following, substituting the appropriate local settings file:
mvn clean install -s localSettings.xml
## ----------------------------------
## DEPLOYMENT INSTRUCTIONS:
## The following two files need to be deployed in the Payara5
glassfish/modules/autostart directory.
## 1. pax-logging-api/target/pax-logging-api-1.10.6-SNAPSHOT.jar
## 2. pax-logging-log4j2/target/pax-logging-log4j2-1.10.6-SNAPSHOT.jar
##
## The other pax-logging-XXX projects do not need to be used.
##
## ----------------------------------
## PATCH DESCRIPTIONS:
## A. The PaxLoggingThreadContext.patch is a backport of fixes from Pax Logging
Jira issue PAXLOGGING-244. The fixes for that issue were not made in the
## official pax-logging-1.10.x branch, even though the bug is noted as
affecting Pax Logging 1.10.0.
##
## B. The PaxLoggingMemoryLeak.patch makes changes to how the individual
loggers for the various logging implementations are created. The loggers are now
## only stored in the m_loggers maps in the factories if the Pax Logging
Manager has not yet been created. Once the Pax Logging Manager is created (i.e.,
## when the org.ops4j.pax.logging.internal.Activator 'start' method is
called by the OSGi framework as part of pax-logging-api bundle activation), all
## existing loggers are updated, and then the m_loggers map in each factory
is cleared. Some of the changes in this patch follow some of what was done on
## the pax-logging-1.11.x branch for PAXLOGGING-307. Some changes for
PAXLOGGING-307 were included in the official Pax Logging 1.10.5 release, but
## inconsistencies in the different logging implementations (that is,
inconsistencies in the Pax Logging version of these implementations) meant
there were
## still memory leaks when a new logger was created.
##
diff --git a/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java b/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java
index 32467cc1..2288425e 100644
--- a/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java
+++ b/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java
@@ -34,7 +34,7 @@ public class Log4jv2LoggerContext implements LoggerContext {
private static final ConcurrentMap<String, Log4jv2Logger> loggers = new ConcurrentHashMap<String, Log4jv2Logger>();
- private static PaxLoggingManager paxLogging;
+ static PaxLoggingManager paxLogging;
public static void setBundleContext( BundleContext ctx )
{
diff --git a/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2ThreadContextMap.java b/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2ThreadContextMap.java
index cc911aa1..94f125d0 100644
--- a/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2ThreadContextMap.java
+++ b/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2ThreadContextMap.java
@@ -27,7 +27,7 @@ import org.apache.logging.log4j.spi.ThreadContextMap;
import org.ops4j.pax.logging.OSGIPaxLoggingManager;
import org.ops4j.pax.logging.PaxContext;
import org.ops4j.pax.logging.PaxLoggingManager;
-import org.osgi.framework.BundleContext;
+import org.ops4j.pax.logging.PaxLoggingService;
/**
* The actual ThreadContext Map. A new ThreadContext Map is created each time it is updated and the Map stored is always
@@ -37,17 +37,10 @@ import org.osgi.framework.BundleContext;
*/
public class Log4jv2ThreadContextMap implements ThreadContextMap {
- private static PaxContext m_context;
+ /** {@link PaxContext} used when {@link org.ops4j.pax.logging.PaxLoggingService} is not available */
private static PaxContext m_defaultContext = new PaxContext();
-
- private static PaxLoggingManager m_paxLogging;
-
- public static void setBundleContext( BundleContext ctx )
- {
- m_paxLogging = new OSGIPaxLoggingManager( ctx );
- // We need to instruct all loggers to ensure the SimplePaxLoggingManager is replaced.
- m_paxLogging.open();
- }
+ /** {@link PaxContext} obtained from {@link org.ops4j.pax.logging.PaxLoggingService} */
+ private static PaxContext m_context;
public static void dispose()
{
@@ -60,8 +53,12 @@ public class Log4jv2ThreadContextMap implements ThreadContextMap {
* or m_defaultContext if the logging manager is not set, or does not have its context available yet.
*/
private static PaxContext getContext() {
- if( m_context==null && m_paxLogging!=null ){
- m_context=(m_paxLogging.getPaxLoggingService()!=null)?m_paxLogging.getPaxLoggingService().getPaxContext():null;
+ PaxLoggingManager manager = Log4jv2LoggerContext.paxLogging;
+ if (manager != null) {
+ synchronized (Log4jv2ThreadContextMap.class) {
+ PaxLoggingService service = manager.getPaxLoggingService();
+ m_context = service != null ? service.getPaxContext() : null;
+ }
}
return m_context!=null?m_context:m_defaultContext;
}
diff --git a/pax-logging-log4j2/src/main/java/org/ops4j/pax/logging/log4j2/internal/PaxLoggerImpl.java b/pax-logging-log4j2/src/main/java/org/ops4j/pax/logging/log4j2/internal/PaxLoggerImpl.java
index 87a79ede..424013b6 100644
--- a/pax-logging-log4j2/src/main/java/org/ops4j/pax/logging/log4j2/internal/PaxLoggerImpl.java
+++ b/pax-logging-log4j2/src/main/java/org/ops4j/pax/logging/log4j2/internal/PaxLoggerImpl.java
@@ -127,7 +127,9 @@ public class PaxLoggerImpl
private void clearDelegateContext()
{
- ThreadContext.clearMap();
+ ThreadContext.remove("bundle.id");
+ ThreadContext.remove("bundle.name");
+ ThreadContext.remove("bundle.version");
}
private void doLog(final Level level, final int svcLevel, final String fqcn, final String message, final Throwable t ) {
diff --git a/pax-logging-api/src/main/java/org/apache/commons/logging/LogFactory.java b/pax-logging-api/src/main/java/org/apache/commons/logging/LogFactory.java
index 70df4d07..9d71c27e 100755
--- a/pax-logging-api/src/main/java/org/apache/commons/logging/LogFactory.java
+++ b/pax-logging-api/src/main/java/org/apache/commons/logging/LogFactory.java
@@ -113,6 +113,7 @@ public class LogFactory
}
}
m_paxLogging.open();
+ m_loggers.clear();
}
}
@@ -253,11 +254,13 @@ public class LogFactory
logger = m_paxLogging.getLogger(name, JclLogger.JCL_FQCN);
}
JclLogger jclLogger = new JclLogger(logger);
- synchronized (m_loggers) {
- if (!m_loggers.containsKey(name)) {
- m_loggers.put(name, new LinkedList<JclLogger>());
+ if (m_paxLogging == null) {
+ synchronized (m_loggers) {
+ if (!m_loggers.containsKey(name)) {
+ m_loggers.put(name, new LinkedList<JclLogger>());
+ }
+ m_loggers.get(name).add(jclLogger);
}
- m_loggers.get(name).add(jclLogger);
}
return jclLogger;
}
diff --git a/pax-logging-api/src/main/java/org/apache/juli/logging/LogFactory.java b/pax-logging-api/src/main/java/org/apache/juli/logging/LogFactory.java
index 51e35667..59863bb6 100644
--- a/pax-logging-api/src/main/java/org/apache/juli/logging/LogFactory.java
+++ b/pax-logging-api/src/main/java/org/apache/juli/logging/LogFactory.java
@@ -34,7 +34,6 @@
package org.apache.juli.logging;
-import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
@@ -157,7 +156,7 @@ public class LogFactory {
static
{
- m_loggers = Collections.synchronizedMap( new WeakHashMap<String, List<JuliLogger>>() );
+ m_loggers = new WeakHashMap<String, List<JuliLogger>>();
singleton = new LogFactory();
}
/**
@@ -201,11 +200,13 @@ public class LogFactory {
logger = m_paxLogging.getLogger( name, JuliLogger.JULI_FQCN );
}
JuliLogger juliLogger = new JuliLogger( logger );
- synchronized (m_loggers) {
- if (!m_loggers.containsKey(name)) {
- m_loggers.put(name, new LinkedList<JuliLogger>());
+ if (m_paxLogging == null) {
+ synchronized (m_loggers) {
+ if (!m_loggers.containsKey(name)) {
+ m_loggers.put(name, new LinkedList<JuliLogger>());
+ }
+ m_loggers.get(name).add(juliLogger);
}
- m_loggers.get(name).add(juliLogger);
}
return juliLogger;
}
@@ -396,6 +397,7 @@ public class LogFactory {
}
}
m_paxLogging.open();
+ m_loggers.clear();
}
}
diff --git a/pax-logging-api/src/main/java/org/apache/log4j/Logger.java b/pax-logging-api/src/main/java/org/apache/log4j/Logger.java
index a8e76f66..3c32be17 100644
--- a/pax-logging-api/src/main/java/org/apache/log4j/Logger.java
+++ b/pax-logging-api/src/main/java/org/apache/log4j/Logger.java
@@ -17,7 +17,6 @@
package org.apache.log4j;
-import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
@@ -65,7 +64,7 @@ public class Logger extends Category
static
{
- m_loggers = Collections.synchronizedMap( new WeakHashMap<String, List<Logger>>() );
+ m_loggers = new WeakHashMap<String, List<Logger>>();
}
public static void setBundleContext( BundleContext ctx )
@@ -83,6 +82,7 @@ public class Logger extends Category
}
}
m_paxLogging.open();
+ m_loggers.clear();
}
}
@@ -138,11 +138,13 @@ public class Logger extends Category
paxLogger = m_paxLogging.getLogger( name, LOG4J_FQCN );
}
Logger logger = new Logger( paxLogger );
- synchronized (m_loggers) {
- if (!m_loggers.containsKey(name)) {
- m_loggers.put(name, new LinkedList<Logger>());
+ if (m_paxLogging == null) {
+ synchronized (m_loggers) {
+ if (!m_loggers.containsKey(name)) {
+ m_loggers.put(name, new LinkedList<Logger>());
+ }
+ m_loggers.get(name).add(logger);
}
- m_loggers.get(name).add(logger);
}
return logger;
}
diff --git a/pax-logging-api/src/main/java/org/ops4j/pax/logging/avalon/AvalonLogFactory.java b/pax-logging-api/src/main/java/org/ops4j/pax/logging/avalon/AvalonLogFactory.java
index d26164bd..6effa471 100644
--- a/pax-logging-api/src/main/java/org/ops4j/pax/logging/avalon/AvalonLogFactory.java
+++ b/pax-logging-api/src/main/java/org/ops4j/pax/logging/avalon/AvalonLogFactory.java
@@ -17,7 +17,6 @@
*/
package org.ops4j.pax.logging.avalon;
-import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
@@ -28,6 +27,7 @@ import org.apache.avalon.framework.logger.Logger;
import org.ops4j.pax.logging.OSGIPaxLoggingManager;
import org.ops4j.pax.logging.PaxLogger;
import org.ops4j.pax.logging.PaxLoggingManager;
+import org.ops4j.pax.logging.internal.FallbackLogFactory;
import org.osgi.framework.BundleContext;
public class AvalonLogFactory
@@ -38,7 +38,7 @@ public class AvalonLogFactory
static
{
- m_loggers = Collections.synchronizedMap( new WeakHashMap<String, List<AvalonLogger>>() );
+ m_loggers = new WeakHashMap<String, List<AvalonLogger>>();
}
public static void setBundleContext( BundleContext context )
@@ -55,6 +55,7 @@ public class AvalonLogFactory
}
}
m_paxLogging.open();
+ m_loggers.clear();
}
}
@@ -81,13 +82,24 @@ public class AvalonLogFactory
{
newName = parent.getName() + "." + name;
}
+ PaxLogger paxLogger;
+ if( m_paxLogging == null )
+ {
+ paxLogger = FallbackLogFactory.createFallbackLog( null, name );
+ }
+ else
+ {
+ paxLogger = m_paxLogging.getLogger( name, AvalonLogger.AVALON_FQCN );
+ }
PaxLogger logger = m_paxLogging.getLogger( newName, AvalonLogger.AVALON_FQCN );
AvalonLogger avalonLogger = new AvalonLogger( logger );
- synchronized (m_loggers) {
- if (!m_loggers.containsKey(newName)) {
- m_loggers.put(newName, new LinkedList<AvalonLogger>());
+ if (m_paxLogging == null) {
+ synchronized (m_loggers) {
+ if (!m_loggers.containsKey(newName)) {
+ m_loggers.put(newName, new LinkedList<AvalonLogger>());
+ }
+ m_loggers.get(newName).add(avalonLogger);
}
- m_loggers.get(newName).add(avalonLogger);
}
return avalonLogger;
}
diff --git a/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2Logger.java b/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2Logger.java
index 467f1706..3e2742d7 100644
--- a/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2Logger.java
+++ b/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2Logger.java
@@ -31,7 +31,7 @@ import org.ops4j.pax.logging.internal.FallbackLogFactory;
*/
public class Log4jv2Logger extends AbstractLogger {
- private static final String LOG4J_FQCN = Logger.class.getName();
+ static final String LOG4J_FQCN = Logger.class.getName();
private volatile PaxLogger delegate;
diff --git a/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java b/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java
index 2288425e..dc3df163 100644
--- a/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java
+++ b/pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java
@@ -16,11 +16,12 @@
*/
package org.ops4j.pax.logging.log4jv2;
-import java.util.concurrent.ConcurrentHashMap;
-import java.util.concurrent.ConcurrentMap;
+import java.util.LinkedList;
+import java.util.List;
+import java.util.Map;
+import java.util.WeakHashMap;
import org.apache.logging.log4j.message.MessageFactory;
-import org.apache.logging.log4j.spi.AbstractLogger;
import org.apache.logging.log4j.spi.ExtendedLogger;
import org.apache.logging.log4j.spi.LoggerContext;
import org.ops4j.pax.logging.OSGIPaxLoggingManager;
@@ -32,17 +33,22 @@ import org.osgi.framework.BundleContext;
*/
public class Log4jv2LoggerContext implements LoggerContext {
- private static final ConcurrentMap<String, Log4jv2Logger> loggers = new ConcurrentHashMap<String, Log4jv2Logger>();
+ private static final Map<String, List<Log4jv2Logger>> m_loggers = new WeakHashMap<String, List<Log4jv2Logger>>();
static PaxLoggingManager paxLogging;
public static void setBundleContext( BundleContext ctx )
{
- paxLogging = new OSGIPaxLoggingManager( ctx );
- for (Log4jv2Logger logger : loggers.values()) {
- logger.setPaxLoggingManager(paxLogging);
+ synchronized (m_loggers) {
+ paxLogging = new OSGIPaxLoggingManager( ctx );
+ for (List<Log4jv2Logger> loggers : m_loggers.values()) {
+ for (Log4jv2Logger logger : loggers) {
+ logger.setPaxLoggingManager(paxLogging);
+ }
+ }
+ paxLogging.open();
+ m_loggers.clear();
}
- paxLogging.open();
}
public static void dispose() {
@@ -59,18 +65,25 @@ public class Log4jv2LoggerContext implements LoggerContext {
@Override
public ExtendedLogger getLogger(final String name, final MessageFactory messageFactory) {
- final ExtendedLogger extendedLogger = loggers.get(name);
- if (extendedLogger != null) {
- AbstractLogger.checkMessageFactory(extendedLogger, messageFactory);
- return extendedLogger;
+ Log4jv2Logger log4jv2Logger = new Log4jv2Logger(name, messageFactory, paxLogging);
+ if (paxLogging == null) {
+ // just add the logger which PaxLoggingManager need to be replaced.
+ synchronized (m_loggers) {
+ if (!m_loggers.containsKey(name)) {
+ m_loggers.put(name, new LinkedList<Log4jv2Logger>());
+ }
+ m_loggers.get(name).add(log4jv2Logger);
+ }
}
- loggers.putIfAbsent(name, new Log4jv2Logger(name, messageFactory, paxLogging));
- return loggers.get(name);
+ return log4jv2Logger;
}
@Override
public boolean hasLogger(final String name) {
- return loggers.containsKey(name);
+ // we don't know in pax-logging... Because org.apache.logging.log4j.spi.LoggerContext.hasLogger()
+ // API may actually be bridged to Logback or Log4j1 or no backend at all.
+ // also, same "name" may be related to different loggers (associated with different bundles)
+ throw new UnsupportedOperationException("Operation not supported in pax-logging");
}
@Override
diff --git a/pax-logging-api/src/main/java/org/ops4j/pax/logging/slf4j/Slf4jLoggerFactory.java b/pax-logging-api/src/main/java/org/ops4j/pax/logging/slf4j/Slf4jLoggerFactory.java
index d2b123f5..f97d120d 100644
--- a/pax-logging-api/src/main/java/org/ops4j/pax/logging/slf4j/Slf4jLoggerFactory.java
+++ b/pax-logging-api/src/main/java/org/ops4j/pax/logging/slf4j/Slf4jLoggerFactory.java
@@ -58,6 +58,7 @@ public class Slf4jLoggerFactory
}
}
m_paxLogging.open();
+ m_loggers.clear();
}
}
