Hello I've updated PAXLOGGING-244 issue and I'm just releasing pax-logging 1.10.7.
regards Grzegorz Grzybek wt., 5 maj 2020 o 21:13 Monica Ron <[email protected]> napisał(a): > 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]> 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] >>> >>> --- >>> 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 >>> <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 > <https://groups.google.com/d/msgid/ops4j/a80b948c-a403-44bf-bb34-9e373c0d2f3c%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/CAAdXmhr7x9rPazN%3DzfhdYDuek2JGvC%2BhYk2eQMrm-DUA%2BBjYRA%40mail.gmail.com.
