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 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 <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> - http://dankulp.com/blog <http://dankulp.com/blog> Talend Community Coder - http://talend.com <http://coders.talend.com/>
