[jboss-jira] [JBoss JIRA] Commented: (JASSIST-104) Proxy cache fails to guard against circular references which inhibits GC of classloaders

[Andrew Dinn (JIRA)]

    [ https://jira.jboss.org/jira/browse/JASSIST-104?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12518362#action_12518362 ] 

Andrew Dinn commented on JASSIST-104:
-------------------------------------

There is another problem here which my test has uncovered which makes me suspicious that there is another related issue here.

My test fails on the unpatched code and works with my patch to ProxyFactory. The patched version also fails if I provide Object as the super class which means that all the proxies are cached under the system class loader.

However, I also ran the test using Object as the super class and with ProxyFactory.useCache=false and it still crashed. So, it is not the proxy cache data which is causing the problem but the retention of the generated proxy classes themselves. Actually, this is obvious from the error. The cache data is not allocated out of perm gen space but creation of the classes does consume perm gen space.

I reran the test with a tiny heap (-Xms10M -Xmx10M) and it still worked for 5000 repetitions and failed for 10000. So, I am pretty certain the out of space error is to do with retaining handles on the proxy classes and nothing to do with retaining the cache data..That's not a lot of classes. Is there something really inefficient going on here? Or is it just very expensive to create 10000 classes at runtme?


> Proxy cache fails to guard against circular references which inhibits GC of classloaders
> ----------------------------------------------------------------------------------------
>
>                 Key: JASSIST-104
>                 URL: https://jira.jboss.org/jira/browse/JASSIST-104
>             Project: Javassist
>          Issue Type: Bug
>    Affects Versions: 3.11.0.GA
>            Reporter: Andrew Dinn
>            Assignee: Andrew Dinn
>         Attachments: jassist-104-patch.diffs
>
>
> The proxy cache employs a two level map to retain details of created proxies. The primary map  is a WeakHashMap which associates classloaders with all proxies created using that classloader. The WeakHashMap is intended to ensure that references to proxies are dropped when a classloader is unloaded.
> The secondary maps appearing as values in the primary map are just  plain hashmaps. These are used to lookup details of a proxy using a name  constructed from the names of its superclass and the interfaces it implements. The target of the secondary map is a CacheKey object which stores  the munged name, a weak reference to the proxy class, the method filter and the current default handler for instances of the proxy class. Any such proxy class will, of  course,  reference its classloader, the very same loader used as the WeakHashMap key for the secondary map in which the CacheKey instance resides. A WeakReference to the proxy class breask the reference cycle, ensuring that the WeakHashMap entry can be cleared when all other references to the classloader have been dropped.
> Unfortunately, this is not enough. Cachekey contains two other fields, handler and filter, which are instances of interfaces MethodHandler and MethodFilter. The values in these fields are supplied by a client application when the proxy is created. In normal use the proxy factory will locate the proxy class in the client application's classloader. In most cases this will be the same classloader that defines the classes used to provide the implementations of MethodHandler and MethodFilter used to populate the handler and filter fields. So, the secondary map must not  employ a direct reference to the objects in these fields because this may result in a cyclic reference to the classloader and thereby inhibit GC.
> There is actually an extra detail in play here which affects any proposed solution. The secondary map key is not actually the munged name. Instead the CacheKey instance is used as both key and value. This is because the equals method for CacheKey checks that the munged name, filter and handler are all equal. Incorporating the filter and handler into the equality check allows caching of proxies for  the same class using different filters and handlers.
> Unfortunately, this is unsafe. The handler field is meant to be updateable by the client after creation of the proxy i.e. after entering it in the cache. Changing this without removing and reinserting the entry risks the integrity of subsequent hash table lookup, replace and remove operations. For example, it is quite easy to end up with two 'equal' entries in the hashtable.
> A solution would be to employ a WeakHashMap for each secondary map, use the munged name as key and employ a class similar to the current CacheKey as the target of the map with the difference that it contains a list of proxy/handler/filter triples in place of the current 3 individual references. This simplifies the lookup (no special equals or hashcode needed) and also minimises the number of weak references (1 per super/interface set). This will also simplify duplicate detection and elimination when the handler  is updated.

-- 
This message is automatically generated by JIRA.
-
If you think it was sent incorrectly contact one of the administrators: https://jira.jboss.org/jira/secure/Administrators.jspa
-
For more information on JIRA, see: http://www.atlassian.com/software/jira