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/CAAdXmhq5zQdGJ%3DbfT_nGxZpqh1PY7g5H8ixDOL046%3DtYi4tCdA%40mail.gmail.com.
