Hi. I know it's been a while since this was last sent. The change in order
of the generic parameters of the WeakHashMap definitely caused a change in
memory behavior, and thus a memory leak in our application.
In some of our code, we have:
public abstract class AbstractBase {
protected Logger logger = LogManager.getLogger(getClass());
public void doSomething() {
logger.debug("Base method.");
}
}
And then we create instances of subclasses:
public class MyClassAAA extends AbstractBase {
public void someMethodA() {
logger.debug("Something.");
}
}
public class MyClassBBB extends AbstractBase {
public void someMethodB() {
logger.debug("Something.");
}
}
The inherited logger gets the name of the subclass, so you can see in the
log whether an instance of MyClassAAA did the logging or MyClassBBB did the
logging. This is a common usage of loggers from what I can tell. Not all
loggers are static. We also create some names dynamically (so we get
"MyClassAAA-1" and "MyClassAAA-2"), so that we can distinguish multiple
instances of it, so we can track data through a single thread (where we
might have 5 threads, each running a separate instance of the MyClassAAA
class), for example.
With the nature of our program, instances of some of these classes with
inherited loggers come and go frequently. Some are very short lived (an
object is created, some processing is performed on it, and the instance is
then dropped and garbage-collected). Some may live for days or for a few
weeks (to consume data from an ActiveMQ queue), but then are removed, never
to be used again. With the old Pax 1.6.1, because the WeakHashMap was
effectively "WeakHashMap<Logger, String>" (even though generic parameters
weren't specified, this was how it was used), the logger would be
garbage-collected when the instance of our class was garbage-collected.
With the current behavior of Pax Logging 1.10.5 with the generic parameters
switched to "WeakHashMap<String, Logger>" (and similar for other logging
types), each instance of a class with an inherited logger may get a new
instance of the logger, and those loggers are never garbage-collected. I
don't know the behavior of the real Log4J, Log4J2, SLF4J, JCL, etc. in
terms of memory management in our use case, but since our application has
always used Pax Logging, the new behavior is definitely a change, and
creates a memory leak.
I downloaded the 1.10.x branch (commit
398a8234c6c6ef831e64bd458263c637795a4087) and changed the following classes
to switch the WeakHashMap parameters back to the original <Logger, String>:
pax-logging-api/src/main/java/org/apache/commons/logging/LogFactory.java
pax-logging-api/src/main/java/org/apache/juli/logging/LogFactory.java
pax-logging-api/src/main/java/org/apache/log4j/Logger.java
pax-logging-api/src/main/java/org/ops4j/pax/logging/avalon/AvalonLogFactory.java
pax-logging-api/src/main/java/org/ops4j/pax/logging/slf4j/Slf4jLoggerFactory.java
I also changed this:
pax-logging-api/src/main/java/org/ops4j/pax/logging/log4jv2/Log4jv2LoggerContext.java
to use a WeakHashMap<Log4jv2Logger, String> instead of a
ConcurrentHashMap<String, Log4jv2Logger>. That one hadn't ever been
ConcurrentHashMap<Log4jv2Logger, String>, but changing it to a
WeakHashMap<Log4jv2Logger, String> improved the memory cleanup when our
instances were garbage-collected (since our upgraded code uses Log4jv2 as
an API instead of Log4J, but we are still using the PAX Logging API and
Service at runtime). I synchronized the loggers access in
Log4jv2LoggerContext, since I no longer had a ConcurrentHashMap.
For now, I have built a local copy of pax-logging-api with changes to the
above files, and it has helped our memory management compared to the 1.10.5
release.
Regards,
Monica
On Tuesday, January 7, 2020 at 4:55:57 AM UTC-5, Grzegorz Grzybek wrote:
>
> Hello
>
> sob., 4 sty 2020 o 02:58 Monica Ron <[email protected] <javascript:>>
> napisał(a):
>
>> The new code works for me. Once we deploy our wars, we don't generally
>> re-deploy until we have a new release, and we usually shutdown the
>> Glassfish/Payara domain to do that deployment. So, shutting down the JVM
>> obviously clears all memory.
>>
>> I'm not really worried (I think our code should be fine, based on how we
>> use it), but I do have one question:
>> Hasn't the garbage-collection behavior changed by changing the original
>> WeakHashMap<Logger,
>> String> (if generics had been added to the code as it is in 1.10.x [with
>> separate m_loggers for each logging API] without otherwise changing
>> underlying code) to WeakHashMap<String, List<Logger>>?
>>
>
> Actually both cases are a bit incorrect IMO... My new case (weak map of
> string → list<Logger>) only ensures that we don't loose loggers, but
> doesn't do any weak-functionality (because the logger strongly references
> key anyway). Previous case (weak map of logger → string) was better, but
> keys (loggers) were mostly used by strong references anyway throughout the
> code - because most libraries use "logger log = loggerFactory.getLogger()"
> idiom all the time - and mostly using static references, so the weak keys
> of old WeakHashMap<Logger, String> were not released anyway.
>
> In pax-logging 1.11.x+, the List<Logger> is definitely not weak, but
> activator of pax-logging-api explicitly clears this list when everything is
> done, so it's even better (IMO).
>
>
>>
>> For 1.11.x, the change made for PAXLOGGING-307 was:
>> public static final Map<String, PaxLoggingManagerAwareLogger> m_loggers =
>> new WeakHashMap<String, PaxLoggingManagerAwareLogger>();
>> became:
>> public static final List<PaxLoggingManagerAwareLogger> m_loggers = new
>> LinkedList<>();
>>
>> Based on the pre-generics code, the 1.11.x map should have had the logger
>> as the key:
>> public static final Map<PaxLoggingManagerAwareLogger, String> m_loggers =
>> new WeakHashMap<PaxLoggingManagerAwareLogger, String>();
>>
>>
>> For a WeakHashMap, if all of the other references to the key got
>> discarded elsewhere, the key-value pair automatically gets discarded from
>> the WeakHashMap, correct? So, if an instance of a class with a reference to
>> a PaxLoggingManagerAwareLogger (or Logger [of whatever logging API you
>> used] for 1.10.x) got garbage-collected, the logger would also have been
>> removed from m_loggers, correct? This automatic removal from m_loggers will
>> not occur with a List<PaxLoggingManagerAwareLogger> (for 1.11.x), or
>> with a WeakHashMap<String, List<Logger>> (for 1.10.x).
>>
>
> For 1.11.x it's removed explicitly here:
> https://github.com/ops4j/org.ops4j.pax.logging/blob/logging-1.11.4/pax-logging-api/src/main/java/org/ops4j/pax/logging/internal/Activator.java#L143-L155
>
> For 1.10.x, taking into account the fact that loggers are created once and
> mostly held statictly, it's not a big issue IMO.
>
>
>>
>> As I said, I don't think this will be a problem for us, but it may be
>> something to consider, if the original WeakHashMap was by intention to help
>> with garbage collection. By this time, no one may know what the original
>> intention is...
>>
>
> :)
>
> regards
> Grzegorz Grzybek
>
>
>> 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/d4a9fb64-9389-4d08-9b3e-8a735b57d6ef%40googlegroups.com
>>
>> <https://groups.google.com/d/msgid/ops4j/d4a9fb64-9389-4d08-9b3e-8a735b57d6ef%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/fdd62aeb-e3ae-4d33-879f-6c54d4ddce43%40googlegroups.com.
