[
https://issues.apache.org/jira/browse/LOG4J2-1434?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15340646#comment-15340646
]
Luke Butters commented on LOG4J2-1434:
--------------------------------------
We could keep the existing {{getStringBuilder()}} and mark it as Deprecated.
Further we change its behaviour to do the pre-check length as you have already
done. So at worst we trim the length before handing out the StringBuilder.
Existing code wont break it just doesn't work as well.
We can then add the provider which is supposed to be closed, future code or
existing code can be slowly switched over to use this. Does this solve your
listed problems?
# The abstract class does not add a close method, existing code wont break.
# The thing that you have to go through to get the StringBuilder has to be
closed, we could make the more initiative with the method name.
# try with resources can be used so exceptional cases work.
# If using the provider we don't wast memory.
I guess it is odd to think we need to close this as we might not think of
StringBuilder as a resource. However if we look at the suggestion where we have
a pool of StringBuilders then StringBuilders are a resource in that pool, a
resource that we must get out of the pool do something with and finally return
to the pool. With that in mind it might seem like a better idea to have a
close() method.
> 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: log4j-dev-unsubscr...@logging.apache.org
For additional commands, e-mail: log4j-dev-h...@logging.apache.org
