pombreda / google-gson

Automatically exported from code.google.com/p/google-gson
0 stars 0 forks source link

Memory Leak in web application #402

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Use on web application / Tomcat , view log
2.
3.

What is the expected output? What do you see instead?
Expected None / Warning

What version of the product are you using? On what operating system?
CentOS/ Apache Tomcat Version 7.0.12

Please provide any additional information below.

Jan 26, 2012 12:09:41 AM org.apache.catalina.loader.WebappClassLoader 
checkThreadLocalMapForLeaks
SEVERE: The web application [] created a ThreadLocal with key of type 
[com.google.gson.internal.bind.MiniGson$1] (value 
[com.google.gson.internal.bind.MiniGson$1@73055179]) and a value of type 
[java.util.HashMap] (value [{}]) 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.

Original issue reported on code.google.com by a...@aniljava.com on 26 Jan 2012 at 6:17

GoogleCodeExporter commented 9 years ago
Additional INFO:
Using Gson as :

String gson = new Gson().toJson(<java.util.Map instance>);

Map<String, String> responseMap = new Gson().fromJson(responseJSON, Map.class); 
//not related to first

Original comment by a...@aniljava.com on 26 Jan 2012 at 6:21

GoogleCodeExporter commented 9 years ago
This is just a stupid warning from Tomcat. Ignore the warning, and consider 
upgrading to a better servlet container.

Original comment by limpbizkit on 27 Jan 2012 at 4:31

GoogleCodeExporter commented 9 years ago
Does this mean the warning is not something to worry about?

Original comment by phil...@gmail.com on 6 Feb 2012 at 4:36

GoogleCodeExporter commented 9 years ago
Yeah, or you could report this as a bug to the Tomcat maintainers.

Original comment by limpbizkit on 6 Feb 2012 at 1:26

GoogleCodeExporter commented 9 years ago
I usually agree that these warning messages are dumb; I think there is some 
merit to to this one. Hear me out based on my reading of the code.

Start here:
http://docs.oracle.com/javase/6/docs/api/java/lang/ThreadLocal.html

ThreadLocal instances are typically private static fields in classes that wish 
to associate state with a thread (e.g., a user ID or Transaction ID).

Whenever I've used ThreadLocals they are static members of the class. I'm 
certainly willing to entertain other use cases. The use in Gson appears to be a 
performance cache.

I had a use case where I created a new Gson() on every request.  It ended up 
associating a ThreadLocal on the servlet container thread.  Until the container 
thread was GC'd these ThreadLocal's were continually associated with the 
container thread. I had several thousand of these warnings get printed out on 
shutdown. Not that big of a deal because these are pretty small; but it doesn't 
give me the warm and fuzzy that this is as right as it should be.

I think it would be more correct if the ThreadLocal inside Gson was static to 
the class.

My $.02

Original comment by marty.r...@gmail.com on 6 Feb 2012 at 8:32

GoogleCodeExporter commented 9 years ago
Gson's ThreadLocal is not a performance cache. Its for detecting reentrancy in 
TypeAdapters. Making it static would make it incorrect. 

The GC can collect ThreadLocal instances in the same way that it collects 
instances of any other type. If you create a Gson instance and release it, 
there is no leak.

The only potential for leak here is that we create up to one (empty) HashMap 
per live Gson instance per live thread. Most applications have a limited number 
of live Gson instances: they either create them and discard them (zero long 
lasting instances) or create one and reuse it (one long lasting instance.)

Original comment by jessewil...@google.com on 7 Feb 2012 at 3:15

GoogleCodeExporter commented 9 years ago
hi,

i have the same problem. and it is not as stupid warning from tomcat. a 
threadlocal value will be put in the threadlocalmap which is an instance 
variable in the Thread-class. so every object here must be removed in a 
serverside application. 

the problem arises, when you stop your application or redeploy it. there is a 
gson-instance in the threadlocalmap and because this is directly referenced 
from thread, this value lives as long as the thread in the server (well, 
forever). the situtation becomes really worse, when you profile the memory: the 
gson-object has my "webappclassloader" as classloader and this classloader is 
also not gc'd. this classloader (as every classloader) caches all classes which 
are loaded with it, so all my classes (and my webapp-library-classes) are also 
not GC'd. and every static variable in my classes (and my library classes) are 
also not GC'd. 

