You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@struts.apache.org by lu...@apache.org on 2026/02/11 17:39:26 UTC
(struts) 01/01: fix(dispatcher): WW-3576 make SessionMap thread-safe
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 <lu...@apache.org>
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 <no...@anthropic.com>
---
.../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());
+ }
+ }
+}