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 <[email protected]> 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, [email protected] 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 >> >> >> 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 <[email protected]> >> 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 > [email protected] <mailto:[email protected]> - http://dankulp.com/blog > <http://dankulp.com/blog> > Talend Community Coder - http://talend.com <http://coders.talend.com/>