please profile a simple webapplication with your gson-threadlocal and undeploy 
the application in the container. you will see, that your 
applicationcode/-classes still is/are referenced by the webappclassloader which 
is referenced by a value which is referenced by a threadlocal-variable. so i 
think: tomcat is correct, this is probably a memory leak!

last but not least: yes, a threalocal can be GC'd like every other object in 
java. but the value in the threadlocal is directly referenced by the current 
thread, so in a serverside application the value in the threadlocal is never 
GC'd as long the thread exists.

please think about it.

Original comment by ulrich.s...@gmail.com on 3 Mar 2012 at 8:24

GoogleCodeExporter commented 9 years ago
@ulrich apologies for the delayed reply.

In your redeploy scenario what exactly is causing objects to leak? The 
ThreadLocal should be collected when nothing references it, and nothing 
references it. There might be a bug in the silly Tomcat diagnostic code.

Original comment by jessewil...@google.com on 22 Mar 2012 at 10:30

GoogleCodeExporter commented 9 years ago
Hi

java.lang.Thread references every threadlocal (real tricky). Tomcat only looks 
for values in the threadlocalmap (instancevariable from java.lang.Thread) and 
issues this warning. So i think it is not a silly warning: the value is not 
gc'd, it is referenced.

Original comment by ulrich.s...@gmail.com on 23 Mar 2012 at 3:28

GoogleCodeExporter commented 9 years ago
if tomcat is what's broken, maybe an interested party could provide a patch for 
tomcat? if a bug is filed with them, post about it here; i am not able to find 
one.

Original comment by jon.shu...@gmail.com on 20 Jul 2012 at 8:19

GoogleCodeExporter commented 9 years ago
tomcat isn't broken. it does correctly warn that a value is not GC'd. and 
tomcat is right, the value is not gc'd.

Original comment by ulrich.s...@gmail.com on 21 Jul 2012 at 4:07

GoogleCodeExporter commented 9 years ago
Any chance of this issue being reconsidered, or at least a workaround being 
investigated? In light of @ulrich's comments, this looks like a valid issue.

Original comment by rob.c...@gmail.com on 16 Oct 2012 at 11:40

GoogleCodeExporter commented 9 years ago
The project members here are wrong. Tomcat's warning is valid.

When you store an object o in a static ThreadLocal tl, it essentially gets 
stored in a map m in the thread t as an entry of the form WeakReference(tl) -> 
o. If t enternally lives in a pool, as is common in servlet containers, m will 
never be GCed so in order to free the entry, tl must be GCed or o must be 
removed from it. However, neither of these will happen if you have a static 
Gson that you never null out.

Now see the problem: t references m, which references o, which references o's 
class, which references the webapp's class loader, which references the class 
containing the Gson, which references tl, so this whole object graph won't be 
GCed. Thus the webapp's class loader and all of its classes are stuck in memory 
even if you undeploy it.

Original comment by j...@dataminr.com on 17 Oct 2012 at 2:10

GoogleCodeExporter commented 9 years ago
I think I might have worked around the problem with r1214.  The issue isn't 
that the ThreadLocal knows about any application classes; it's just that I'm 
*subclassing* thread local (as it is intended to be) and that's preventing 
unloading.

The correct fix for this problem is for Tomcat to not reuse threads once an 
applications has been unloaded. Threads are not stateless! Tomcat's shared 
thread pool is a broken optimization that's the root cause of this trouble. But 
I suspect it's a lot harder to fix Tomcat than it is to work around the problem 
in Gson, so we work around the problem in Gson. Sigh.

Original comment by limpbizkit on 23 Oct 2012 at 2:46

GoogleCodeExporter commented 9 years ago
Sorry, but i think you are wrong :-)

Here:
https://sites.google.com/site/gson/gson-user-guide#TOC-Gson-Performance-and-Scal
ability

you can read this:
=====
The Gson instance does not maintain any state while invoking Json operations. 
So, you are free to reuse the same object for multiple Json serialization and 
deserialization operations.
=====

So most people who are using gson in serverside apps use a single instance 
(static instance variable of type Gson). As long as this instance lives, the 
included ThreadLocal instancevariable is not GC'd. And now read comment 13 
again. 

