[jira] [Commented] (GROOVY-4698) Possible memory leak in Tomcat

["ASF GitHub Bot (JIRA)" <ji...@apache.org>]

    [ 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 <jw...@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 <jw...@apache.org>
Date:   2016-01-20T01:56:00Z

    Rework ThreadLocal caching

commit c32722a990d743dfc30151f719599df2a8b729e1
Author: John Wagenleitner <jw...@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)