This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch fix/WW-5537-memory-leak-cleanup in repository https://gitbox.apache.org/repos/asf/struts.git
commit 4a1054d3bc4549c9dfcbcc9bad2951153214161a Author: Lukasz Lenart <[email protected]> AuthorDate: Mon Mar 23 07:25:07 2026 +0100 WW-5537 fix(core): resolve classloader/memory leaks during Tomcat hot deployment Introduce InternalDestroyable interface with container-based discovery to clean up static caches, daemon threads, and shared references that pin the webapp classloader after undeploy. This prevents OutOfMemoryError (Metaspace) on repeated hot deployments. Changes: - Add InternalDestroyable/ContextAwareDestroyable interfaces for cleanup hooks - Clear OGNL, Component, ScopeInterceptor, DefaultFileManager static caches - Stop FinalizableReferenceQueue daemon thread and null its classloader - Clear FreeMarker template/introspection caches from ServletContext - Replace ContainerHolder ThreadLocal with volatile to prevent thread-pool leaks - Clear static dispatcherListeners list on Dispatcher cleanup - Add JSONCacheDestroyable for json plugin cache cleanup - Register all destroyables via struts-beans.xml / struts-plugin.xml Co-Authored-By: Claude Opus 4.6 <[email protected]> --- .../inject/util/FinalizableReferenceQueue.java | 35 +++- .../xwork2/ognl/accessor/CompoundRootAccessor.java | 19 +- .../xwork2/util/fs/DefaultFileManager.java | 19 +- .../org/apache/struts2/components/Component.java | 7 + ...rHolder.java => ComponentCacheDestroyable.java} | 31 +-- .../apache/struts2/dispatcher/ContainerHolder.java | 22 +- .../dispatcher/ContextAwareDestroyable.java | 53 +++++ .../org/apache/struts2/dispatcher/Dispatcher.java | 98 +++++++-- ...a => FinalizableReferenceQueueDestroyable.java} | 31 +-- .../dispatcher/FreemarkerCacheDestroyable.java | 57 ++++++ .../struts2/dispatcher/InternalDestroyable.java | 50 +++++ ...tainerHolder.java => OgnlCacheDestroyable.java} | 33 ++- .../struts2/dispatcher/PrepareOperations.java | 1 + ....java => ScopeInterceptorCacheDestroyable.java} | 32 +-- .../struts2/interceptor/ScopeInterceptor.java | 16 +- core/src/main/resources/struts-beans.xml | 16 ++ .../inject/util/FinalizableReferenceQueueTest.java | 38 ++++ .../dispatcher/DispatcherCleanupLeakTest.java | 227 +++++++++++++++++++++ .../org/apache/struts2/json/DefaultJSONWriter.java | 8 + .../apache/struts2/json/JSONCacheDestroyable.java | 35 ++++ plugins/json/src/main/resources/struts-plugin.xml | 2 + 21 files changed, 714 insertions(+), 116 deletions(-) diff --git a/core/src/main/java/com/opensymphony/xwork2/inject/util/FinalizableReferenceQueue.java b/core/src/main/java/com/opensymphony/xwork2/inject/util/FinalizableReferenceQueue.java index 2335b2722..6295b2d22 100644 --- a/core/src/main/java/com/opensymphony/xwork2/inject/util/FinalizableReferenceQueue.java +++ b/core/src/main/java/com/opensymphony/xwork2/inject/util/FinalizableReferenceQueue.java @@ -26,7 +26,7 @@ import java.util.logging.Logger; * * @author Bob Lee ([email protected]) */ -class FinalizableReferenceQueue extends ReferenceQueue<Object> { +public class FinalizableReferenceQueue extends ReferenceQueue<Object> { private static final Logger logger = Logger.getLogger(FinalizableReferenceQueue.class.getName()); @@ -45,22 +45,49 @@ class FinalizableReferenceQueue extends ReferenceQueue<Object> { logger.log(Level.SEVERE, "Error cleaning up after reference.", t); } + private volatile Thread drainThread; + void start() { Thread thread = new Thread("FinalizableReferenceQueue") { @Override public void run() { - while (true) { + while (!Thread.currentThread().isInterrupted()) { try { cleanUp(remove()); - } catch (InterruptedException e) { /* ignore */ } + } catch (InterruptedException e) { + break; + } } } }; thread.setDaemon(true); thread.start(); + this.drainThread = thread; + } + + /** + * Stops the background drain thread and releases the singleton instance, + * preventing the webapp classloader from being pinned after undeploy. + */ + public static synchronized void stopAndClear() { + if (instance instanceof FinalizableReferenceQueue) { + FinalizableReferenceQueue queue = (FinalizableReferenceQueue) instance; + Thread t = queue.drainThread; + if (t != null) { + t.interrupt(); + try { + t.join(5000); + } catch (InterruptedException ignored) { + Thread.currentThread().interrupt(); + } + t.setContextClassLoader(null); + queue.drainThread = null; + } + } + instance = null; } - static ReferenceQueue<Object> instance = createAndStart(); + static volatile ReferenceQueue<Object> instance = createAndStart(); static FinalizableReferenceQueue createAndStart() { FinalizableReferenceQueue queue = new FinalizableReferenceQueue(); diff --git a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java index be332047b..fc5ac04e8 100644 --- a/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java +++ b/core/src/main/java/com/opensymphony/xwork2/ognl/accessor/CompoundRootAccessor.java @@ -33,6 +33,7 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.StrutsConstants; import org.apache.struts2.StrutsException; +import org.apache.struts2.dispatcher.InternalDestroyable; import java.beans.IntrospectionException; import java.beans.PropertyDescriptor; @@ -53,7 +54,7 @@ import static org.apache.commons.lang3.BooleanUtils.toBoolean; * @author Rainer Hermanns * @version $Revision$ */ -public class CompoundRootAccessor implements RootAccessor { +public class CompoundRootAccessor implements RootAccessor, InternalDestroyable { /** * Used by OGNl to generate bytecode @@ -74,6 +75,22 @@ public class CompoundRootAccessor implements RootAccessor { private final static Logger LOG = LogManager.getLogger(CompoundRootAccessor.class); private final static Class[] EMPTY_CLASS_ARRAY = new Class[0]; private static final Map<MethodCall, Boolean> invalidMethods = new ConcurrentHashMap<>(); + + /** + * Clears the cached invalid methods map to prevent classloader leaks on hot redeploy. + */ + public static void clearCache() { + invalidMethods.clear(); + } + + /** + * @since 6.9.0 + */ + @Override + public void destroy() { + clearCache(); + } + private boolean devMode; private boolean disallowCustomOgnlMap; diff --git a/core/src/main/java/com/opensymphony/xwork2/util/fs/DefaultFileManager.java b/core/src/main/java/com/opensymphony/xwork2/util/fs/DefaultFileManager.java index 5708bd5f2..48317cf68 100644 --- a/core/src/main/java/com/opensymphony/xwork2/util/fs/DefaultFileManager.java +++ b/core/src/main/java/com/opensymphony/xwork2/util/fs/DefaultFileManager.java @@ -21,6 +21,7 @@ package com.opensymphony.xwork2.util.fs; import com.opensymphony.xwork2.FileManager; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.dispatcher.InternalDestroyable; import java.io.IOException; import java.io.InputStream; @@ -33,7 +34,7 @@ import java.util.regex.Pattern; /** * Default implementation of {@link FileManager} */ -public class DefaultFileManager implements FileManager { +public class DefaultFileManager implements FileManager, InternalDestroyable { private static Logger LOG = LogManager.getLogger(DefaultFileManager.class); @@ -43,6 +44,22 @@ public class DefaultFileManager implements FileManager { protected static final Map<String, Revision> files = Collections.synchronizedMap(new HashMap<String, Revision>()); private static final List<URL> lazyMonitoredFilesCache = Collections.synchronizedList(new ArrayList<URL>()); + /** + * Clears both the files and lazy monitored files caches to prevent classloader leaks on hot redeploy. + */ + public static void clearCache() { + files.clear(); + lazyMonitoredFilesCache.clear(); + } + + /** + * @since 6.9.0 + */ + @Override + public void destroy() { + clearCache(); + } + protected boolean reloadingConfigs = false; public DefaultFileManager() { diff --git a/core/src/main/java/org/apache/struts2/components/Component.java b/core/src/main/java/org/apache/struts2/components/Component.java index d61bcc072..7e6bb4d67 100644 --- a/core/src/main/java/org/apache/struts2/components/Component.java +++ b/core/src/main/java/org/apache/struts2/components/Component.java @@ -68,6 +68,13 @@ public class Component { */ protected static ConcurrentMap<Class<?>, Collection<String>> standardAttributesMap = new ConcurrentHashMap<>(); + /** + * Clears the cached standard attributes map to prevent classloader leaks on hot redeploy. + */ + public static void clearStandardAttributesMap() { + standardAttributesMap.clear(); + } + protected boolean devMode = false; protected boolean escapeHtmlBody = false; protected ValueStack stack; diff --git a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java b/core/src/main/java/org/apache/struts2/dispatcher/ComponentCacheDestroyable.java similarity index 51% copy from core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java copy to core/src/main/java/org/apache/struts2/dispatcher/ComponentCacheDestroyable.java index 2d1c61657..5d0249172 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/ComponentCacheDestroyable.java @@ -18,30 +18,19 @@ */ package org.apache.struts2.dispatcher; -import com.opensymphony.xwork2.inject.Container; +import org.apache.struts2.components.Component; /** - * Simple class to hold Container instance per thread to minimise number of attempts - * to read configuration and build each time a new configuration. - * <p> - * As ContainerHolder operates just per thread (which means per request) there is no need - * to check if configuration changed during the same request. If changed between requests, - * first call to store Container in ContainerHolder will be with the new configuration. + * Clears {@link Component}'s static standard attributes cache to prevent + * classloader leaks on hot redeploy. Wrapper is needed because {@code Component} + * requires constructor arguments that prevent direct container instantiation. + * + * @since 6.9.0 */ -class ContainerHolder { - - private static final ThreadLocal<Container> instance = new ThreadLocal<>(); - - public static void store(Container newInstance) { - instance.set(newInstance); - } +public class ComponentCacheDestroyable implements InternalDestroyable { - public static Container get() { - return instance.get(); + @Override + public void destroy() { + Component.clearStandardAttributesMap(); } - - public static void clear() { - instance.remove(); - } - } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java b/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java index 2d1c61657..3d54082cd 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java @@ -21,27 +21,33 @@ package org.apache.struts2.dispatcher; import com.opensymphony.xwork2.inject.Container; /** - * Simple class to hold Container instance per thread to minimise number of attempts + * Simple class to hold Container instance to minimise number of attempts * to read configuration and build each time a new configuration. * <p> - * As ContainerHolder operates just per thread (which means per request) there is no need - * to check if configuration changed during the same request. If changed between requests, - * first call to store Container in ContainerHolder will be with the new configuration. + * WW-5537: Changed from ThreadLocal to volatile shared reference to prevent + * classloader leaks during hot redeployment. ThreadLocal values on idle pool + * threads are not cleared by {@code ThreadLocal.remove()} on other threads, + * causing the Container (and its classloader) to be retained after undeploy. */ class ContainerHolder { - private static final ThreadLocal<Container> instance = new ThreadLocal<>(); + private static volatile Container instance; public static void store(Container newInstance) { - instance.set(newInstance); + instance = newInstance; } public static Container get() { - return instance.get(); + return instance; } + /** + * Clears the shared container reference. Safe to call concurrently with + * {@link #store(Container)} because {@link Dispatcher#getContainer()} lazily + * repopulates the holder from the configuration manager when it finds a null value. + */ public static void clear() { - instance.remove(); + instance = null; } } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/ContextAwareDestroyable.java b/core/src/main/java/org/apache/struts2/dispatcher/ContextAwareDestroyable.java new file mode 100644 index 000000000..578620d91 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/dispatcher/ContextAwareDestroyable.java @@ -0,0 +1,53 @@ +/* + * 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.struts2.dispatcher; + +import javax.servlet.ServletContext; + +/** + * Extension of {@link InternalDestroyable} for components that require + * {@link ServletContext} during cleanup (e.g. clearing servlet-scoped caches). + * + * <p>During {@link Dispatcher#cleanup()}, the discovery loop checks each + * {@code InternalDestroyable} bean: if it implements this subinterface, + * {@link #destroy(ServletContext)} is called instead of {@link #destroy()}.</p> + * + * @since 6.9.0 + * @see InternalDestroyable + * @see Dispatcher#cleanup() + */ +public interface ContextAwareDestroyable extends InternalDestroyable { + + /** + * Releases state that requires access to the {@link ServletContext}. + * + * @param servletContext the current servlet context, may be {@code null} + * if the Dispatcher was created without one + */ + void destroy(ServletContext servletContext); + + /** + * Default no-op — {@link Dispatcher} calls + * {@link #destroy(ServletContext)} instead when it recognises this type. + */ + @Override + default void destroy() { + // no-op: context-aware variant is the real entry point + } +} diff --git a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java index f55108719..03a25a365 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/Dispatcher.java @@ -451,7 +451,36 @@ public class Dispatcher { * Releases all instances bound to this dispatcher instance. */ public void cleanup() { - // clean up ObjectFactory + destroyObjectFactory(); + + // clean up Dispatcher itself for this thread + instance.remove(); + servletContext.setAttribute(StrutsStatics.SERVLET_DISPATCHER, null); + + destroyDispatcherListeners(); + + destroyInterceptors(); + + destroyInternalBeans(); + + // Clear container holder when application is unloaded / server shutdown + ContainerHolder.clear(); + + //cleanup action context + ActionContext.clear(); + + // clean up configuration + configurationManager.destroyConfiguration(); + configurationManager = null; + } + + /** + * Destroys the {@link ObjectFactory} if it implements {@link ObjectFactoryDestroyable}. + * Called at the beginning of {@link #cleanup()}. + * + * @since 6.9.0 + */ + protected void destroyObjectFactory() { if (objectFactory == null) { LOG.warn("Object Factory is null, something is seriously wrong, no clean up will be performed"); } @@ -459,23 +488,36 @@ public class Dispatcher { try { ((ObjectFactoryDestroyable) objectFactory).destroy(); } catch (Exception e) { - // catch any exception that may occur during destroy() and log it LOG.error("Exception occurred while destroying ObjectFactory [{}]", objectFactory.toString(), e); } } + } - // clean up Dispatcher itself for this thread - instance.remove(); - servletContext.setAttribute(StrutsStatics.SERVLET_DISPATCHER, null); - - // clean up DispatcherListeners + /** + * Notifies all registered {@link DispatcherListener}s that this dispatcher + * is being destroyed, then clears the listener list. + * + * @since 6.9.0 + */ + protected void destroyDispatcherListeners() { if (!dispatcherListeners.isEmpty()) { for (DispatcherListener l : dispatcherListeners) { l.dispatcherDestroyed(this); } + // WW-5537: Clear the static listener list to release references that may + // pin the webapp classloader after undeploy. Listeners must be re-registered + // if a new Dispatcher is created (e.g. on redeploy). + dispatcherListeners.clear(); } + } - // clean up all interceptors by calling their destroy() method + /** + * Destroys all interceptors registered in the current configuration. + * Called during {@link #cleanup()} before {@link #destroyInternalBeans()}. + * + * @since 6.9.0 + */ + protected void destroyInterceptors() { Set<Interceptor> interceptors = new HashSet<>(); Collection<PackageConfig> packageConfigs = configurationManager.getConfiguration().getPackageConfigs().values(); for (PackageConfig packageConfig : packageConfigs) { @@ -490,16 +532,38 @@ public class Dispatcher { for (Interceptor interceptor : interceptors) { interceptor.destroy(); } + } - // Clear container holder when application is unloaded / server shutdown - ContainerHolder.clear(); - - //cleanup action context - ActionContext.clear(); - - // clean up configuration - configurationManager.destroyConfiguration(); - configurationManager = null; + /** + * Discovers and invokes all {@link InternalDestroyable} beans registered + * in the container, clearing static caches and stopping daemon threads + * to prevent classloader leaks during hot redeployment (WW-5537). + * + * <p>Beans implementing {@link ContextAwareDestroyable} receive the + * {@link javax.servlet.ServletContext} via + * {@link ContextAwareDestroyable#destroy(javax.servlet.ServletContext)}.</p> + * + * @since 6.9.0 + */ + protected void destroyInternalBeans() { + if (configurationManager != null && configurationManager.getConfiguration() != null) { + Container container = configurationManager.getConfiguration().getContainer(); + Set<String> destroyableNames = container.getInstanceNames(InternalDestroyable.class); + for (String name : destroyableNames) { + try { + InternalDestroyable destroyable = container.getInstance(InternalDestroyable.class, name); + if (destroyable instanceof ContextAwareDestroyable) { + ((ContextAwareDestroyable) destroyable).destroy(servletContext); + } else { + destroyable.destroy(); + } + } catch (Exception e) { + LOG.warn("Error during internal cleanup [{}]", name, e); + } + } + } else { + LOG.warn("ConfigurationManager is null during cleanup, InternalDestroyable beans will not be invoked"); + } } private void init_FileManager() throws ClassNotFoundException { diff --git a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java b/core/src/main/java/org/apache/struts2/dispatcher/FinalizableReferenceQueueDestroyable.java similarity index 51% copy from core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java copy to core/src/main/java/org/apache/struts2/dispatcher/FinalizableReferenceQueueDestroyable.java index 2d1c61657..1535d87e2 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/FinalizableReferenceQueueDestroyable.java @@ -18,30 +18,19 @@ */ package org.apache.struts2.dispatcher; -import com.opensymphony.xwork2.inject.Container; +import com.opensymphony.xwork2.inject.util.FinalizableReferenceQueue; /** - * Simple class to hold Container instance per thread to minimise number of attempts - * to read configuration and build each time a new configuration. - * <p> - * As ContainerHolder operates just per thread (which means per request) there is no need - * to check if configuration changed during the same request. If changed between requests, - * first call to store Container in ContainerHolder will be with the new configuration. + * Adapter that exposes {@link FinalizableReferenceQueue#stopAndClear()} as an + * {@link InternalDestroyable} bean, since {@code FinalizableReferenceQueue} + * has a private constructor and cannot be directly registered in the container. + * + * @since 6.9.0 */ -class ContainerHolder { - - private static final ThreadLocal<Container> instance = new ThreadLocal<>(); - - public static void store(Container newInstance) { - instance.set(newInstance); - } +public class FinalizableReferenceQueueDestroyable implements InternalDestroyable { - public static Container get() { - return instance.get(); + @Override + public void destroy() { + FinalizableReferenceQueue.stopAndClear(); } - - public static void clear() { - instance.remove(); - } - } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/FreemarkerCacheDestroyable.java b/core/src/main/java/org/apache/struts2/dispatcher/FreemarkerCacheDestroyable.java new file mode 100644 index 000000000..946684e03 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/dispatcher/FreemarkerCacheDestroyable.java @@ -0,0 +1,57 @@ +/* + * 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.struts2.dispatcher; + +import freemarker.ext.beans.BeansWrapper; +import freemarker.template.Configuration; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.views.freemarker.FreemarkerManager; + +import javax.servlet.ServletContext; + +/** + * WW-5537: Clears FreeMarker's template and class introspection caches + * stored in {@link ServletContext} during application undeploy, preventing + * classloader leaks. + * + * @since 6.9.0 + */ +public class FreemarkerCacheDestroyable implements ContextAwareDestroyable { + + private static final Logger LOG = LogManager.getLogger(FreemarkerCacheDestroyable.class); + + @Override + public void destroy(ServletContext servletContext) { + if (servletContext == null) { + return; + } + Object fmConfig = servletContext.getAttribute(FreemarkerManager.CONFIG_SERVLET_CONTEXT_KEY); + if (fmConfig instanceof Configuration) { + Configuration cfg = (Configuration) fmConfig; + cfg.clearTemplateCache(); + cfg.clearEncodingMap(); + if (cfg.getObjectWrapper() instanceof BeansWrapper) { + ((BeansWrapper) cfg.getObjectWrapper()).clearClassIntrospectionCache(); + } + servletContext.removeAttribute(FreemarkerManager.CONFIG_SERVLET_CONTEXT_KEY); + LOG.debug("FreeMarker configuration cleaned up"); + } + } +} diff --git a/core/src/main/java/org/apache/struts2/dispatcher/InternalDestroyable.java b/core/src/main/java/org/apache/struts2/dispatcher/InternalDestroyable.java new file mode 100644 index 000000000..085f54879 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/dispatcher/InternalDestroyable.java @@ -0,0 +1,50 @@ +/* + * 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.struts2.dispatcher; + +/** + * Internal framework interface for components that hold static state + * (caches, daemon threads, etc.) requiring cleanup during application + * undeploy to prevent classloader leaks. + * + * <p>Implementations are registered as named beans in {@code struts-beans.xml} + * (or plugin descriptors) with type {@code InternalDestroyable}. During + * {@link Dispatcher#cleanup()}, all registered implementations are discovered + * via {@code Container.getInstanceNames(InternalDestroyable.class)} and + * invoked automatically.</p> + * + * <p>The order in which implementations are invoked is not guaranteed. + * Implementations must not depend on other {@code InternalDestroyable} + * beans having been (or not yet been) destroyed. Ordering can be + * influenced via the {@code order} attribute in bean registration.</p> + * + * <p>This is not part of the public user API. For user/plugin lifecycle + * callbacks, use {@link DispatcherListener} instead.</p> + * + * @since 6.9.0 + * @see Dispatcher#cleanup() + */ +public interface InternalDestroyable { + + /** + * Releases static state held by this component. Called once during + * {@link Dispatcher#cleanup()}. + */ + void destroy(); +} diff --git a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java b/core/src/main/java/org/apache/struts2/dispatcher/OgnlCacheDestroyable.java similarity index 51% copy from core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java copy to core/src/main/java/org/apache/struts2/dispatcher/OgnlCacheDestroyable.java index 2d1c61657..9bf9915b9 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/OgnlCacheDestroyable.java @@ -18,30 +18,21 @@ */ package org.apache.struts2.dispatcher; -import com.opensymphony.xwork2.inject.Container; +import com.opensymphony.xwork2.ognl.OgnlUtil; + +import java.beans.Introspector; /** - * Simple class to hold Container instance per thread to minimise number of attempts - * to read configuration and build each time a new configuration. - * <p> - * As ContainerHolder operates just per thread (which means per request) there is no need - * to check if configuration changed during the same request. If changed between requests, - * first call to store Container in ContainerHolder will be with the new configuration. + * Clears OGNL runtime caches and JDK introspection caches that hold + * {@code Class<?>} references, preventing classloader leaks on hot redeploy. + * + * @since 6.9.0 */ -class ContainerHolder { - - private static final ThreadLocal<Container> instance = new ThreadLocal<>(); - - public static void store(Container newInstance) { - instance.set(newInstance); - } - - public static Container get() { - return instance.get(); - } +public class OgnlCacheDestroyable implements InternalDestroyable { - public static void clear() { - instance.remove(); + @Override + public void destroy() { + OgnlUtil.clearRuntimeCache(); + Introspector.flushCaches(); } - } diff --git a/core/src/main/java/org/apache/struts2/dispatcher/PrepareOperations.java b/core/src/main/java/org/apache/struts2/dispatcher/PrepareOperations.java index ab4323526..961412a5d 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/PrepareOperations.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/PrepareOperations.java @@ -77,6 +77,7 @@ public class PrepareOperations { } finally { ActionContext.clear(); Dispatcher.clearInstance(); + ContainerHolder.clear(); devModeOverride.remove(); } }); diff --git a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java b/core/src/main/java/org/apache/struts2/dispatcher/ScopeInterceptorCacheDestroyable.java similarity index 51% copy from core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java copy to core/src/main/java/org/apache/struts2/dispatcher/ScopeInterceptorCacheDestroyable.java index 2d1c61657..7a7f54406 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/ScopeInterceptorCacheDestroyable.java @@ -18,30 +18,20 @@ */ package org.apache.struts2.dispatcher; -import com.opensymphony.xwork2.inject.Container; +import org.apache.struts2.interceptor.ScopeInterceptor; /** - * Simple class to hold Container instance per thread to minimise number of attempts - * to read configuration and build each time a new configuration. - * <p> - * As ContainerHolder operates just per thread (which means per request) there is no need - * to check if configuration changed during the same request. If changed between requests, - * first call to store Container in ContainerHolder will be with the new configuration. + * Clears {@link ScopeInterceptor}'s static locks map to prevent classloader + * leaks on hot redeploy. Separated from the interceptor itself because the + * locks map is static and must be cleared regardless of whether the interceptor + * is configured in any package. + * + * @since 6.9.0 */ -class ContainerHolder { - - private static final ThreadLocal<Container> instance = new ThreadLocal<>(); - - public static void store(Container newInstance) { - instance.set(newInstance); - } +public class ScopeInterceptorCacheDestroyable implements InternalDestroyable { - public static Container get() { - return instance.get(); + @Override + public void destroy() { + ScopeInterceptor.clearLocks(); } - - public static void clear() { - instance.remove(); - } - } diff --git a/core/src/main/java/org/apache/struts2/interceptor/ScopeInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ScopeInterceptor.java index 765fa40ed..77789c1d2 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ScopeInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ScopeInterceptor.java @@ -232,7 +232,21 @@ public class ScopeInterceptor extends AbstractInterceptor implements PreResultLi return o; } - private static Map<Object, Object> locks = new IdentityHashMap<>(); + private static final Map<Object, Object> locks = new IdentityHashMap<>(); + + /** + * Clears the locks map to prevent classloader leaks on hot redeploy. + */ + public static void clearLocks() { + synchronized (locks) { + locks.clear(); + } + } + + @Override + public void destroy() { + clearLocks(); + } static void lock(Object o, ActionInvocation invocation) throws Exception { synchronized (o) { diff --git a/core/src/main/resources/struts-beans.xml b/core/src/main/resources/struts-beans.xml index bb0ede480..878692bdf 100644 --- a/core/src/main/resources/struts-beans.xml +++ b/core/src/main/resources/struts-beans.xml @@ -274,4 +274,20 @@ <bean type="org.apache.struts2.interceptor.csp.CspNonceReader" name="struts" class="org.apache.struts2.interceptor.csp.StrutsCspNonceReader"/> + <!-- WW-5537: InternalDestroyable beans for automatic cleanup during undeploy --> + <bean type="org.apache.struts2.dispatcher.InternalDestroyable" name="componentCache" + class="org.apache.struts2.dispatcher.ComponentCacheDestroyable"/> + <bean type="org.apache.struts2.dispatcher.InternalDestroyable" name="compoundRootAccessor" + class="com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor"/> + <bean type="org.apache.struts2.dispatcher.InternalDestroyable" name="defaultFileManager" + class="com.opensymphony.xwork2.util.fs.DefaultFileManager"/> + <bean type="org.apache.struts2.dispatcher.InternalDestroyable" name="scopeInterceptorCache" + class="org.apache.struts2.dispatcher.ScopeInterceptorCacheDestroyable"/> + <bean type="org.apache.struts2.dispatcher.InternalDestroyable" name="ognlCache" + class="org.apache.struts2.dispatcher.OgnlCacheDestroyable"/> + <bean type="org.apache.struts2.dispatcher.InternalDestroyable" name="finalizableReferenceQueue" + class="org.apache.struts2.dispatcher.FinalizableReferenceQueueDestroyable"/> + <bean type="org.apache.struts2.dispatcher.InternalDestroyable" name="freemarkerCache" + class="org.apache.struts2.dispatcher.FreemarkerCacheDestroyable"/> + </struts> diff --git a/core/src/test/java/com/opensymphony/xwork2/inject/util/FinalizableReferenceQueueTest.java b/core/src/test/java/com/opensymphony/xwork2/inject/util/FinalizableReferenceQueueTest.java new file mode 100644 index 000000000..2d09a9655 --- /dev/null +++ b/core/src/test/java/com/opensymphony/xwork2/inject/util/FinalizableReferenceQueueTest.java @@ -0,0 +1,38 @@ +/* + * 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 com.opensymphony.xwork2.inject.util; + +import org.junit.Test; + +import java.lang.ref.ReferenceQueue; + +import static org.junit.Assert.assertNull; + +public class FinalizableReferenceQueueTest { + + @Test + public void stopAndClearIsIdempotent() { + // Should not throw even when called multiple times + FinalizableReferenceQueue.stopAndClear(); + FinalizableReferenceQueue.stopAndClear(); + + ReferenceQueue<Object> instance = FinalizableReferenceQueue.getInstance(); + assertNull("FinalizableReferenceQueue instance should be null after stopAndClear", instance); + } +} diff --git a/core/src/test/java/org/apache/struts2/dispatcher/DispatcherCleanupLeakTest.java b/core/src/test/java/org/apache/struts2/dispatcher/DispatcherCleanupLeakTest.java new file mode 100644 index 000000000..94ce919b3 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/DispatcherCleanupLeakTest.java @@ -0,0 +1,227 @@ +/* + * 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.struts2.dispatcher; + +import com.opensymphony.xwork2.inject.Container; +import com.opensymphony.xwork2.ognl.accessor.CompoundRootAccessor; +import com.opensymphony.xwork2.util.fs.DefaultFileManager; +import org.apache.struts2.StrutsJUnit4InternalTestCase; +import org.apache.struts2.components.Component; +import org.apache.struts2.interceptor.ScopeInterceptor; +import org.junit.Test; + +import java.lang.reflect.Field; +import java.net.URL; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; +import java.util.concurrent.ConcurrentMap; +import java.util.concurrent.atomic.AtomicBoolean; + +import static java.util.Collections.emptyMap; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; + +/** + * WW-5537: Verifies that Dispatcher.cleanup() properly clears all static state + * that could prevent classloader garbage collection during hot redeployment. + */ +public class DispatcherCleanupLeakTest extends StrutsJUnit4InternalTestCase { + + @Test + public void cleanupDiscoversAllInternalDestroyableBeans() { + initDispatcher(emptyMap()); + + Container container = dispatcher.getConfigurationManager().getConfiguration().getContainer(); + Set<String> names = container.getInstanceNames(InternalDestroyable.class); + + Set<String> expected = new HashSet<>(Arrays.asList( + "componentCache", "compoundRootAccessor", "defaultFileManager", + "scopeInterceptorCache", "ognlCache", "finalizableReferenceQueue", + "freemarkerCache" + )); + assertTrue("All core InternalDestroyable beans should be registered, missing: " + + missing(expected, names), + names.containsAll(expected)); + } + + @Test + public void cleanupContinuesWhenDestroyableThrows() { + initDispatcher(emptyMap()); + + // Populate a cache to verify cleanup still runs after a failure + Field mapField; + try { + mapField = Component.class.getDeclaredField("standardAttributesMap"); + mapField.setAccessible(true); + @SuppressWarnings("unchecked") + ConcurrentMap<Class<?>, Collection<String>> map = + (ConcurrentMap<Class<?>, Collection<String>>) mapField.get(null); + map.put(String.class, new ArrayList<>()); + assertFalse("Precondition: standardAttributesMap should not be empty", map.isEmpty()); + } catch (Exception e) { + throw new RuntimeException(e); + } + + // Register a destroyable that throws before other cleanup runs + final AtomicBoolean secondCalled = new AtomicBoolean(false); + InternalDestroyable failing = () -> { throw new RuntimeException("test failure"); }; + InternalDestroyable tracking = () -> secondCalled.set(true); + + // Call cleanup — the loop should catch the exception and continue + Container container = dispatcher.getConfigurationManager().getConfiguration().getContainer(); + Set<String> names = container.getInstanceNames(InternalDestroyable.class); + + // Simulate the loop with our test destroyables injected + List<InternalDestroyable> destroyables = new ArrayList<>(); + destroyables.add(failing); + for (String name : names) { + destroyables.add(container.getInstance(InternalDestroyable.class, name)); + } + destroyables.add(tracking); + + for (InternalDestroyable d : destroyables) { + try { + d.destroy(); + } catch (Exception e) { + // mirrors Dispatcher.cleanup() error handling + } + } + + assertTrue("Destroyable after the failing one should still be called", secondCalled.get()); + } + + @Test + @SuppressWarnings("unchecked") + public void cleanupClearsComponentStandardAttributesMap() throws Exception { + initDispatcher(emptyMap()); + + Field mapField = Component.class.getDeclaredField("standardAttributesMap"); + mapField.setAccessible(true); + ConcurrentMap<Class<?>, Collection<String>> map = + (ConcurrentMap<Class<?>, Collection<String>>) mapField.get(null); + + map.put(String.class, new ArrayList<>()); + assertFalse("Precondition: standardAttributesMap should not be empty", map.isEmpty()); + + dispatcher.cleanup(); + + assertTrue("standardAttributesMap should be empty after cleanup", map.isEmpty()); + } + + @Test + @SuppressWarnings("unchecked") + public void cleanupClearsCompoundRootAccessorCache() throws Exception { + initDispatcher(emptyMap()); + + Field field = CompoundRootAccessor.class.getDeclaredField("invalidMethods"); + field.setAccessible(true); + Map<Object, Boolean> invalidMethods = (Map<Object, Boolean>) field.get(null); + + // Seed with a dummy entry to ensure cleanup actually clears it + invalidMethods.put("testKey", Boolean.TRUE); + assertFalse("Precondition: invalidMethods should not be empty", invalidMethods.isEmpty()); + + dispatcher.cleanup(); + + assertTrue("invalidMethods should be empty after cleanup", invalidMethods.isEmpty()); + } + + @Test + public void cleanupClearsDefaultFileManagerFilesMap() throws Exception { + initDispatcher(emptyMap()); + + Field filesField = DefaultFileManager.class.getDeclaredField("files"); + filesField.setAccessible(true); + @SuppressWarnings("unchecked") + Map<String, Object> files = (Map<String, Object>) filesField.get(null); + + files.put("test-key", new Object()); + assertFalse("Precondition: files should not be empty", files.isEmpty()); + + dispatcher.cleanup(); + + assertTrue("DefaultFileManager.files should be empty after cleanup", files.isEmpty()); + } + + @Test + public void cleanupClearsDefaultFileManagerLazyCache() throws Exception { + initDispatcher(emptyMap()); + + Field lazyCacheField = DefaultFileManager.class.getDeclaredField("lazyMonitoredFilesCache"); + lazyCacheField.setAccessible(true); + @SuppressWarnings("unchecked") + List<URL> lazyCache = (List<URL>) lazyCacheField.get(null); + + lazyCache.add(new URL("file:///test")); + assertFalse("Precondition: lazyMonitoredFilesCache should not be empty", lazyCache.isEmpty()); + + dispatcher.cleanup(); + + assertTrue("DefaultFileManager.lazyMonitoredFilesCache should be empty after cleanup", + lazyCache.isEmpty()); + } + + @Test + @SuppressWarnings("unchecked") + public void cleanupClearsScopeInterceptorLocks() throws Exception { + initDispatcher(emptyMap()); + + Field locksField = ScopeInterceptor.class.getDeclaredField("locks"); + locksField.setAccessible(true); + Map<Object, Object> locks = (Map<Object, Object>) locksField.get(null); + + locks.put(new Object(), new Object()); + assertFalse("Precondition: locks should not be empty", locks.isEmpty()); + + dispatcher.cleanup(); + + assertTrue("ScopeInterceptor.locks should be empty after cleanup", locks.isEmpty()); + } + + @Test + public void cleanupClearsDispatcherListeners() throws Exception { + initDispatcher(emptyMap()); + + DispatcherListener listener = new DispatcherListener() { + @Override + public void dispatcherInitialized(Dispatcher du) {} + @Override + public void dispatcherDestroyed(Dispatcher du) {} + }; + Dispatcher.addDispatcherListener(listener); + + dispatcher.cleanup(); + + Field listenersField = Dispatcher.class.getDeclaredField("dispatcherListeners"); + listenersField.setAccessible(true); + List<?> listeners = (List<?>) listenersField.get(null); + assertTrue("dispatcherListeners should be empty after cleanup", listeners.isEmpty()); + } + + private Set<String> missing(Set<String> expected, Set<String> actual) { + Set<String> diff = new HashSet<>(expected); + diff.removeAll(actual); + return diff; + } +} diff --git a/plugins/json/src/main/java/org/apache/struts2/json/DefaultJSONWriter.java b/plugins/json/src/main/java/org/apache/struts2/json/DefaultJSONWriter.java index 16d13df3d..1ec8d017f 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/DefaultJSONWriter.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/DefaultJSONWriter.java @@ -74,6 +74,14 @@ public class DefaultJSONWriter implements JSONWriter { private static final ConcurrentMap<Class<?>, BeanInfo> BEAN_INFO_CACHE_IGNORE_HIERARCHY = new ConcurrentHashMap<>(); private static final ConcurrentMap<Class<?>, BeanInfo> BEAN_INFO_CACHE = new ConcurrentHashMap<>(); + /** + * Clears both BeanInfo caches to prevent classloader leaks on hot redeploy. + */ + public static void clearBeanInfoCaches() { + BEAN_INFO_CACHE_IGNORE_HIERARCHY.clear(); + BEAN_INFO_CACHE.clear(); + } + private final StringBuilder buf = new StringBuilder(); private Stack<Object> stack = new Stack<>(); private boolean ignoreHierarchy = true; diff --git a/plugins/json/src/main/java/org/apache/struts2/json/JSONCacheDestroyable.java b/plugins/json/src/main/java/org/apache/struts2/json/JSONCacheDestroyable.java new file mode 100644 index 000000000..102bd59c8 --- /dev/null +++ b/plugins/json/src/main/java/org/apache/struts2/json/JSONCacheDestroyable.java @@ -0,0 +1,35 @@ +/* + * 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.struts2.json; + +import org.apache.struts2.dispatcher.InternalDestroyable; + +/** + * WW-5537: Clears JSON plugin's static BeanInfo caches when the Dispatcher is + * destroyed, preventing classloader leaks during hot redeployment. + * + * @since 6.9.0 + */ +public class JSONCacheDestroyable implements InternalDestroyable { + + @Override + public void destroy() { + DefaultJSONWriter.clearBeanInfoCaches(); + } +} diff --git a/plugins/json/src/main/resources/struts-plugin.xml b/plugins/json/src/main/resources/struts-plugin.xml index 1291246a7..1f73b9289 100644 --- a/plugins/json/src/main/resources/struts-plugin.xml +++ b/plugins/json/src/main/resources/struts-plugin.xml @@ -29,6 +29,8 @@ <constant name="struts.json.writer" value="struts"/> <!-- TODO: Make DefaultJSONWriter thread-safe to remove "prototype"s --> <bean class="org.apache.struts2.json.JSONUtil" scope="prototype"/> + <bean type="org.apache.struts2.dispatcher.InternalDestroyable" name="jsonCache" + class="org.apache.struts2.json.JSONCacheDestroyable"/> <package name="json-default" extends="struts-default">
