[
https://issues.apache.org/jira/browse/GROOVY-4698?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15107852#comment-15107852
]
ASF GitHub Bot commented on GROOVY-4698:
----------------------------------------
GitHub user jwagenleitner opened a pull request:
https://github.com/apache/groovy/pull/245
GROOVY-4698 - Possible memory leak in Tomcat (GroovyScriptEngine
ThreadLocal)
Recommend reviewing each commit separately. The first commit I think is
fairly safe and addresses the issue reported. The others I'm a little less
sure about.
863247f - main fix is ensuring that `localTh.remove()` (changed from
`localTh.set(null)`) is always called even if an exception occurs. Did some
refactoring by pulling out the code used to update dependencies and the script
cache. Even though some changes have been made to the ThreadLocal's in this
class since that JIRA, the problem remains because `set(null)` will not be
called if `super.parseClass` throws. I think this is a fairly safe change.
dbfa026- If we always clean up the ThreadLocal by calling remove, I think
things can be simplified by removing the `WeakReference` holder and
synchronized `getLocalData` method.
c32722a- Since `super.parseClass` is synchronized and the other code in
that method just updates thread confined (ThreadLocal) data and
ConcurrentHashMap scriptCache, it seemed like that could be done outside of
synchronization. I don't know if there might be a potential problem with
ordering of the `scriptCache.put` calls. However, if it is supposed to be
synchronized, then I'm not sure a ThreadLocal is needed since that would mean
all access to the ThreadLocal is synchronized. In that case why not just have
a `private LocalData localData;` field and set it with a `new LocalData()` each
time `parseClass` is called?
**UNIT TEST NOTE**: I'm not sure how good the added test methods are. They
were helpful in showing the issue before the changes (commit 863247f).
However, they might be brittle since they rely on poking into
package-private/private members.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/jwagenleitner/groovy GROOVY-4698
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/245.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #245
----
commit 863247f25b4092d99c8e0fcc85d573f11647cd90
Author: John Wagenleitner <jwagenleit...@apache.org>
Date: 2016-01-20T01:55:25Z
GROOVY-4698 - Possible memory leak in Tomcat (GroovyScriptEngine
ThreadLocal)
GSE uses a ThreadLocal to temporarily cache data so it can be used in the
compilation phase. Once the script is compiled the cache data is no longer
necessary. ThreadLocal.set(null) was used which leaves the ThreadLocal as a
key in the Thread's ThreadLocalMap entries. In addition, if any exceptions
were thrown before that call it would leave a ThreadLocal entry with a
LocalData value in the thread's ThreadLocalMap tables. This change tries to
ensure that the ThreadLocal is removed when no longer needed.
commit dbfa0266e256910a27dfa6acbcc0fabd2fcc8270
Author: John Wagenleitner <jwagenleit...@apache.org>
Date: 2016-01-20T01:56:00Z
Rework ThreadLocal caching
commit c32722a990d743dfc30151f719599df2a8b729e1
Author: John Wagenleitner <jwagenleit...@apache.org>
Date: 2016-01-20T01:56:22Z
Remove synchronized block
The call to super.parseClass(GroovyCodeSource,boolean) is synchronized.
Updating local dependency cache and script cache is performed on thread
confined (ThreadLocal) and scriptCache ConcurrentHashMap.
----
> Possible memory leak in Tomcat
> ------------------------------
>
> Key: GROOVY-4698
> URL: https://issues.apache.org/jira/browse/GROOVY-4698
> Project: Groovy
> Issue Type: Bug
> Components: groovy-runtime
> Affects Versions: 1.7.8
> Environment: Groovy 1.7.8, Tomcat 6.0.28
> Reporter: François-Xavier Guillemette
>
> Using Groovy 1.7.8 Groovlets on Tomcat 6.0.28 yields the following in the log
> files:
> {code}
> The web application [...] created a ThreadLocal with key of type [null]
> (value [groovy.util.GroovyScriptEngine$2@68aed52c]) and a value of type
> [org.codehaus.groovy.tools.gse.StringSetMap] (value [{}]) but failed to
> remove it when the web application was stopped. This is very likely to create
> a memory leak.
> {code}
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