Btw: this is not an issue in tomcat, it is also an issue in websphere (i had a 
comparable issue with WAS7) and i think in weblogic, ... an dany server which 
uses a threadpool. the difference between tomcat an this servers is that tomcat 
issues a warning, the others don't. but the memoryleak exists (believe me, we 
had a lot of work with a websphere-app and threadlocals which were not cleared; 
many many libraries put threadlocals but don't clear them).

And it cannot be fixed in any of these servers, it is a issue in JRE, because 
the values in the TL are referenced from the thread as long as the TL lives. 
The "Thread.threadlocalMap" is a map for "global variables in the thread 
scope". if you put values in this scope you should clear them out later. Tomcat 
issues a warning that there are values in this map which were not cleared. 

As Gson does not start the thread, Gson should not take assumptions about the 
lifecycle of a thread. You are taking the assumption, that a thread should be 
thrown away after it handled a request. That's wrong for most servers. 

I think your Gson class needs some sort of "clear" Method; or the people should 
be informed that a Gson-Instance does not clear it's state in the thread scope 
and it would be better to NOT use a single instance of type "Gson".

Original comment by ulrich.s...@gmail.com on 23 Oct 2012 at 4:53

GoogleCodeExporter commented 9 years ago
I don't know if a "clear" method would make sense given that you could just 
null out the Gson reference. Perhaps a cleaner resolution would be to stop 
using a ThreadLocal and instead pass state in the [de]serialization context.

Original comment by j...@dataminr.com on 23 Oct 2012 at 2:19

GoogleCodeExporter commented 9 years ago
And how would one null out this ThreadLocal reference created by Gson ?

Original comment by mich...@newsrx.com on 19 Dec 2012 at 9:01

GoogleCodeExporter commented 9 years ago
Null out the Gson reference; then it and its ThreadLocal get GCed.

Original comment by j...@dataminr.com on 19 Dec 2012 at 11:27

GoogleCodeExporter commented 9 years ago
I switched to creating the Gson instances on demand so that they would 
hopefully get GCed as soon as scope changed, and now I get hundreds of of:

created a ThreadLocal with key of type [com.google.gson.Gson$1] (value 
[com.google.gson.Gson$1@4ea55a96]) and a value of type [java.util.HashMap] 
(value [{}]) 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.

Can a way be added to EXPLICITLY permit request for thread destruction via a 
static class method call?

Original comment by mich...@newsrx.com on 31 Jan 2013 at 3:48

GoogleCodeExporter commented 9 years ago
@18

I null out the reference in contextDestroyed and still get the warnings.

Original comment by edychen on 4 Mar 2013 at 8:22

GoogleCodeExporter commented 9 years ago
I think you cannot null out the reference because 
java.lang.ThreadLocal.ThreadLocalMap still references the values. ThreadLocal's 
must be cleared explictly and in the same thread as the were filled with a 
value, because the "current thread" is the key in the map. So really don't try 
to clear the values in "contextDestroyed".

I think you have to live with it ... it is a bug in a library :-)

Original comment by ulrich.s...@gmail.com on 5 Mar 2013 at 5:46

GoogleCodeExporter commented 9 years ago
>> ThreadLocal's must be cleared explictly and in the same thread as the were 
filled with a value, because the "current thread" is the key in the map.

Please explain how i can do that. (Code-Sample?)
Or exists in the meantime an other workaround to face this problem?
My Provider don't allow my Webapp to be deployed because of this issue so this 
is a very critical problem for me.

Original comment by hannes.w...@gmail.com on 27 Mar 2013 at 10:16

GoogleCodeExporter commented 9 years ago
Since I just started using Apache ActiveMQ, which is bundled with TomEE
(tomcat7), I see more threadlocals in my tomee/tomcat7 log when shutting
down the container.

So, I did some research, and I think I found an ActiveMQ
issue/mail-discussion-thread which discussed this; i think the ActiveMQ
JIRA/issue was the better thread to follow, and I saw a recommendation
there that stated that this is a classloader issue; if the library that is
causing the issue is in your tomcat/lib folder, then move it to the
web-inf/... folder, and IIRC, problem can be solved that way.

