This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch fix/WW-3576-session-map-thread-safety in repository https://gitbox.apache.org/repos/asf/struts.git
commit 691ee8168906b81996cefefa6be66ff4e5dffe2e Author: Lukasz Lenart <[email protected]> AuthorDate: Wed Feb 11 19:39:06 2026 +0200 fix(dispatcher): WW-3576 make SessionMap thread-safe Apply volatile + local variable capture + double-check locking pattern to prevent race conditions between null checks and synchronized blocks. Changes: - Add volatile modifier to session field for visibility across threads - Capture session reference in local variable before null check - Add double-check inside synchronized block after acquiring lock - Add concurrent test cases to verify thread-safety Fixes https://issues.apache.org/jira/browse/WW-3576 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --- .../org/apache/struts2/dispatcher/SessionMap.java | 61 ++- .../dispatcher/SessionMapConcurrencyTest.java | 465 +++++++++++++++++++++ 2 files changed, 508 insertions(+), 18 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java index 76af042be..ef8c30593 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -39,7 +39,7 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa @Serial private static final long serialVersionUID = 4678843241638046854L; - protected HttpSession session; + protected volatile HttpSession session; protected Set<Entry<String, Object>> entries; protected HttpServletRequest request; @@ -62,11 +62,15 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa * Invalidate the http session. */ public void invalidate() { - if (session == null) { + HttpSession localSession = session; + if (localSession == null) { return; } - synchronized (session.getId().intern()) { + synchronized (localSession.getId().intern()) { + if (session == null) { + return; + } session.invalidate(); session = null; entries = null; @@ -79,18 +83,21 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public void clear() { - if (session == null) { + HttpSession localSession = session; + if (localSession == null) { return; } - synchronized (session.getId().intern()) { + synchronized (localSession.getId().intern()) { + if (session == null) { + return; + } entries = null; final Enumeration<String> attributeNamesEnum = session.getAttributeNames(); while (attributeNamesEnum.hasMoreElements()) { session.removeAttribute(attributeNamesEnum.nextElement()); } } - } /** @@ -100,11 +107,15 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public Set<Entry<String, Object>> entrySet() { - if (session == null) { + HttpSession localSession = session; + if (localSession == null) { return Collections.emptySet(); } - synchronized (session.getId().intern()) { + synchronized (localSession.getId().intern()) { + if (session == null) { + return Collections.emptySet(); + } if (entries == null) { entries = new HashSet<>(); @@ -132,18 +143,22 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa * Returns the session attribute associated with the given key or <tt>null</tt> if it doesn't exist. * * <b>Note:</b> Must use the same signature as {@link java.util.AbstractMap#get(java.lang.Object)} to ensure the - * expected specialized behaviour is performed here (and not the generic ancestor behaviour). + * expected specialized behaviour is performed here (and not the generic ancestor behaviour). * * @param key the name of the session attribute. * @return the session attribute or <tt>null</tt> if it doesn't exist. */ @Override public Object get(final Object key) { - if (session == null) { + HttpSession localSession = session; + if (localSession == null) { return null; } - synchronized (session.getId().intern()) { + synchronized (localSession.getId().intern()) { + if (session == null) { + return null; + } return session.getAttribute(key != null ? key.toString() : null); } } @@ -157,12 +172,14 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public Object put(final String key, final Object value) { + HttpSession localSession; synchronized (this) { if (session == null) { session = request.getSession(true); } + localSession = session; } - synchronized (session.getId().intern()) { + synchronized (localSession.getId().intern()) { final Object oldValue = get(key); entries = null; session.setAttribute(key, value); @@ -174,18 +191,22 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa * Removes the specified session attribute. * * <b>Note:</b> Must use the same signature as {@link java.util.AbstractMap#remove(java.lang.Object)} to ensure the - * expected specialized behaviour is performed here (and not the generic ancestor behaviour). + * expected specialized behaviour is performed here (and not the generic ancestor behaviour). * * @param key the name of the attribute to remove. * @return the value that was removed or <tt>null</tt> if the value was not found (and hence, not removed). */ @Override public Object remove(final Object key) { - if (session == null) { + HttpSession localSession = session; + if (localSession == null) { return null; } - synchronized (session.getId().intern()) { + synchronized (localSession.getId().intern()) { + if (session == null) { + return null; + } entries = null; final String keyAsString = (key != null ? key.toString() : null); @@ -201,18 +222,22 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa * Checks if the specified session attribute with the given key exists. * * <b>Note:</b> Must use the same signature as {@link java.util.AbstractMap#containsKey(java.lang.Object)} to ensure the - * expected specialized behaviour is performed here (and not the generic ancestor behaviour). + * expected specialized behaviour is performed here (and not the generic ancestor behaviour). * * @param key the name of the session attribute. * @return <tt>true</tt> if the session attribute exits or <tt>false</tt> if it doesn't exist. */ @Override public boolean containsKey(final Object key) { - if (session == null) { + HttpSession localSession = session; + if (localSession == null) { return false; } - synchronized (session.getId().intern()) { + synchronized (localSession.getId().intern()) { + if (session == null) { + return false; + } final String keyAsString = (key != null ? key.toString() : null); return (session.getAttribute(keyAsString) != null); } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapConcurrencyTest.java b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapConcurrencyTest.java new file mode 100644 index 000000000..9711ef14e --- /dev/null +++ b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapConcurrencyTest.java @@ -0,0 +1,465 @@ +/* + * 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.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpSession; + +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Concurrent tests for SessionMap to verify thread-safety (WW-3576). + * <p> + * These tests verify that the double-check locking pattern with volatile + * correctly prevents race conditions between null checks and synchronized blocks. + */ +class SessionMapConcurrencyTest { + + private MockHttpServletRequest request; + private MockHttpSession mockSession; + + @BeforeEach + void setUp() { + mockSession = new MockHttpSession(); + request = new MockHttpServletRequest(); + request.setSession(mockSession); + } + + /** + * Tests that concurrent invalidate() calls do not cause NullPointerException. + * This was the primary issue reported in WW-3576. + */ + @Test + void concurrentInvalidateDoesNotThrowNPE() throws InterruptedException { + int threadCount = 10; + int iterations = 100; + AtomicReference<Throwable> error = new AtomicReference<>(); + + for (int i = 0; i < iterations; i++) { + MockHttpSession session = new MockHttpSession(); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setSession(session); + SessionMap sessionMap = new SessionMap(req); + + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(threadCount); + + for (int t = 0; t < threadCount; t++) { + new Thread(() -> { + try { + startLatch.await(); + sessionMap.invalidate(); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + } + + startLatch.countDown(); + doneLatch.await(5, TimeUnit.SECONDS); + + if (error.get() != null) { + throw new AssertionError("Concurrent invalidate() threw exception", error.get()); + } + } + } + + /** + * Tests that concurrent get() and invalidate() do not cause NullPointerException. + */ + @Test + void concurrentGetAndInvalidateDoesNotThrowNPE() throws InterruptedException { + int iterations = 100; + AtomicReference<Throwable> error = new AtomicReference<>(); + + for (int i = 0; i < iterations; i++) { + MockHttpSession session = new MockHttpSession(); + session.setAttribute("key", "value"); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setSession(session); + SessionMap sessionMap = new SessionMap(req); + + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(2); + + // Thread 1: repeatedly calls get() + new Thread(() -> { + try { + startLatch.await(); + for (int j = 0; j < 100; j++) { + sessionMap.get("key"); + } + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + // Thread 2: calls invalidate() + new Thread(() -> { + try { + startLatch.await(); + sessionMap.invalidate(); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + startLatch.countDown(); + doneLatch.await(5, TimeUnit.SECONDS); + + if (error.get() != null) { + throw new AssertionError("Concurrent get/invalidate threw exception", error.get()); + } + } + } + + /** + * Tests that concurrent put() and invalidate() do not cause NullPointerException. + */ + @Test + void concurrentPutAndInvalidateDoesNotThrowNPE() throws InterruptedException { + int iterations = 100; + AtomicReference<Throwable> error = new AtomicReference<>(); + + for (int i = 0; i < iterations; i++) { + MockHttpSession session = new MockHttpSession(); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setSession(session); + SessionMap sessionMap = new SessionMap(req); + + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(2); + + // Thread 1: repeatedly calls put() + new Thread(() -> { + try { + startLatch.await(); + for (int j = 0; j < 100; j++) { + sessionMap.put("key" + j, "value" + j); + } + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + // Thread 2: calls invalidate() + new Thread(() -> { + try { + startLatch.await(); + Thread.sleep(1); // slight delay to let put() start + sessionMap.invalidate(); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + startLatch.countDown(); + doneLatch.await(5, TimeUnit.SECONDS); + + if (error.get() != null) { + throw new AssertionError("Concurrent put/invalidate threw exception", error.get()); + } + } + } + + /** + * Tests that concurrent clear() and invalidate() do not cause NullPointerException. + */ + @Test + void concurrentClearAndInvalidateDoesNotThrowNPE() throws InterruptedException { + int iterations = 100; + AtomicReference<Throwable> error = new AtomicReference<>(); + + for (int i = 0; i < iterations; i++) { + MockHttpSession session = new MockHttpSession(); + session.setAttribute("key1", "value1"); + session.setAttribute("key2", "value2"); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setSession(session); + SessionMap sessionMap = new SessionMap(req); + + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(2); + + // Thread 1: calls clear() + new Thread(() -> { + try { + startLatch.await(); + sessionMap.clear(); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + // Thread 2: calls invalidate() + new Thread(() -> { + try { + startLatch.await(); + sessionMap.invalidate(); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + startLatch.countDown(); + doneLatch.await(5, TimeUnit.SECONDS); + + if (error.get() != null) { + throw new AssertionError("Concurrent clear/invalidate threw exception", error.get()); + } + } + } + + /** + * Tests that concurrent remove() and invalidate() do not cause NullPointerException. + */ + @Test + void concurrentRemoveAndInvalidateDoesNotThrowNPE() throws InterruptedException { + int iterations = 100; + AtomicReference<Throwable> error = new AtomicReference<>(); + + for (int i = 0; i < iterations; i++) { + MockHttpSession session = new MockHttpSession(); + session.setAttribute("key", "value"); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setSession(session); + SessionMap sessionMap = new SessionMap(req); + + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(2); + + // Thread 1: calls remove() + new Thread(() -> { + try { + startLatch.await(); + sessionMap.remove("key"); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + // Thread 2: calls invalidate() + new Thread(() -> { + try { + startLatch.await(); + sessionMap.invalidate(); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + startLatch.countDown(); + doneLatch.await(5, TimeUnit.SECONDS); + + if (error.get() != null) { + throw new AssertionError("Concurrent remove/invalidate threw exception", error.get()); + } + } + } + + /** + * Tests that concurrent containsKey() and invalidate() do not cause NullPointerException. + */ + @Test + void concurrentContainsKeyAndInvalidateDoesNotThrowNPE() throws InterruptedException { + int iterations = 100; + AtomicReference<Throwable> error = new AtomicReference<>(); + + for (int i = 0; i < iterations; i++) { + MockHttpSession session = new MockHttpSession(); + session.setAttribute("key", "value"); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setSession(session); + SessionMap sessionMap = new SessionMap(req); + + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(2); + + // Thread 1: repeatedly calls containsKey() + new Thread(() -> { + try { + startLatch.await(); + for (int j = 0; j < 100; j++) { + sessionMap.containsKey("key"); + } + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + // Thread 2: calls invalidate() + new Thread(() -> { + try { + startLatch.await(); + sessionMap.invalidate(); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + startLatch.countDown(); + doneLatch.await(5, TimeUnit.SECONDS); + + if (error.get() != null) { + throw new AssertionError("Concurrent containsKey/invalidate threw exception", error.get()); + } + } + } + + /** + * Tests that concurrent entrySet() and invalidate() do not cause NullPointerException. + */ + @Test + void concurrentEntrySetAndInvalidateDoesNotThrowNPE() throws InterruptedException { + int iterations = 100; + AtomicReference<Throwable> error = new AtomicReference<>(); + + for (int i = 0; i < iterations; i++) { + MockHttpSession session = new MockHttpSession(); + session.setAttribute("key", "value"); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setSession(session); + SessionMap sessionMap = new SessionMap(req); + + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(2); + + // Thread 1: repeatedly calls entrySet() + new Thread(() -> { + try { + startLatch.await(); + for (int j = 0; j < 100; j++) { + sessionMap.entrySet(); + } + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + // Thread 2: calls invalidate() + new Thread(() -> { + try { + startLatch.await(); + sessionMap.invalidate(); + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }).start(); + + startLatch.countDown(); + doneLatch.await(5, TimeUnit.SECONDS); + + if (error.get() != null) { + throw new AssertionError("Concurrent entrySet/invalidate threw exception", error.get()); + } + } + } + + /** + * Stress test with multiple operations happening concurrently. + */ + @Test + void stressTestMixedOperations() throws InterruptedException { + int threadCount = 20; + int operationsPerThread = 50; + AtomicReference<Throwable> error = new AtomicReference<>(); + AtomicInteger completedOperations = new AtomicInteger(0); + + MockHttpSession session = new MockHttpSession(); + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setSession(session); + SessionMap sessionMap = new SessionMap(req); + + ExecutorService executor = Executors.newFixedThreadPool(threadCount); + CountDownLatch startLatch = new CountDownLatch(1); + CountDownLatch doneLatch = new CountDownLatch(threadCount); + + for (int t = 0; t < threadCount; t++) { + final int threadId = t; + executor.submit(() -> { + try { + startLatch.await(); + for (int j = 0; j < operationsPerThread; j++) { + int operation = (threadId + j) % 6; + String key = "key" + threadId + "_" + j; + switch (operation) { + case 0 -> sessionMap.put(key, "value"); + case 1 -> sessionMap.get(key); + case 2 -> sessionMap.remove(key); + case 3 -> sessionMap.containsKey(key); + case 4 -> sessionMap.entrySet(); + case 5 -> sessionMap.clear(); + } + completedOperations.incrementAndGet(); + } + } catch (Throwable e) { + error.compareAndSet(null, e); + } finally { + doneLatch.countDown(); + } + }); + } + + startLatch.countDown(); + boolean finished = doneLatch.await(30, TimeUnit.SECONDS); + executor.shutdownNow(); + + assertThat(finished).as("All threads should complete within timeout").isTrue(); + if (error.get() != null) { + throw new AssertionError("Stress test threw exception after " + completedOperations.get() + " operations", error.get()); + } + } +}
