[
https://issues.apache.org/jira/browse/LOG4J2-1434?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15339564#comment-15339564
]
Remko Popma commented on LOG4J2-1434:
-------------------------------------
I am not a fan of introducing a new Provider to obtain the StringBuilder. All
current built-in Layouts (and any unknown number of user-defined Layouts) that
extend AbstractStringLayout now get their StringBuilder from the layout. If we
change this so that the StringBuilder can only be obtained through this new
Provider we would break custom Layouts.
The options I see are:
# Let AbstractStringLayout implement Closeable. Advantage: try-with-resources
ensures that StringBuilder is cleaned up after use. Drawback #1: custom Layouts
that have a custom {{close()}} method would break. Drawback #2: client code
needs to be aware of and follow this idiom. Client code looks like this:
{code}
// AbstractStringLayout subclass:
try (this) {
format(event, getStringBuilder());
}
{code}
# AbstractStringLayout provides a Closeable object whose close() method calls
trimToMaxSize(StringBuilder). Advantage: try-with-resources ensures that
StringBuilder is cleaned up after use. Drawback #1: arguably unintuitive
(although this is debatable). Drawback #2: client code needs to be aware of and
follow this idiom. Client code looks like this:
{code}
// AbstractStringLayout subclass:
try (STRINGBUILDER_CLEANER) {
format(event, getStringBuilder());
}
{code}
# Manual cleanup without try with resources. Drawback #1: client code needs to
be aware of and follow this idiom. Drawback #2: without try-finally exceptions
can result in cleanup being skipped. Client code looks like this:
{code}
// AbstractStringLayout subclass:
private void format(LogEvent event, StringBuilder stringBuilder) {
.... // format the event
trimToMaxSize(stringBuilder); // clean up when done
}
{code}
# Auto-cleanup before handing out the StringBuilder. Advantage: No changes
required for client code. Drawback: memory not cleaned up if no message follows
a large message.
In my opinion none of the above solutions is perfect and they all have
different trade-offs. I've currently implemented option #4 which I think gives
the most bang for the buck. We can do more in addition, like a #2 for example,
and update the call sites to use the StringBuilder in a try-with-resources
block, but the vast majority of problems is addressed by what is currently in
master.
> StringBuffer in ThreadLocal can cause excessive memory usage after large log
> messages
> -------------------------------------------------------------------------------------
>
> Key: LOG4J2-1434
> URL: https://issues.apache.org/jira/browse/LOG4J2-1434
> Project: Log4j 2
> Issue Type: Bug
> Affects Versions: 2.6.1
> Reporter: Luke Butters
> Assignee: Remko Popma
> Fix For: 2.6.2
>
>
> In an effort to speed up logging ThreadLocals have been introduced see
> LOG4J2-1125 however this does causes memory issues.
> The problem of the ThreadLocal occurs when threads are re-used which is an
> absolutely valid way of using java. For example an executor service can
> re-use threads as well as Jetty.
> Below I demonstrate a contrived example of the memory leak:
> {code}
> int stringSize = 1024*1024*10; //~10MB maybe 20MB for UTF-16
> StringBuilder sb = new StringBuilder(stringSize);
> for(int i = 0; i < stringSize; i++) {
> sb.append('a' + i % 5);
> }
>
> String largeString = sb.toString();
>
> sb = null; //Let it be GC'ed
> ExecutorService es = Executors.newFixedThreadPool(100);
> final CountDownLatch countDownLatch = new CountDownLatch(100);
> for(int i = 0; i < 100; i++) {
> es.execute(()-> {
> //Log the big string to demonstrate the issue.
> log.fatal(largeString);
>
> //Ensure we use all 100 of our threads by not releasing this
> thread yet.
> countDownLatch.countDown();
> });
>
> //We sleep for 2s so we more easily watch memory growth
> Thread.sleep(2000);
> }
> {code}
> I recommend that log4j2 immediately remove the ThreadLocal as a small gain in
> performance does not outweigh the problems associated with memory leaks.
> Finally other options for caching the StringBuilder with a ThreadLocal could
> be considered for example we might re-use StringBuilders that are no larger
> than 3k while removing the ones which are larger than 3k.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