I have not tried that, since TomEE bundles ActiveMQ library in tomee/lib
folder, I prefer not to de-bundle activeMQ from TomEE, and I really don't
have an issue with the threadlocals reported in the log when shutting down
tomee/tomcat7.

Original comment by smithh03...@gmail.com on 27 Mar 2013 at 11:34

GoogleCodeExporter commented 9 years ago
I don't think you can solve the issue by putting the library in the web-inf/lib 
folder (most people do this). The problem here is that the systemclassloader 
from java itself references the value which is not cleared because ThreadLocal 
(and ThreadLocalMap) are standard java classes and they "glue" the value to the 
current thread. if they reference a value which was loaded by a different 
classloader, this value (and the classes and everything hanging behind) will 
not be gc'ed. so a higher-order classloader references a 
application-classloader. no chance.

you have to live with it: if you use gson, you have this bug. and the team will 
not fix it because they think it is not a bug.

Original comment by ulrich.s...@gmail.com on 29 Mar 2013 at 1:06

GoogleCodeExporter commented 9 years ago
I believe this code solves the issue, at least in Java 7 and Gson 2.2.2. Just 
call it when your webapp shuts down. But I don't use Tomcat currently so I 
haven't tested it in action. (Of course, it isn't exactly the epitome of 
elegance and isn't at all portable...)

Original comment by j...@dataminr.com on 29 Mar 2013 at 2:54

Attachments:

GoogleCodeExporter commented 9 years ago
I tried the GSONThreadLocalImmolater and tested.

1. added the class, without 'main()', to my project (web app running on
tomcat/tomee)

2. added the following to @PreDestroy method of CDI @ApplicationScoped bean

    try {
        Integer threadLocalCount;
        GSONThreadLocalImmolater gsonImmolator = new
GSONThreadLocalImmolater();
        threadLocalCount = gsonImmolator.immolate();
        logger.info("gsonImmolator.immolate() completed: immolated " +
                    threadLocalCount + " GSON values in ThreadLocals");
    } catch (Exception e) {
        logger.info("caught exception raised by gsonImmolator.immolate()",
e);
    } finally {
        // do nothing
    }

3. Ran the web app, executed part of app that has gson dependency, exited
the app, and shutdown tomee/tomcat7. below is what was in the log

Mar 29, 2013 1:11:01 PM pf.ApplicationScopeBean destroy
INFO: gsonImmolator.immolate() completed: immolated 1 GSON values in
ThreadLocals
Mar 29, 2013 1:11:01 PM pf.ApplicationScopeBean destroy
INFO: END

4. Voila, that looks good, that it immolated '1' GSON value in
ThreadLocals, because when I usually stop tomee/tomcat7 on production
server, I see the following (which was possibly immolated on my development
server, just now)

Mar 28, 2013 9:27:00 PM org.apache.catalina.loader.WebappClassLoader
checkThreadLocalMapForLeaks
SEVERE: The web application [/mcmsweb] created a ThreadLocal with key of
type [com.google.gson.Gson$1] (value [com.google.gson.Gson$1@e9a08f7]) and
a value of type [java.util.HashMap] (value [{}]) 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.

5. Now, '1' threadlocal down and 'more' to go, since I still get the
following in my log after shutting down tomee/tomcat7. Thanks!

