This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch feature/WW-3576-session-map-staleness in repository https://gitbox.apache.org/repos/asf/struts.git
commit e6526359f8f926f1f352d8c9a469f2b4654ff441 Author: Lukasz Lenart <[email protected]> AuthorDate: Sun Oct 18 08:56:59 2026 +0200 WW-3576 fix(core): read SessionMap's session once per call and drop the interned-id lock Co-Authored-By: Claude Opus 5.5 <[email protected]> --- .../org/apache/struts2/dispatcher/SessionMap.java | 114 ++++++++++----------- .../apache/struts2/dispatcher/SessionMapTest.java | 39 +++++++ 2 files changed, 95 insertions(+), 58 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..54318391b 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,15 +62,14 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa * Invalidate the http session. */ public void invalidate() { - if (session == null) { + HttpSession current = session; + if (current == null) { return; } - synchronized (session.getId().intern()) { - session.invalidate(); - session = null; - entries = null; - } + current.invalidate(); + session = null; + entries = null; } /** @@ -79,18 +78,16 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public void clear() { - if (session == null) { + HttpSession current = session; + if (current == null) { return; } - synchronized (session.getId().intern()) { - entries = null; - final Enumeration<String> attributeNamesEnum = session.getAttributeNames(); - while (attributeNamesEnum.hasMoreElements()) { - session.removeAttribute(attributeNamesEnum.nextElement()); - } + entries = null; + final Enumeration<String> attributeNamesEnum = current.getAttributeNames(); + while (attributeNamesEnum.hasMoreElements()) { + current.removeAttribute(attributeNamesEnum.nextElement()); } - } /** @@ -100,32 +97,33 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public Set<Entry<String, Object>> entrySet() { - if (session == null) { + HttpSession current = session; + if (current == null) { return Collections.emptySet(); } - synchronized (session.getId().intern()) { - if (entries == null) { - entries = new HashSet<>(); + Set<Entry<String, Object>> result = entries; + if (result == null) { + result = new HashSet<>(); - final Enumeration<String> enumeration = session.getAttributeNames(); + final Enumeration<String> enumeration = current.getAttributeNames(); - while (enumeration.hasMoreElements()) { - final String key = enumeration.nextElement(); - final Object value = session.getAttribute(key); - entries.add(new StringObjectEntry(key, value) { - @Override - public Object setValue(final Object obj) { - session.setAttribute(key, obj); + while (enumeration.hasMoreElements()) { + final String key = enumeration.nextElement(); + final Object value = current.getAttribute(key); + result.add(new StringObjectEntry(key, value) { + @Override + public Object setValue(final Object obj) { + current.setAttribute(key, obj); - return value; - } - }); - } + return value; + } + }); } + entries = result; } - return entries; + return result; } /** @@ -139,13 +137,12 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public Object get(final Object key) { - if (session == null) { + HttpSession current = session; + if (current == null) { return null; } - synchronized (session.getId().intern()) { - return session.getAttribute(key != null ? key.toString() : null); - } + return current.getAttribute(key != null ? key.toString() : null); } /** @@ -157,17 +154,20 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public Object put(final String key, final Object value) { - synchronized (this) { - if (session == null) { - session = request.getSession(true); + HttpSession current = session; + if (current == null) { + synchronized (this) { + if (session == null) { + session = request.getSession(true); + } + current = session; } } - synchronized (session.getId().intern()) { - final Object oldValue = get(key); - entries = null; - session.setAttribute(key, value); - return oldValue; - } + + final Object oldValue = get(key); + entries = null; + current.setAttribute(key, value); + return oldValue; } /** @@ -181,19 +181,18 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public Object remove(final Object key) { - if (session == null) { + HttpSession current = session; + if (current == null) { return null; } - synchronized (session.getId().intern()) { - entries = null; + entries = null; - final String keyAsString = (key != null ? key.toString() : null); - final Object value = get(keyAsString); - session.removeAttribute(keyAsString); + final String keyAsString = (key != null ? key.toString() : null); + final Object value = get(keyAsString); + current.removeAttribute(keyAsString); - return value; - } + return value; } @@ -208,13 +207,12 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public boolean containsKey(final Object key) { - if (session == null) { + HttpSession current = session; + if (current == null) { return false; } - synchronized (session.getId().intern()) { - final String keyAsString = (key != null ? key.toString() : null); - return (session.getAttribute(keyAsString) != null); - } + final String keyAsString = (key != null ? key.toString() : null); + return (current.getAttribute(keyAsString) != null); } } diff --git a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java index b15f8a308..3edc9c380 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java @@ -24,11 +24,14 @@ import java.util.Enumeration; import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicReference; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpSession; import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpSession; import junit.framework.TestCase; @@ -277,6 +280,42 @@ public class SessionMapTest extends TestCase { sessionMock.verify(); } + public void testGetConcurrentWithInvalidateNeverThrows() throws Exception { + CountDownLatch invalidating = new CountDownLatch(1); + CountDownLatch release = new CountDownLatch(1); + MockHttpSession session = new MockHttpSession() { + @Override + public void invalidate() { + invalidating.countDown(); + try { release.await(); } catch (InterruptedException e) { Thread.currentThread().interrupt(); } + super.invalidate(); + } + }; + session.setAttribute("key", "value"); + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setSession(session); + SessionMap sessionMap = new SessionMap(request); + AtomicReference<Throwable> failure = new AtomicReference<>(); + + Thread invalidator = new Thread(sessionMap::invalidate); + invalidator.start(); + invalidating.await(); + Thread reader = new Thread(() -> { + try { sessionMap.get("key"); } catch (Throwable t) { failure.set(t); } + }); + reader.start(); + long deadline = System.currentTimeMillis() + 5000; + while (reader.getState() != Thread.State.BLOCKED && reader.getState() != Thread.State.TERMINATED) { + assertTrue("reader neither blocked nor finished", System.currentTimeMillis() < deadline); + Thread.sleep(1); + } + release.countDown(); + invalidator.join(5000); + reader.join(5000); + + assertNull("get() concurrent with invalidate() threw", failure.get()); + } + @Override protected void setUp() throws Exception { sessionMock = new Mock(HttpSession.class);
