From 07669933cb4f3b5c094b8d122f86ce0442be53c0 Mon Sep 17 00:00:00 2001 From: markt Date: Mon, 19 Sep 2011 15:31:43 +0000 Subject: [PATCH] Fix resource leak in annotations cache that prevented unloading of resources that used annotations git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1172664 13f79535-47bb-0310-9956-ffa450edef68 --- .../catalina/core/DefaultInstanceManager.java | 51 +++++++++++-- .../catalina/core/TestDefaultInstanceManager.java | 86 ++++++++++++++++++++++ test/webapp-3.0/annotations.jsp | 28 +++++++ 3 files changed, 157 insertions(+), 8 deletions(-) create mode 100644 test/org/apache/catalina/core/TestDefaultInstanceManager.java create mode 100644 test/webapp-3.0/annotations.jsp diff --git a/java/org/apache/catalina/core/DefaultInstanceManager.java b/java/org/apache/catalina/core/DefaultInstanceManager.java index 9a5972c54..537606d57 100644 --- a/java/org/apache/catalina/core/DefaultInstanceManager.java +++ b/java/org/apache/catalina/core/DefaultInstanceManager.java @@ -21,6 +21,7 @@ package org.apache.catalina.core; import java.io.IOException; import java.io.InputStream; +import java.lang.ref.WeakReference; import java.lang.reflect.AccessibleObject; import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; @@ -35,7 +36,7 @@ import java.util.Collections; import java.util.List; import java.util.Map; import java.util.Properties; -import java.util.concurrent.ConcurrentHashMap; +import java.util.WeakHashMap; import javax.annotation.PostConstruct; import javax.annotation.PreDestroy; @@ -70,8 +71,8 @@ public class DefaultInstanceManager implements InstanceManager { private Properties restrictedFilters = new Properties(); private Properties restrictedListeners = new Properties(); private Properties restrictedServlets = new Properties(); - private Map,List> annotationCache = - new ConcurrentHashMap, List>(); + private final Map,WeakReference>> annotationCache = + new WeakHashMap, WeakReference>>(); public DefaultInstanceManager(Context context, Map> injectionMap, org.apache.catalina.Context catalinaContext, ClassLoader containerClassLoader) { classLoader = catalinaContext.getLoader().getClassLoader(); @@ -178,7 +179,10 @@ public class DefaultInstanceManager implements InstanceManager { // At the end the postconstruct annotated // method is invoked - List annotations = annotationCache.get(clazz); + List annotations; + synchronized (annotationCache) { + annotations = annotationCache.get(clazz).get(); + } for (AnnotationCacheEntry entry : annotations) { if (entry.getType() == AnnotationCacheEntryType.POST_CONSTRUCT) { Method postConstruct = (Method) entry.getAccessibleObject(); @@ -209,7 +213,14 @@ public class DefaultInstanceManager implements InstanceManager { // At the end the postconstruct annotated // method is invoked - List annotations = annotationCache.get(clazz); + List annotations = null; + synchronized (annotationCache) { + WeakReference> ref = + annotationCache.get(clazz); + if (ref != null) { + annotations = ref.get(); + } + } if (annotations == null) { // instance not created through the instance manager return; @@ -243,7 +254,14 @@ public class DefaultInstanceManager implements InstanceManager { InvocationTargetException, NamingException { while (clazz != null) { - List annotations = annotationCache.get(clazz); + List annotations = null; + synchronized (annotationCache) { + WeakReference> ref = + annotationCache.get(clazz); + if (ref != null) { + annotations = ref.get(); + } + } if (annotations == null) { annotations = new ArrayList(); @@ -396,7 +414,11 @@ public class DefaultInstanceManager implements InstanceManager { // Use common empty list to save memory annotations = Collections.emptyList(); } - annotationCache.put(clazz, annotations); + synchronized (annotationCache) { + annotationCache.put(clazz, + new WeakReference>( + annotations)); + } } else { // If the annotations for this class have been cached, the // annotations for all the super classes will have been cachced @@ -429,7 +451,10 @@ public class DefaultInstanceManager implements InstanceManager { Class clazz = instance.getClass(); while (clazz != null) { - List annotations = annotationCache.get(clazz); + List annotations; + synchronized (annotationCache) { + annotations = annotationCache.get(clazz).get(); + } for (AnnotationCacheEntry entry : annotations) { if (entry.getType() == AnnotationCacheEntryType.FIELD) { if (entry.getAccessibleObject() instanceof Method) { @@ -448,6 +473,16 @@ public class DefaultInstanceManager implements InstanceManager { } + /** + * Makes cache size available to unit tests. + */ + protected int getAnnotationCacheSize() { + synchronized (annotationCache) { + return annotationCache.size(); + } + } + + protected Class loadClassMaybePrivileged(final String className, final ClassLoader classLoader) throws ClassNotFoundException { Class clazz; if (SecurityUtil.isPackageProtectionEnabled()) { diff --git a/test/org/apache/catalina/core/TestDefaultInstanceManager.java b/test/org/apache/catalina/core/TestDefaultInstanceManager.java new file mode 100644 index 000000000..e2c1befcc --- /dev/null +++ b/test/org/apache/catalina/core/TestDefaultInstanceManager.java @@ -0,0 +1,86 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.catalina.core; + +import java.io.File; + +import static org.junit.Assert.assertEquals; +import org.junit.Test; + +import org.apache.catalina.Wrapper; +import org.apache.catalina.startup.Tomcat; +import org.apache.catalina.startup.TomcatBaseTest; + + +public class TestDefaultInstanceManager extends TomcatBaseTest { + + @Test + public void testClassUnloading() throws Exception { + + DefaultInstanceManager instanceManager = doClassUnloadingPrep(); + + // Request a JSP page (that doesn't load any tag libraries etc.) + // This page does use @PostConstruct to ensure that the cache does not + // retain strong references + getUrl("http://localhost:" + getPort() + "/test/annotations.jsp"); + // Request a second JSP (again, no tag libraries etc.) + getUrl("http://localhost:" + getPort() + "/test/bug36923.jsp"); + + // Check the number of classes in the cache + int count = instanceManager.getAnnotationCacheSize(); + + // Request a third JSP (again, no tag libraries etc.) + getUrl("http://localhost:" + getPort() + "/test/bug51544.jsp"); + + // Force a GC to clear out unloaded class (first JSP) + System.gc(); + + // Spin a while until GC happens or we wait too long + int loop = 0; + while (loop < 10) { + if (instanceManager.getAnnotationCacheSize() == count) { + break; + } + Thread.sleep(100); + loop++; + } + + // First JSP should be unloaded and replaced by third (second left + // alone) so no change in overall count + assertEquals(count, instanceManager.getAnnotationCacheSize()); + } + + private DefaultInstanceManager doClassUnloadingPrep() throws Exception { + Tomcat tomcat = getTomcatInstance(); + + // Create the context (don't use addWebapp as we want to modify the + // JSP Servlet settings). + File appDir = new File("test/webapp-3.0"); + StandardContext ctxt = (StandardContext) tomcat.addContext( + null, "/test", appDir.getAbsolutePath()); + + // Configure the defaults and then tweak the JSP servlet settings + // Note: Min value for maxLoadedJsps is 2 + Tomcat.initWebappDefaults(ctxt); + Wrapper w = (Wrapper) ctxt.findChild("jsp"); + w.addInitParameter("maxLoadedJsps", "2"); + + tomcat.start(); + + return (DefaultInstanceManager) ctxt.getInstanceManager(); + } +} diff --git a/test/webapp-3.0/annotations.jsp b/test/webapp-3.0/annotations.jsp new file mode 100644 index 000000000..180743dcb --- /dev/null +++ b/test/webapp-3.0/annotations.jsp @@ -0,0 +1,28 @@ +<%-- + Licensed to the Apache Software Foundation (ASF) under one or more + contributor license agreements. See the NOTICE file distributed with + this work for additional information regarding copyright ownership. + The ASF licenses this file to You under the Apache License, Version 2.0 + (the "License"); you may not use this file except in compliance with + the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +--%> +<%@page import="javax.annotation.PostConstruct"%> + + Annotations test case + +

Hello world

+ + +<%! + @PostConstruct + public void doNothing() { + } +%> \ No newline at end of file -- 2.11.0