Never mind… I see you already did yesterday. I’m a bit behind. :) Thanks! Dan
> On Jan 15, 2019, at 6:30 PM, Daniel Kulp <dk...@apache.org> wrote: > > Freeman, > > This looks much better. Can you get this merged in? Thanks! > > Dan > > >> On Dec 24, 2018, at 4:50 AM, Freeman Fang <freeman.f...@gmail.com >> <mailto:freeman.f...@gmail.com>> wrote: >> >> Hi Dan, >> >> I just revised to use ClassValue as ProxyClassLoaderCache backend, and I >> pushed the changes to ProxyClassLoaderCache branch, would you please take a >> look? >> >> Merry Christmas! >> －－－－－－－－－－－－－ >> Freeman(Yue) Fang >> >> Red Hat, Inc. >> >> >> >> >> >>> On Dec 21, 2018, at 11:43 PM, Daniel Kulp <dk...@apache.org >>> <mailto:dk...@apache.org>> wrote: >>> >>> This commit has a bunch of problems. >>> >>> At no point is anything removed from these caches until the cache fills. >>> Thus, would definitely be considered a memory leak of some sort. >>> >>> Also, because the class loader is held onto strongly, the classes are >>> locked and not able to be garbage collected. This would prevent apps from >>> being unloaded, result in locked jar/wars/ears, etc… >>> >>> This really needs to be reverted before 3.3. >>> >>> My suggestion, I think, would be if you can determine which interface in >>> the array is the user one (and not CXF/JDK), use a ClassValue<ClassLoader> >>> to compute and hold this. >>> >>> >>> >>> Dan >>> >>> >>> >>>> On Dec 21, 2018, at 12:32 AM, ff...@apache.org <mailto:ff...@apache.org> >>>> wrote: >>>> >>>> This is an automated email from the ASF dual-hosted git repository. >>>> >>>> ffang pushed a commit to branch master >>>> in repository https://gitbox.apache.org/repos/asf/cxf.git >>>> <https://gitbox.apache.org/repos/asf/cxf.git> >>>> >>>> >>>> The following commit(s) were added to refs/heads/master by this push: >>>> new af33b2a [CXF-7934]Add cache for ProxyClassLoader >>>> af33b2a is described below >>>> >>>> commit af33b2a28c39504388ea370f6849a06d551b4dd1 >>>> Author: Freeman Fang <freeman.f...@gmail.com >>>> <mailto:freeman.f...@gmail.com>> >>>> AuthorDate: Fri Dec 21 13:32:19 2018 +0800 >>>> >>>> [CXF-7934]Add cache for ProxyClassLoader >>>> --- >>>> .../org/apache/cxf/common/util/ProxyHelper.java | 53 >>>> +++++++++++++++++++++- >>>> .../org/apache/cxf/jaxrs/utils/InjectionUtils.java | 50 >>>> +++++++++++++++++--- >>>> 2 files changed, 95 insertions(+), 8 deletions(-) >>>> >>>> diff --git >>>> a/core/src/main/java/org/apache/cxf/common/util/ProxyHelper.java >>>> b/core/src/main/java/org/apache/cxf/common/util/ProxyHelper.java >>>> index 7b185aa..ad83ad9 100644 >>>> --- a/core/src/main/java/org/apache/cxf/common/util/ProxyHelper.java >>>> +++ b/core/src/main/java/org/apache/cxf/common/util/ProxyHelper.java >>>> @@ -24,6 +24,13 @@ import java.lang.reflect.Method; >>>> import java.lang.reflect.Proxy; >>>> import java.security.AccessController; >>>> import java.security.PrivilegedAction; >>>> +import java.util.Collections; >>>> +import java.util.HashMap; >>>> +import java.util.Map; >>>> +import java.util.logging.Level; >>>> +import java.util.logging.Logger; >>>> + >>>> +import org.apache.cxf.common.logging.LogUtils; >>>> >>>> /** >>>> * >>>> @@ -39,8 +46,15 @@ public class ProxyHelper { >>>> } >>>> HELPER = theHelper; >>>> } >>>> - >>>> - >>>> + >>>> + private static final Logger LOG = >>>> LogUtils.getL7dLogger(ProxyHelper.class); >>>> + >>>> + protected Map<String, ClassLoader> proxyClassLoaderCache = >>>> + Collections.synchronizedMap(new HashMap<String, ClassLoader>()); >>>> + protected int cacheSize = >>>> + >>>> Integer.parseInt(System.getProperty("org.apache.cxf.proxy.classloader.size", >>>> "3000")); >>>> + >>>> + >>>> protected ProxyHelper() { >>>> } >>>> >>>> @@ -59,9 +73,26 @@ public class ProxyHelper { >>>> */ >>>> private ClassLoader getClassLoaderForInterfaces(final ClassLoader >>>> loader, final Class<?>[] interfaces) { >>>> if (canSeeAllInterfaces(loader, interfaces)) { >>>> + LOG.log(Level.FINE, "current classloader " + loader + " can >>>> see all interface"); >>>> return loader; >>>> } >>>> + String sortedNameFromInterfaceArray = >>>> getSortedNameFromInterfaceArray(interfaces); >>>> + ClassLoader cachedLoader = >>>> proxyClassLoaderCache.get(sortedNameFromInterfaceArray); >>>> + if (cachedLoader != null) { >>>> + if (canSeeAllInterfaces(cachedLoader, interfaces)) { >>>> + //found cached loader >>>> + LOG.log(Level.FINE, "find required loader from >>>> ProxyClassLoader cache with key" >>>> + + sortedNameFromInterfaceArray); >>>> + return cachedLoader; >>>> + } else { >>>> + //found cached loader somehow can't see all interfaces >>>> + LOG.log(Level.FINE, "find a loader from ProxyClassLoader >>>> cache with key " >>>> + + sortedNameFromInterfaceArray >>>> + + " but can't see all interfaces"); >>>> + } >>>> + } >>>> ProxyClassLoader combined; >>>> + LOG.log(Level.FINE, "can't find required ProxyClassLoader from >>>> cache, create a new one with parent " + loader); >>>> final SecurityManager sm = System.getSecurityManager(); >>>> if (sm == null) { >>>> combined = new ProxyClassLoader(loader, interfaces); >>>> @@ -75,9 +106,26 @@ public class ProxyHelper { >>>> } >>>> for (Class<?> currentInterface : interfaces) { >>>> combined.addLoader(getClassLoader(currentInterface)); >>>> + LOG.log(Level.FINE, "interface for new created >>>> ProxyClassLoader is " >>>> + + currentInterface.getName()); >>>> + LOG.log(Level.FINE, "interface's classloader for new created >>>> ProxyClassLoader is " >>>> + + currentInterface.getClassLoader()); >>>> } >>>> + if (proxyClassLoaderCache.size() >= cacheSize) { >>>> + LOG.log(Level.FINE, "proxyClassLoaderCache is full, need >>>> clear it"); >>>> + proxyClassLoaderCache.clear(); >>>> + } >>>> + proxyClassLoaderCache.put(sortedNameFromInterfaceArray, combined); >>>> return combined; >>>> } >>>> + >>>> + private String getSortedNameFromInterfaceArray(Class<?>[] interfaces) >>>> { >>>> + SortedArraySet<String> arraySet = new SortedArraySet<String>(); >>>> + for (Class<?> currentInterface : interfaces) { >>>> + arraySet.add(currentInterface.getName() + >>>> currentInterface.getClassLoader()); >>>> + } >>>> + return arraySet.toString(); >>>> + } >>>> >>>> private static ClassLoader getClassLoader(final Class<?> clazz) { >>>> final SecurityManager sm = System.getSecurityManager(); >>>> @@ -123,4 +171,5 @@ public class ProxyHelper { >>>> public static Object getProxy(ClassLoader loader, Class<?>[] >>>> interfaces, InvocationHandler handler) { >>>> return HELPER.getProxyInternal(loader, interfaces, handler); >>>> } >>>> + >>>> } >>>> diff --git >>>> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java >>>> >>>> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java >>>> index ce4d239..e2c80e1 100644 >>>> --- >>>> a/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java >>>> +++ >>>> b/rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/utils/InjectionUtils.java >>>> @@ -137,7 +137,13 @@ public final class InjectionUtils { >>>> private static final String ENUM_CONVERSION_CASE_SENSITIVE = >>>> "enum.conversion.case.sensitive"; >>>> >>>> private static final String IGNORE_MATRIX_PARAMETERS = >>>> "ignore.matrix.parameters"; >>>> - >>>> + >>>> + private static Map<String, ProxyClassLoader> proxyClassLoaderCache = >>>> + Collections.synchronizedMap(new HashMap<String, >>>> ProxyClassLoader>()); >>>> + >>>> + private static int cacheSize = >>>> + >>>> Integer.parseInt(System.getProperty("org.apache.cxf.proxy.classloader.size", >>>> "3000")); >>>> + >>>> private InjectionUtils() { >>>> >>>> } >>>> @@ -1076,9 +1082,25 @@ public final class InjectionUtils { >>>> proxy = createThreadLocalServletApiContext(type.getName()); >>>> } >>>> if (proxy == null) { >>>> - ProxyClassLoader loader = new >>>> ProxyClassLoader(Proxy.class.getClassLoader()); >>>> - loader.addLoader(type.getClassLoader()); >>>> - loader.addLoader(ThreadLocalProxy.class.getClassLoader()); >>>> + ProxyClassLoader loader >>>> + = proxyClassLoaderCache.get(type.getName() + >>>> type.getClassLoader()); >>>> + if (loader == null >>>> + || !canSeeAllClasses(loader, new Class<?>[]{Proxy.class, >>>> type, ThreadLocalProxy.class})) { >>>> + // to avoid creating too much ProxyClassLoader to save >>>> Metaspace usage >>>> + LOG.log(Level.FINE, "can't find required ProxyClassLoader >>>> for type " + type.getName()); >>>> + LOG.log(Level.FINE, "create a new one with parent " + >>>> Proxy.class.getClassLoader()); >>>> + loader = new >>>> ProxyClassLoader(Proxy.class.getClassLoader()); >>>> + loader.addLoader(type.getClassLoader()); >>>> + LOG.log(Level.FINE, "type classloader is " + >>>> type.getClassLoader()); >>>> + loader.addLoader(ThreadLocalProxy.class.getClassLoader()); >>>> + LOG.log(Level.FINE, "ThreadLocalProxy classloader is " >>>> + + >>>> ThreadLocalProxy.class.getClassLoader().getClass().getName()); >>>> + if (proxyClassLoaderCache.size() >= cacheSize) { >>>> + LOG.log(Level.FINE, "proxyClassLoaderCache is full, >>>> need clear it"); >>>> + proxyClassLoaderCache.clear(); >>>> + } >>>> + proxyClassLoaderCache.put(type.getName() + >>>> type.getClassLoader(), loader); >>>> + } >>>> return (ThreadLocalProxy<T>)Proxy.newProxyInstance(loader, >>>> new Class[] {type, >>>> ThreadLocalProxy.class }, >>>> new ThreadLocalInvocationHandler<T>()); >>>> @@ -1086,8 +1108,24 @@ public final class InjectionUtils { >>>> >>>> return (ThreadLocalProxy<T>)proxy; >>>> } >>>> - >>>> - private static boolean isServletApiContext(String name) { >>>> + >>>> + private static boolean canSeeAllClasses(ClassLoader loader, >>>> Class<?>[] interfaces) { >>>> + for (Class<?> currentInterface : interfaces) { >>>> + String ifName = currentInterface.getName(); >>>> + try { >>>> + Class<?> ifClass = Class.forName(ifName, true, loader); >>>> + if (ifClass != currentInterface) { >>>> + return false; >>>> + } >>>> + >>>> + } catch (NoClassDefFoundError | ClassNotFoundException e) { >>>> + return false; >>>> + } >>>> + } >>>> + return true; >>>> + } >>>> + >>>> + private static boolean isServletApiContext(String name) { >>>> return name.startsWith("javax.servlet."); >>>> } >>>> >>>> >>> >>> -- >>> Daniel Kulp >>> dk...@apache.org <mailto:dk...@apache.org> <mailto:dk...@apache.org >>> <mailto:dk...@apache.org>> - http://dankulp.com/blog >>> <http://dankulp.com/blog> <http://dankulp.com/blog >>> <http://dankulp.com/blog>> >>> Talend Community Coder - http://talend.com <http://talend.com/> >>> <http://coders.talend.com/ <http://coders.talend.com/>> > > -- > Daniel Kulp > dk...@apache.org <mailto:dk...@apache.org> - http://dankulp.com/blog > <http://dankulp.com/blog> > Talend Community Coder - http://talend.com <http://coders.talend.com/> -- Daniel Kulp dk...@apache.org <mailto:dk...@apache.org> - http://dankulp.com/blog <http://dankulp.com/blog> Talend Community Coder - http://talend.com <http://coders.talend.com/>
