Bug 59805 - LocaleUtil causes memory leak
Summary: LocaleUtil causes memory leak
Status: RESOLVED FIXED
Alias: None
Product: POI
Classification: Unclassified
Component: POI Overall (show other bugs)
Version: 3.14-FINAL
Hardware: PC All
: P2 normal (vote)
Target Milestone: ---
Assignee: POI Developers List
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-07-06 03:18 UTC by apptaro
Modified: 2016-07-07 02:10 UTC (History)
0 users



Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description apptaro 2016-07-06 03:18:46 UTC
org.apache.poi.util.LocaleUtil (introduced in 3.13) causes memory leak.

The following error messages are logged when redeploying an application which uses POI 3.14:

06-Jul-2016 11:05:48.760 SEVERE [ContainerBackgroundProcessor[StandardEngine[Catalina]]] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [ROOT] created a ThreadLocal with key of type [org.apache.poi.util.LocaleUtil$2] (value [org.apache.poi.util.LocaleUtil$2@198b08c]) and a value of type [java.util.Locale] (value [ja_JP]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time totry and avoid a probable memory leak.

06-Jul-2016 11:05:48.769 SEVERE [ContainerBackgroundProcessor[StandardEngine[Catalina]]] org.apache.catalina.loader.WebappClassLoaderBase.checkThreadLocalMapForLeaks The web application [ROOT] created a ThreadLocal with key of type [org.apache.poi.util.LocaleUtil$1] (value [org.apache.poi.util.LocaleUtil$1@14dfba9]) and a value of type [sun.util.calendar.ZoneInfo] (value [sun.util.calendar.ZoneInfo[id="Asia/Tokyo",offset=32400000,dstSavings=0,useDaylight=false,transitions=10,lastRule=null]]) but failed to remove it when the web application was stopped. Threads are going to be renewed over time to try and avoid a probable memory leak.
Comment 1 Javen O'Neal 2016-07-06 04:16:11 UTC
Thanks for the report.

I think Catalina is reporting a false positive here.

Looking over the short LocaleUtil class [1], there are two ThreadLocal constants, but the thread local wrapping is necessary for the purpose of this class: to globally set the locale and timezone.

The constants are static final, so the memory consumption doesn't increase with time or usage.

The only change I would make to this class is making the class final.

I am closing this as invalid, unless someone provides a solution that I'm not seeing. There should be no need to destroy and recreate threads due to LocaleUtil.

[1] https://svn.apache.org/viewvc/poi/trunk/src/java/org/apache/poi/util/LocaleUtil.java?view=markup
Comment 2 Javen O'Neal 2016-07-06 04:53:35 UTC
(In reply to Javen O'Neal from comment #1)
> I think Catalina is reporting a false positive here.

I take that back. This may cause a PermGen memory leak since every thread holds onto a reference to the ThreadLocal subclass.ly.

Searching for ThreadLocal memory leak, I got this result:
https://stackoverflow.com/questions/17968803/threadlocal-memory-leak

Log4j had a similar problem on their ThreadLocalMap.
This memory leak was fixed here: https://svn.apache.org/viewvc/logging/log4j/trunk/src/main/java/org/apache/log4j/MDC.java?annotate=1231361&diff_format=f&pathrev=1231361#l177

The solution is to call ThreadLocal.remove somewhere...
Comment 3 apptaro 2016-07-06 05:37:49 UTC
Thank you for the quick response.

Maybe you could add LocaleUtil.resetUserTimeZone and LocaleUtil.resetUserLocale methods and call ThreadLocal.remove in them. Then it will be user's responsibility to avoid memory leaks.

There are other ways to address the issue, such as:
- call ThreadLocal.remove for each request using a servlet filter (Filter.doFilter)
- clear all references to the ThreadLocal when a servlet context is destroyed (ServletContextListener.contextDestroyed)
But it seems those solutions are too much.
Comment 4 Andreas Beeker 2016-07-06 08:27:06 UTC
Looking at the second response in stackoverflow - what would happen if we store Strings in the ThreadLocal and lookup the Timezone/Locale via those Strings?

As Locale and TimeZone are classes from the root classloader, are they connected to the web application classloader? So would it be any different than storing Strings?

What confuses me is the term "created a ThreadLocal with key of type [org.apache.poi.util.LocaleUtil$1]" ... so does this mean, even when storing Strings there's always a reference to the webapp classloader? (... or is this a week reference and doesn't count?)
Comment 5 Javen O'Neal 2016-07-06 09:25:43 UTC
Made LocaleUtil a static class in r1751601
Added unit tests for LocaleUtil in r1751620 and r1751629
Added LocaleUtil#resetUserLocale and #resetUserTimeZone in r1751641
Comment 6 apptaro 2016-07-07 00:44:24 UTC
Thank you for your quick response.

Looking at the code, and I think it's better to remove initialValue from ThreadLocal, and make getUserTimeZone/getUserLocal return default when nothing has been set, like this:

public static TimeZone getUserTimeZone() {
    TimeZone timeZone = userTimeZone.get();
    return (timeZone != null) ? timeZone : TimeZone.getDefault();
}

Otherwise, users need to call resetUserTimeZone/resetUserLocale to avoid memory leaks even when they have not called setUserTimeZone/setUserLocale.
Comment 7 Javen O'Neal 2016-07-07 00:56:26 UTC
Are there any issues with two subsequent calls to LocaleUtil.getUserTimeZone returning different values?

I guess this is consistent with two calls to TimeZone.getDefault()...
Comment 8 Javen O'Neal 2016-07-07 01:10:56 UTC
(In reply to apptaro from comment #6)
> public static TimeZone getUserTimeZone() {
>     TimeZone timeZone = userTimeZone.get();
>     return (timeZone != null) ? timeZone : TimeZone.getDefault();
> }

Applied in r1751739.
Comment 9 apptaro 2016-07-07 01:22:39 UTC
Looks good. Thank you very much.
Comment 10 Javen O'Neal 2016-07-07 02:01:37 UTC
Updated changelog in r1751740.
Comment 11 Javen O'Neal 2016-07-07 02:10:07 UTC
Updated unit test to reset LocaleUtil ThreadLocals in r1751741.