This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5537-classloader-leak-fixes in repository https://gitbox.apache.org/repos/asf/struts.git
commit cfda44b3b55004664897eb31dc06ae1388eee139 Author: Lukasz Lenart <[email protected]> AuthorDate: Mon Mar 23 09:54:09 2026 +0100 WW-5537 ContainerHolder: ThreadLocal with AtomicLong generation counter Co-Authored-By: Claude Opus 4.6 <[email protected]> --- .../apache/struts2/dispatcher/ContainerHolder.java | 53 ++++++++++++--- .../struts2/dispatcher/PrepareOperations.java | 1 + .../struts2/dispatcher/ContainerHolderTest.java | 78 ++++++++++++++++++++++ 3 files changed, 124 insertions(+), 8 deletions(-) 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 5e0b0ceb1..708d897f0 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/ContainerHolder.java @@ -20,28 +20,65 @@ package org.apache.struts2.dispatcher; import org.apache.struts2.inject.Container; +import java.util.concurrent.atomic.AtomicLong; + /** - * Simple class to hold Container instance per thread to minimise number of attempts - * to read configuration and build each time a new configuration. + * Per-thread cache for the Container instance, minimising repeated reads from + * {@link org.apache.struts2.config.ConfigurationManager}. * <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: Uses a ThreadLocal for per-request isolation with an AtomicLong generation + * counter for cross-thread invalidation during app undeploy. When + * {@link #invalidateAll()} is called, all threads see the updated generation on their + * next {@link #get()} and return {@code null}, forcing a fresh read from + * ConfigurationManager. This prevents classloader leaks caused by idle pool threads + * retaining stale Container references after hot redeployment. */ class ContainerHolder { - private static final ThreadLocal<Container> instance = new ThreadLocal<>(); + private static final ThreadLocal<CachedContainer> instance = new ThreadLocal<>(); + + /** + * Incremented on each {@link #invalidateAll()} call. Threads compare their cached + * generation against this value to detect staleness. + */ + private static final AtomicLong generation = new AtomicLong(); public static void store(Container newInstance) { - instance.set(newInstance); + instance.set(new CachedContainer(newInstance, generation.get())); } public static Container get() { - return instance.get(); + CachedContainer cached = instance.get(); + if (cached == null) { + return null; + } + if (cached.generation() != generation.get()) { + instance.remove(); + return null; + } + return cached.container(); } + /** + * Clears the current thread's cached container reference. + * Used for per-request cleanup. + */ public static void clear() { instance.remove(); } + /** + * Invalidates all threads' cached container references by advancing the generation + * counter. Each thread will detect the stale generation on its next {@link #get()} + * call and clear its own ThreadLocal. Also clears the calling thread immediately. + * <p> + * Used during application undeploy ({@link Dispatcher#cleanup()}) to ensure idle + * pool threads do not pin the webapp classloader via retained Container references. + */ + public static void invalidateAll() { + generation.incrementAndGet(); + instance.remove(); + } + + private record CachedContainer(Container container, long generation) {} } 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 09df2c15f..eb37b4812 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/test/java/org/apache/struts2/dispatcher/ContainerHolderTest.java b/core/src/test/java/org/apache/struts2/dispatcher/ContainerHolderTest.java new file mode 100644 index 000000000..a11717dbc --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/ContainerHolderTest.java @@ -0,0 +1,78 @@ +/* + * 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 org.apache.struts2.inject.Container; +import org.junit.After; +import org.junit.Test; + +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.Mockito.mock; + +public class ContainerHolderTest { + + @After + public void tearDown() { + ContainerHolder.clear(); + } + + @Test + public void storeAndGet() { + Container c = mock(Container.class); + ContainerHolder.store(c); + assertThat(ContainerHolder.get()).isSameAs(c); + } + + @Test + public void clearRemovesCurrentThread() { + ContainerHolder.store(mock(Container.class)); + ContainerHolder.clear(); + assertThat(ContainerHolder.get()).isNull(); + } + + @Test + public void invalidateAllMakesOtherThreadsSeeNull() throws Exception { + Container c = mock(Container.class); + + // Another thread stores a container + Thread t = new Thread(() -> ContainerHolder.store(c)); + t.start(); + t.join(); + + // Invalidate on main thread + ContainerHolder.invalidateAll(); + + // Other thread's cached value should now be stale + AtomicReference<Container> otherThreadResult = new AtomicReference<>(); + Thread t2 = new Thread(() -> otherThreadResult.set(ContainerHolder.get())); + t2.start(); + t2.join(); + + assertThat(otherThreadResult.get()).isNull(); + } + + @Test + public void invalidateAllClearsCallingThread() { + ContainerHolder.store(mock(Container.class)); + ContainerHolder.invalidateAll(); + assertThat(ContainerHolder.get()).isNull(); + } +}