Mar 29, 2013 1:11:02 PM org.apache.catalina.loader.WebappClassLoader
clearReferencesThreads
SEVERE: The web application [/mcmsweb] appears to have started a thread
named [PoolIdleReleaseTimer] but has failed to stop it. This is very likely
to create a memory leak.
Mar 29, 2013 1:11:02 PM org.apache.catalina.loader.WebappClassLoader
clearReferencesThreads
SEVERE: The web application [/mcmsweb] appears to have started a thread
named [Default JMS Resource Adapter-worker-1] but has failed to stop it.
This is very likely to create a memory leak.
Mar 29, 2013 1:11:02 PM org.apache.catalina.loader.WebappClassLoader
clearReferencesThreads
SEVERE: The web application [/mcmsweb] appears to have started a thread
named [Default JMS Resource Adapter-worker-2] but has failed to stop it.
This is very likely to create a memory leak.
Mar 29, 2013 1:11:02 PM org.apache.catalina.loader.WebappClassLoader
clearReferencesThreads
SEVERE: The web application [/mcmsweb] appears to have started a thread
named [ActiveMQ VMTransport: vm://localhost#5-2] but has failed to stop it.
This is very likely to create a memory leak.
Mar 29, 2013 1:11:02 PM org.apache.catalina.loader.WebappClassLoader
clearReferencesThreads
SEVERE: The web application [/mcmsweb] appears to have started a thread
named [ActiveMQ VMTransport: vm://localhost#5-3] but has failed to stop it.
This is very likely to create a memory leak.
Mar 29, 2013 1:11:02 PM org.apache.catalina.loader.WebappClassLoader
clearReferencesThreads
SEVERE: The web application [/mcmsweb] appears to have started a thread
named [ActiveMQ VMTransport: vm://localhost#4-3] but has failed to stop it.
This is very likely to create a memory leak.
Mar 29, 2013 1:11:02 PM org.apache.catalina.loader.WebappClassLoader
checkThreadLocalMapForLeaks
SEVERE: The web application [/mcmsweb] created a ThreadLocal with key of
type [com.google.api.client.util.escape.Platform$1] (value
[com.google.api.client.util.escape.Platform$1@30b7785e]) and a value of
type [char[]] (value [[C@5d3f7c4e]) 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.

Original comment by smithh03...@gmail.com on 29 Mar 2013 at 5:58

GoogleCodeExporter commented 9 years ago
That code works great. I tweaked the code, removed the IF which checks if the 
class is 'Gson.class', and just let it run through all threadlocals != null, 
and now all of my threadlocals are cleared!

Kindly, disregard the last part of my previous response; after doing some 
testing and reviewing the threads in Java Visual VM, I see that the ActiveMQ 
threads are cleared as well, most likely because the following occurs last 
(after catalina's WebappClassLoader checkThreadLocalMapForLeaks) :

Mar 29, 2013 6:05:43 PM 
org.apache.openejb.resource.activemq.ActiveMQResourceAdapter stop
INFO: Stopping ActiveMQ
Mar 29, 2013 6:05:43 PM 
org.apache.openejb.resource.activemq.ActiveMQResourceAdapter stopImpl
INFO: Stopped ActiveMQ broker

Original comment by smithh03...@gmail.com on 29 Mar 2013 at 10:20

GoogleCodeExporter commented 9 years ago
I don't know if it's wise to clear all ThreadLocals because you could be 
clearing state necessary for the standard library or Tomcat or something else. 
Even if the appears to work, it might cause things to fail in subtle ways, and 
if you upgrade Tomcat or Java, it might cause different things to fail. The 
code was hacky enough in its original form.

Original comment by j...@dataminr.com on 29 Mar 2013 at 10:32

GoogleCodeExporter commented 9 years ago
Understood, thanks.

Original comment by smithh03...@gmail.com on 29 Mar 2013 at 10:35

GoogleCodeExporter commented 9 years ago
[deleted comment]
GoogleCodeExporter commented 9 years ago
It is worth noting that this issue seems to have been addressed in revision 
1223 of the code: http://code.google.com/p/google-gson/source/detail?r=1223

This change was part of release 2.2.3 of the Gson library.

Basic tests (deploy webapp, perform serialization, undeploy webapp) show that 
Tomcat reports warnings of the type as discussed in this issue for version 
2.2.2 of Gson, but does not report this type of warnings when using versions 
2.2.3 or 2.2.4.

Original comment by Guus.der...@gmail.com on 24 Jul 2013 at 1:26

GoogleCodeExporter commented 9 years ago
Since this issue has been resolved can we have it marked as resolved? As it 
currently stands you have to read all the way to the bottom to find out that 
it's been fixed and the current status of "wontfix" makes it seem like wasted 
effort.

I'm also just going to mention that I've had this issue while using Netty and 
Gson 2.2.2 instead of Tomcat just so that anybody else in a similar situation 
can more easily find the solution to this issue.

Original comment by sam.peng...@gmail.com on 24 Feb 2014 at 5:56

GoogleCodeExporter commented 9 years ago
Echoing Sam. Please mark as issue as resolved. I just wasted 15 minutes only to 
find out it was fixed it with 2.2.3.

Original comment by thomashu...@gmail.com on 8 Apr 2014 at 9:39