Hi. The new checkin looks much better, and matches what I have from my patch.
Please go ahead and release 1.10.7 with the updated code. You might want to mention in the Release Notes that PAXLOGGING-244 (the ThreadContext problem) is now fixed on the 1.10.x branch (and/or update the "Fix Versions" on PAXLOGGING-244 to include 1.10.7). It might help someone to have that info. Thanks for applying my patches and making an official release. Being able to use an official version is easier than our having to keep around our own patched version. The investigation of all this improved our code, too, as we now use the ThreadContext instead of making quite so many distinct loggers. Hopefully this will be the last issue for a while. ;) Monica On Tuesday, May 5, 2020 at 1:53:30 AM UTC-4, Grzegorz Grzybek wrote: > > Hello again ;) > > Thanks for detailed check! I confess - I wasn't able to do `git apply < > xxx.patch`, so I did it manually and: > - I forgot about changing the clear() to removal of "bundle.*" entries > - I didn't notice double put() > > This time I did 2nd review and here's the result: > https://github.com/ops4j/org.ops4j.pax.logging/commit/3250cd60675a48530000f74877fb49a68a2313ec > > - this time before new release ;) > > So this time, if you (please) check it, I'll release 1.10.7. > > regards > Grzegorz Grzybek > > pon., 4 maj 2020 o 16:34 Monica Ron <[email protected] <javascript:>> > napisał(a): > >> 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] <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/731de33c-9f08-463c-8db5-c92c892fc42f%40googlegroups.com >> >> <https://groups.google.com/d/msgid/ops4j/731de33c-9f08-463c-8db5-c92c892fc42f%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/a80b948c-a403-44bf-bb34-9e373c0d2f3c%40googlegroups.com.
