Hi. I very much appreciate the update and the new release. Thank you. I'm glad that the memory leak patch helped and that you could see from your updated tests what I had seen regarding leaks.
*I hate to tell you this, but you'll need to make some fixes and release 1.10.x again.* *First*, you didn't include my Log4J2 ThreadContext patch for fixing https://ops4j1.jira.com/browse/PAXLOGGING-244 on the updated 1.10.x branch. Without that fix, my updated code that uses the ThreadContext instead of using so many loggers with different names does not work (because the ThreadContext gets cleared too soon). That PAXLOGGING-244 fix affects the *pax-logging-log4j2.jar*, not just pax-logging-api.jar. My ThreadContext patch was included in a previous post. As shown in the "README" attached to that post, I applied my ThreadContext patch first, and then applied my memory leak patch. *Second*, besides missing the PAXLOGGING-244 fix, *you have the following issues in what you did patch*. The patch seems to have been applied incorrectly: pax-logging-api/src/main/java/org/apache/commons/logging/LogFactory.java Line 262/264: jclLogger gets added to the m_loggers.get(name) list TWICE. (This is wrong--need to delete 264 [keep 262 so that it gets added inside the synchronized block].) pax-logging-api/src/main/java/org/apache/juli/logging/LogFactory.java Line 37: Import of java.util.Collections is no longer used, and can be removed. Line 159: Extra semicolon (no big deal, but might as well remove it while you fix the other issues). Line 209/211: juliLogger gets added to the m_loggers.get(name) list TWICE. (This is wrong--need to delete 211 [keep 209 so that it gets added inside the synchronized block].) pax-logging-api/src/main/java/org/apache/log4j/Logger.java Line 20: Import of java.util.Collections is no longer used, and can be removed. Line 147/149: logger gets added to the m_loggers.get(name) list TWICE. (This is wrong--need to delete 149 [keep 147 so that it gets added inside the synchronized block].) pax-logging-api/src/main/java/org/ops4j/pax/logging/avalon/AvalonLogFactory.java Line 20: Import of java.util.Collections is no longer used, and can be removed. Line 86-94/95-96: A local variable 'paxLogger' is created on 86-94, but then is not used. Line 95-96 needs to use the paxLogger instead of creating and using a new local variable 'logger'. (Line 95-96 ignores the possibility of needing the "fallback logger".) (*Looks like I missed this in my patch.* I don't have anything that uses Avalon Log, so I didn't test it. I changed Avalon Log to make all of the logging implementations work similarly with regards to the fallback logger and the real m_paxLogging.getLogger, clearing the m_loggers list in setBundleContext, and only storing a value in m_loggers at all if the m_paxLogging was null when a new instance of a logger was created). I think line 86-94 need to use newName, then line 95 needs to be deleted, and Line 96 needs to use the paxLogger variable.) Line 102/104: avalonLogger gets added to the m_loggers.get(newName) list TWICE. (This is wrong--need to delete 104 [keep it inside the synchronized block].) Thus, the getLogger method needs to be the following (to get the correct PaxLogger implementation based on the newName, including possibility of using a "fallback logger"). *This is different from my original patch.*: public static Logger getLogger( AvalonLogger parent, String name ) { String newName; if( parent == null ) { newName = name; } else { newName = parent.getName() + "." + name; } PaxLogger paxLogger; if( m_paxLogging == null ) { paxLogger = FallbackLogFactory.createFallbackLog( null, newName ); } else { paxLogger = m_paxLogging.getLogger( newName, AvalonLogger.AVALON_FQCN ); } AvalonLogger avalonLogger = new AvalonLogger( paxLogger ); 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); } } return avalonLogger; } pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java Line 49/52: paxLogging.open() is called twice. (This is wrong--need to delete 52 [keep 49 so that it gets updated inside the synchronized block].) pax-logging-api/src/main/java/org/ops4j/pax/logging/slf4j/Slf4jLoggerFactory.java Line 38: m_paxLogging was made "package protected" instead of "private". It still works, but not sure why the change, since it isn't used anywhere outside the class? It can be left "package protected" if you want, but it seemed odd (since it is not clear why the change was made). =============== Please fix the ThreadContext problem (patch provided earlier, based on changes on 1.11.x branch for PAXLOGGING-244) and address the above problems. If you want me to look at it before you actually make the 1.10.7 release, please let me know. Thanks again! Monica On Monday, May 4, 2020 at 4:40:55 AM UTC-4, Grzegorz Grzybek wrote: > > Hello again > > Without waiting, I've just released pax-logging 1.10.6 version - I hope > it'll solve all your (Monica Ron) problems ;) > > regards > Grzegorz Grzybek > -- -- ------------------ 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/731de33c-9f08-463c-8db5-c92c892fc42f%40googlegroups.com.
