google-code-export / wro4j

Automatically exported from code.google.com/p/wro4j
1 stars 1 forks source link

AbstractCssImportPreProcessor leaks ThreadLocal variable during Tomcat shutdown #695

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Use Wro4j to render some CSS Imports
2. Shutdown Tomcat

What is the expected output? What do you see instead?

Expected no ThreadLocal's during shutdown. Instead, some are leaked. The 
following appears in the tomcat log file:

SEVERE: The web application [] created a ThreadLocal with key of type 
[ro.isdc.wro.model.resource.processor.impl.css.AbstractCssImportPreProcessor$1] 
(value [ro.isdc.wro.model.resource.processor.impl.css.AbstractCssImportPreP
rocessor$1@1a15838]) and a value of type [java.util.ArrayList] (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.

What version of the product are you using? On what operating system?

An inspection of the code in 1.6.2 shows there is no shutdown process to ensure 
the ThreadLocal's are cleaned up:

http://grepcode.com/file/repo1.maven.org/maven2/ro.isdc.wro4j/wro4j-core/1.6.2/r
o/isdc/wro/model/resource/processor/impl/css/AbstractCssImportPreProcessor.java#
AbstractCssImportPreProcessor.0processedImports

Original issue reported on code.google.com by pardsb...@gmail.com on 25 Mar 2013 at 6:17

GoogleCodeExporter commented 9 years ago
Though there is no change in regard to this, you should try the latest version 
(1.6.3).

Original comment by alex.obj...@gmail.com on 25 Mar 2013 at 6:38

GoogleCodeExporter commented 9 years ago
Thanks for the tip Alex! I'll try it out if I get a chance. Or 1.6.4. :)

Original comment by pardsb...@gmail.com on 25 Mar 2013 at 10:01

GoogleCodeExporter commented 9 years ago
I don't know if you would like me to create another issue for this or not, but 
the WroConfigurationMBean is also not removed from JMX during shutdown, so that 
causes an additional memory leak risk.

For reference see here:

http://stackoverflow.com/questions/6415831/jmx-how-to-prevent-classloader-memory
-leaks-in-a-servlet-container

Or here the "Avoiding memory leaks" section of the SLF4J JMX configurator 
documentation:

http://logback.qos.ch/manual/jmxConfig.html

Original comment by pardsb...@gmail.com on 26 Mar 2013 at 11:19

GoogleCodeExporter commented 9 years ago
@pardsbane  there is a fix available in branch issue695 (on github). Could you 
test it on your environment and let me know if the fix works as expected? 
Thanks in advance.

Original comment by alex.obj...@gmail.com on 28 Mar 2013 at 10:15

GoogleCodeExporter commented 9 years ago
@alex, I reviewed the code, it looks good. What is the Context.correlationId? 
Is that a ThreadLocal variable as well, or is it cleaned up correctly?

Is there a version with this change available as a snapshot in a Maven 
repository? I would be happy to test out the fix but it will take me some time 
to get to it if I need to build the code to get it into my build process.

Thanks for making the fix so quickly.

Original comment by pardsb...@gmail.com on 1 Apr 2013 at 7:02

GoogleCodeExporter commented 9 years ago
The correlationId is a unique id associated with a request cycle and is hold 
inside a ThreadLocal which cleaned-up in WroFilter after each processing.

There is no maven snapshot repo, however you can build the wro4j-core (only) 
module locally and test it in your environment. 

Let me know if you have more questions or need any help. I will merge soon the 
branch into stable 1.6.x branch and will mark it as fixed as soon as you can 
confirm that everything works as expected.

Original comment by alex.obj...@gmail.com on 1 Apr 2013 at 7:07

GoogleCodeExporter commented 9 years ago
Merged in stable branch 1.6.x. 
Marking issue as fixed.
Feel free to reopen if the problem can be reproduced with the latest build.

Original comment by alex.obj...@gmail.com on 1 Apr 2013 at 7:44

GoogleCodeExporter commented 9 years ago
Hi,

I compiled the issue695 branch and included it as system dependency, it will 
take a few deploy-cycles to verify if the fix is taken care of but I should 
have results soon. On a related note, the version of the Google Closure 
Compiler wro4j depends on is old and has a similar memroy leak, I solved this 
by forcing my project to use a newer version with:

        <dependency>
            <groupId>com.google.javascript</groupId>
            <artifactId>closure-compiler</artifactId>
            <version>v20130227</version>
        </dependency>

See this bug: https://code.google.com/p/closure-compiler/issues/detail?id=685

I think the version of closure that wro4j normally includes is slightly newer 
than the version the bug I mention above says the problem is fixed in, however 
the problem clearly still exists in the version wro4j depends on. I believe 
upgrading to the version I mention above (v20130227) solved the problem. I can 
create a separate bug for that if you want.

Original comment by pardsb...@gmail.com on 1 Apr 2013 at 8:08

GoogleCodeExporter commented 9 years ago
The googleClosure dependency was updated in 1.6.3 release.

Original comment by alex.obj...@gmail.com on 1 Apr 2013 at 8:10

GoogleCodeExporter commented 9 years ago
@alex, thats great, thanks.

I ran a quick test and in my most recent PermGen OOM dump I don't see any 
references to wro4j. That isn't a guarantee, but it is a good indication the 
problem is fixed. Thanks!

Original comment by j...@joshpollak.com on 1 Apr 2013 at 9:40

GoogleCodeExporter commented 9 years ago

Original comment by alex.obj...@gmail.com on 8 Jun 2013 at 9:43