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 eb37b4812d..2cc8871a1c 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/PrepareOperations.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/PrepareOperations.java @@ -75,6 +75,7 @@ public void cleanupRequest(final HttpServletRequest request) { try { dispatcher.cleanUpRequest(request); } finally { + detachSessionMaps(request); ActionContext.clear(); Dispatcher.clearInstance(); ContainerHolder.clear(); @@ -83,6 +84,20 @@ public void cleanupRequest(final HttpServletRequest request) { }); } + private static void detachSessionMaps(HttpServletRequest request) { + if (!request.isAsyncStarted()) { + SessionMap.detachAll(request); + detachSessionMap(); + } + } + + private static void detachSessionMap() { + ActionContext context = ActionContext.getContext(); + if (context != null && context.getSession() instanceof SessionMap sessionMap) { + sessionMap.detach(); + } + } + /** * Creates the action context and initializes the thread local * 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 76af042bef..72f767287e 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -28,6 +28,8 @@ import java.util.Enumeration; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.ConcurrentLinkedQueue; +import java.util.Queue; /** * A simple implementation of the {@link java.util.Map} interface to handle a collection of HTTP session @@ -39,10 +41,19 @@ public class SessionMap extends AbstractMap implements Serializa @Serial private static final long serialVersionUID = 4678843241638046854L; - protected HttpSession session; + private static final String REQUEST_MAPS = SessionMap.class.getName() + ".maps"; + + // volatile only publishes the pinned reference (HttpSession is thread-safe per the Servlet spec); + // non-transient as before, so subclass serialization is unchanged + @SuppressWarnings({"java:S3077", "java:S1948"}) + protected volatile HttpSession session; + /** + * @deprecated no longer read or written; {@link #entrySet()} builds a fresh set on every call. + */ + @Deprecated(since = "7.5.0", forRemoval = true) protected Set> entries; protected HttpServletRequest request; - + private volatile boolean detached; /** * Creates a new session map given a http servlet request. Note, the enumeration of request @@ -56,76 +67,109 @@ public SessionMap(final HttpServletRequest request) { // "execAndWait" interceptor this.request = request; this.session = request.getSession(false); + register(request); + } + + void detach() { + detached = true; + } + + private void register(HttpServletRequest request) { + RequestMaps registered = request.getAttribute(REQUEST_MAPS) instanceof RequestMaps existing ? existing : null; + if (registered == null) { + registered = new RequestMaps(); + request.setAttribute(REQUEST_MAPS, registered); + } + registered.maps.add(this); + } + + static void detachAll(HttpServletRequest request) { + if (request.getAttribute(REQUEST_MAPS) instanceof RequestMaps registered) { + registered.maps.forEach(SessionMap::detach); + request.removeAttribute(REQUEST_MAPS); + } + } + + private static final class RequestMaps { + private final Queue maps = new ConcurrentLinkedQueue<>(); + + @Override + public String toString() { + return "RequestMaps"; + } } /** * 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; + try { + current.invalidate(); + } catch (IllegalStateException e) { + // already invalidated elsewhere } + unpin(current); } /** - * Removes all attributes from the session as well as clears entries in this - * map. + * Removes all attributes from the session. */ @Override public void clear() { - if (session == null) { + HttpSession current = session; + if (current == null) { return; } - synchronized (session.getId().intern()) { - entries = null; - final Enumeration attributeNamesEnum = session.getAttributeNames(); + try { + final Enumeration attributeNamesEnum = current.getAttributeNames(); while (attributeNamesEnum.hasMoreElements()) { - session.removeAttribute(attributeNamesEnum.nextElement()); + current.removeAttribute(attributeNamesEnum.nextElement()); } + } catch (IllegalStateException e) { + unpin(current); } - } /** - * Returns a Set of attributes from the http session. + * Returns a Set of attributes from the http session, built fresh on every call. * * @return a Set of attributes from the http session. */ @Override public Set> entrySet() { - if (session == null) { + HttpSession current = session; + if (current == null) { return Collections.emptySet(); } - synchronized (session.getId().intern()) { - if (entries == null) { - entries = new HashSet<>(); + final Set> result = new HashSet<>(); + try { + final Enumeration enumeration = current.getAttributeNames(); - final Enumeration enumeration = session.getAttributeNames(); + 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); - 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); - - return value; - } - }); - } + return value; + } + }); } + } catch (IllegalStateException e) { + unpin(current); + return Collections.emptySet(); } - return entries; + return result; } /** @@ -139,12 +183,16 @@ public Object setValue(final Object obj) { */ @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); + try { + return current.getAttribute(key != null ? key.toString() : null); + } catch (IllegalStateException e) { + unpin(current); + return null; } } @@ -157,16 +205,42 @@ public Object get(final Object key) { */ @Override public Object put(final String key, final Object value) { + HttpSession current = session; + if (current == null) { + current = pin(); + } + + final Object oldValue = get(key); + try { + current.setAttribute(key, value); + } catch (IllegalStateException e) { + unpin(current); + pin().setAttribute(key, value); + return null; + } + return oldValue; + } + + private HttpSession pin() { synchronized (this) { - if (session == null) { - session = request.getSession(true); + HttpSession current = session; + if (current == null) { + if (detached) { + throw new IllegalStateException("The request of this session map has ended and its session is gone; " + + "it cannot resolve a new session"); + } + current = request.getSession(true); + session = current; } + return current; } - synchronized (session.getId().intern()) { - final Object oldValue = get(key); - entries = null; - session.setAttribute(key, value); - return oldValue; + } + + private void unpin(final HttpSession stale) { + synchronized (this) { + if (session == stale) { + session = null; + } } } @@ -181,19 +255,21 @@ public Object put(final String key, final Object value) { */ @Override public Object remove(final Object key) { - if (session == null) { + HttpSession current = session; + if (current == null) { return null; } - synchronized (session.getId().intern()) { - entries = null; - - final String keyAsString = (key != null ? key.toString() : null); - final Object value = get(keyAsString); - session.removeAttribute(keyAsString); - - return value; + final String keyAsString = (key != null ? key.toString() : null); + final Object value = get(keyAsString); + try { + current.removeAttribute(keyAsString); + } catch (IllegalStateException e) { + unpin(current); + return null; } + + return value; } @@ -208,13 +284,17 @@ public Object remove(final Object key) { */ @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); + try { + return current.getAttribute(keyAsString) != null; + } catch (IllegalStateException e) { + unpin(current); + return false; } } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java b/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java index adab43dc6e..f83a67de46 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java @@ -45,7 +45,7 @@ public class StrutsBackgroundProcess implements BackgroundProcess, Serializable protected transient Exception exception; protected String result; - protected boolean done; + protected volatile boolean done; /** * Constructs a background process diff --git a/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java b/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java index abc060c4c2..2b320cbe01 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java @@ -28,6 +28,8 @@ import jakarta.servlet.http.HttpServletRequest; import java.util.stream.IntStream; +import static org.junit.Assert.assertThrows; + public class PrepareOperationsTest extends StrutsInternalTestCase { public void testCreateActionContextWhenRequestHasOne() { HttpServletRequest req = new MockHttpServletRequest(); @@ -60,6 +62,123 @@ public void testRequestCleanup() { assertNull(ContainerHolder.get()); } + public void testRequestCleanupDetachesTheContextSessionMap() { + MockHttpServletRequest req = new MockHttpServletRequest(); + SessionMap sessionMap = new SessionMap(req); + ActionContext.getContext().withSession(sessionMap); + PrepareOperations prepare = new PrepareOperations(dispatcher); + prepare.trackRecursion(req); + + prepare.cleanupRequest(req); + + assertThrows(IllegalStateException.class, () -> sessionMap.put("key", "value")); + assertNull(req.getSession(false)); + } + + public void testRequestCleanupDetachesTheHelperSessionMapAfterCreateSession() { + MockHttpServletRequest req = new MockHttpServletRequest(); + SessionMap original = new SessionMap(req); + ActionContext context = ActionContext.getContext() + .withSession(original) + .with(DispatcherConstants.SESSION, original) + .withSession(new SessionMap(req)); + PrepareOperations prepare = new PrepareOperations(dispatcher); + prepare.trackRecursion(req); + + prepare.cleanupRequest(req); + + SessionMap helper = (SessionMap) context.get(DispatcherConstants.SESSION); + assertThrows(IllegalStateException.class, () -> helper.put("key", "value")); + assertNull(req.getSession(false)); + } + + public void testRequestCleanupDetachesEveryRequestSessionMap() { + MockHttpServletRequest req = new MockHttpServletRequest(); + SessionMap first = new SessionMap(req); + SessionMap second = new SessionMap(req); + PrepareOperations prepare = new PrepareOperations(dispatcher); + prepare.trackRecursion(req); + + prepare.cleanupRequest(req); + + assertThrows(IllegalStateException.class, () -> first.put("key", "value")); + assertThrows(IllegalStateException.class, () -> second.put("key", "value")); + assertNull(req.getSession(false)); + } + + public void testRequestCleanupKeepsSessionMapsAttachedWhileAsyncStarted() { + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setAsyncStarted(true); + SessionMap sessionMap = new SessionMap(req); + PrepareOperations prepare = new PrepareOperations(dispatcher); + prepare.trackRecursion(req); + + prepare.cleanupRequest(req); + + sessionMap.put("key", "value"); + assertNotNull(req.getSession(false)); + } + + public void testAsyncDispatchCleanupDetachesTheSessionMapsKeptAttached() { + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setAsyncStarted(true); + SessionMap sessionMap = new SessionMap(req); + PrepareOperations prepare = new PrepareOperations(dispatcher); + prepare.trackRecursion(req); + prepare.cleanupRequest(req); + req.setAsyncStarted(false); + prepare.trackRecursion(req); + + prepare.cleanupRequest(req); + + assertThrows(IllegalStateException.class, () -> sessionMap.put("key", "value")); + assertNull(req.getSession(false)); + } + + public void testRequestCleanupKeepsTheBoundSessionMapAttachedWhileAsyncStarted() { + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setAsyncStarted(true); + SessionMap sessionMap = new SessionMap(req); + ActionContext.getContext().withSession(sessionMap); + PrepareOperations prepare = new PrepareOperations(dispatcher); + prepare.trackRecursion(req); + + prepare.cleanupRequest(req); + + sessionMap.put("key", "value"); + assertNotNull(req.getSession(false)); + } + + public void testBoundSessionMapIsDetachedByTheAsyncDispatchCleanup() { + MockHttpServletRequest req = new MockHttpServletRequest(); + req.setAsyncStarted(true); + SessionMap sessionMap = new SessionMap(req); + ActionContext.getContext().withSession(sessionMap); + PrepareOperations prepare = new PrepareOperations(dispatcher); + prepare.trackRecursion(req); + prepare.cleanupRequest(req); + req.setAsyncStarted(false); + ActionContext.of().withSession(sessionMap).bind(); + prepare.trackRecursion(req); + + prepare.cleanupRequest(req); + + assertThrows(IllegalStateException.class, () -> sessionMap.put("key", "value")); + assertNull(req.getSession(false)); + } + + public void testRequestCleanupWithoutActionContext() { + HttpServletRequest req = new MockHttpServletRequest(); + PrepareOperations prepare = new PrepareOperations(dispatcher); + prepare.trackRecursion(req); + ActionContext.clear(); + + prepare.cleanupRequest(req); + + assertNull(Dispatcher.getInstance()); + assertNull(ContainerHolder.get()); + } + public void testWrappedRequestCleanup() { HttpServletRequest req = new MockHttpServletRequest(); PrepareOperations prepare = new PrepareOperations(dispatcher); 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 b15f8a308c..a925ca74db 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,15 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +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; @@ -38,6 +42,8 @@ import com.mockobjects.dynamic.Mock; import java.util.AbstractMap; +import static org.awaitility.Awaitility.await; + /** */ @@ -277,12 +283,196 @@ public void testRemoveResultInSessionAttributes() throws Exception { 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 failure = new AtomicReference<>(); + AtomicReference read = new AtomicReference<>(); + + Thread invalidator = new Thread(sessionMap::invalidate); + invalidator.start(); + invalidating.await(); + Thread reader = new Thread(() -> { + try { read.set(sessionMap.get("key")); } catch (Throwable t) { failure.set(t); } + }); + reader.start(); + await().atMost(5, TimeUnit.SECONDS) + .until(() -> reader.getState() == Thread.State.BLOCKED || reader.getState() == Thread.State.TERMINATED); + release.countDown(); + invalidator.join(5000); + reader.join(5000); + + assertFalse("reader did not finish", reader.isAlive()); + assertNull("get() concurrent with invalidate() threw: " + failure.get(), failure.get()); + assertEquals("value", read.get()); + } + + private MockHttpSession invalidatedSession; + private MockHttpServletRequest invalidatedRequest; + + private SessionMap sessionMapOverInvalidatedSession() { + invalidatedRequest = new MockHttpServletRequest(); + invalidatedSession = new MockHttpSession(); + invalidatedSession.setAttribute("key", "value"); + invalidatedRequest.setSession(invalidatedSession); + SessionMap sessionMap = new SessionMap(invalidatedRequest); + invalidatedSession.invalidate(); + return sessionMap; + } + + private void assertPutLandsInNewSession(SessionMap sessionMap) { + sessionMap.put("other", "fresh"); + + HttpSession current = invalidatedRequest.getSession(false); + assertNotNull(current); + assertNotSame(invalidatedSession, current); + assertEquals("fresh", current.getAttribute("other")); + } + + public void testGetOnSessionInvalidatedElsewhere() { + SessionMap sessionMap = sessionMapOverInvalidatedSession(); + assertNull(sessionMap.get("key")); + assertPutLandsInNewSession(sessionMap); + } + + public void testContainsKeyOnSessionInvalidatedElsewhere() { + SessionMap sessionMap = sessionMapOverInvalidatedSession(); + assertFalse(sessionMap.containsKey("key")); + assertPutLandsInNewSession(sessionMap); + } + + public void testEntrySetOnSessionInvalidatedElsewhere() { + SessionMap sessionMap = sessionMapOverInvalidatedSession(); + assertTrue(sessionMap.entrySet().isEmpty()); + assertPutLandsInNewSession(sessionMap); + } + + public void testRemoveOnSessionInvalidatedElsewhere() { + SessionMap sessionMap = sessionMapOverInvalidatedSession(); + assertNull(sessionMap.remove("key")); + assertPutLandsInNewSession(sessionMap); + } + + public void testClearOnSessionInvalidatedElsewhere() { + SessionMap sessionMap = sessionMapOverInvalidatedSession(); + sessionMap.clear(); + assertPutLandsInNewSession(sessionMap); + } + + public void testInvalidateOnSessionInvalidatedElsewhere() { + SessionMap sessionMap = sessionMapOverInvalidatedSession(); + sessionMap.invalidate(); + assertPutLandsInNewSession(sessionMap); + } + + public void testPutAfterInvalidationElsewhereUsesANewSession() { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession session = new MockHttpSession(); + request.setSession(session); + SessionMap sessionMap = new SessionMap(request); + + session.invalidate(); + sessionMap.put("key", "value"); + + HttpSession current = request.getSession(false); + assertNotNull(current); + assertNotSame(session, current); + assertEquals("value", current.getAttribute("key")); + } + + public void testAttributeSetDirectlyIsVisibleThroughEntrySet() { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession session = new MockHttpSession(); + request.setSession(session); + SessionMap sessionMap = new SessionMap(request); + assertTrue(sessionMap.entrySet().isEmpty()); + + session.setAttribute("key", "value"); + + assertEquals(1, sessionMap.entrySet().size()); + } + + public void testPinnedSessionNeverTouchesTheRequestAgain() { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setSession(new MockHttpSession()); + SessionMap sessionMap = new SessionMap(request); + request.close(); + + sessionMap.put("key", "value"); + assertEquals("value", sessionMap.get("key")); + assertEquals(1, sessionMap.entrySet().size()); + assertEquals("value", sessionMap.remove("key")); + } + + public void testDetachedPutAfterInvalidationNeverReachesTheRecycledRequest() { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession pinned = new MockHttpSession(); + request.setSession(pinned); + SessionMap sessionMap = new SessionMap(request); + pinned.invalidate(); + MockHttpSession otherUsers = new MockHttpSession(); + request.setSession(otherUsers); + sessionMap.detach(); + + try { + sessionMap.put("key", "value"); + fail("a detached map with no live session must not resolve a session through its request"); + } catch (IllegalStateException expected) { + assertFalse(expected.getMessage().contains("value")); + } + assertFalse(otherUsers.getAttributeNames().hasMoreElements()); + assertSame(otherUsers, request.getSession(false)); + } + + public void testDetachedNeverPinnedPutThrows() { + MockHttpServletRequest request = new MockHttpServletRequest(); + SessionMap sessionMap = new SessionMap(request); + sessionMap.detach(); + + try { + sessionMap.put("key", "value"); + fail("a detached map with no pinned session must not create one through its request"); + } catch (IllegalStateException expected) { + assertNull(request.getSession(false)); + } + } + + public void testDetachedMapKeepsWorkingOnItsLivePinnedSession() { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession pinned = new MockHttpSession(); + request.setSession(pinned); + SessionMap sessionMap = new SessionMap(request); + sessionMap.detach(); + + sessionMap.put("key", "value"); + + assertEquals("value", pinned.getAttribute("key")); + assertEquals("value", sessionMap.get("key")); + assertTrue(sessionMap.containsKey("key")); + assertEquals(1, sessionMap.entrySet().size()); + assertEquals("value", sessionMap.remove("key")); + } + @Override protected void setUp() throws Exception { sessionMock = new Mock(HttpSession.class); sessionMock.matchAndReturn("getId", "1"); requestMock = new Mock(HttpServletRequest.class); requestMock.matchAndReturn("getSession", new Constraint[]{new IsEqual(Boolean.FALSE)}, sessionMock.proxy()); + requestMock.matchAndReturn("getAttribute", new Constraint[]{new IsAnything()}, null); + requestMock.matchAndReturn("setAttribute", new Constraint[]{new IsAnything(), new IsAnything()}, null); } diff --git a/core/src/test/java/org/apache/struts2/interceptor/CreateSessionInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/CreateSessionInterceptorTest.java index 43a098a0dd..4e1deae857 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/CreateSessionInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/CreateSessionInterceptorTest.java @@ -25,6 +25,7 @@ import org.jmock.Mock; import org.jmock.core.constraint.IsEqual; import org.jmock.core.matcher.InvokeOnceMatcher; +import org.jmock.core.stub.ReturnStub; import jakarta.servlet.http.HttpServletRequest; @@ -48,6 +49,8 @@ public void testCreateSession() throws Exception { httpServletRequestMock.expects(new InvokeOnceMatcher()).method("getSession").with(new IsEqual(Boolean.FALSE)); httpServletRequestMock.expects(new InvokeOnceMatcher()).method("getSession").with(new IsEqual(Boolean.TRUE)); httpServletRequestMock.expects(new InvokeOnceMatcher()).method("getSession").with(new IsEqual(Boolean.FALSE)); + httpServletRequestMock.stubs().method("getAttribute").will(new ReturnStub(null)); + httpServletRequestMock.stubs().method("setAttribute"); HttpServletRequest request = (HttpServletRequest) httpServletRequestMock.proxy(); ServletActionContext.setRequest(request); diff --git a/core/src/test/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptorTest.java index 01977f1ce1..b9caaa6a5c 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/debugging/DebuggingInterceptorTest.java @@ -68,7 +68,11 @@ public void debugXml() throws Exception { - + + + class org.apache.struts2.dispatcher.SessionMap$RequestMaps + + @@ -171,7 +175,11 @@ public void debugXmlWithConsole() throws Exception { &lt;debug&gt; &lt;parameters/&gt; &lt;context/&gt; - &lt;request/&gt; + &lt;request&gt; + &lt;org.apache.struts2.dispatcher.SessionMap.maps&gt; + &lt;class&gt;class org.apache.struts2.dispatcher.SessionMap$RequestMaps&lt;/class&gt; + &lt;/org.apache.struts2.dispatcher.SessionMap.maps&gt; + &lt;/request&gt; &lt;session/&gt; &lt;valueStack&gt; &lt;value&gt; diff --git a/docs/superpowers/plans/2026-10-18-WW-3576-session-map-staleness.md b/docs/superpowers/plans/2026-10-18-WW-3576-session-map-staleness.md new file mode 100644 index 0000000000..ac855a53f6 --- /dev/null +++ b/docs/superpowers/plans/2026-10-18-WW-3576-session-map-staleness.md @@ -0,0 +1,195 @@ +# WW-3576 SessionMap Staleness — Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Stop `SessionMap` from going stale (NPE race, `IllegalStateException` after invalidation). + +**Architecture:** The request `SessionMap` pins its session, reads it once per call into a local, treats `IllegalStateException` as "invalidated → no session", and drops the `entries` cache and the interned-id lock. + +**Tech Stack:** Java 17, Jakarta Servlet 6, JUnit 3 (`junit.framework.TestCase` / `StrutsInternalTestCase`), Spring `MockHttpServletRequest`/`MockHttpSession`, AssertJ, Log4j2 via `org.apache.struts2.test.LogCapture`. + +**Spec:** `docs/superpowers/specs/2026-10-18-WW-3576-session-map-staleness-design.md` + +## Global Constraints + +- Branch `feature/WW-3576-session-map-staleness`; every commit message starts `WW-3576 `; stage explicit paths only (never `git add -A`), never `git stash`. +- Tests are JUnit 3 in every file touched here: `testXxx()` names, no `@Test`. +- Build with `./mvnw` (no `mvn` on PATH); check Maven's exit code, not a grep of its output. +- Deprecations are `@Deprecated(since = "7.5.0", forRemoval = true)`; no reference in source to a ticket that is not filed. +- No null guards on required beans; no parameter reassignment; no boolean flag arguments; default to no comments. +- Never log session attribute values. + +## Review Focus + +- **Map used after its request ended** (async-plugin `Callable`, stored `SessionAware` field): with a session pinned, no call may touch the request — Task 2 `testPinnedSessionNeverTouchesTheRequestAgain` (closed `MockHttpServletRequest`). + +--- + +### Task 1: Deterministic NPE reproduction and race-free `SessionMap` + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java` +- Test: `core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java` + +**Interfaces:** +- Produces: `SessionMap` with the same public API; `protected volatile HttpSession session` stays the pinned field. + +- [ ] **Step 1: Write the failing test** (in `SessionMapTest`) + +```java +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 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()); +} +``` + +- [ ] **Step 2: Run it and watch it fail on current code** + +Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=SessionMapTest#testGetConcurrentWithInvalidateNeverThrows -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` +Expected: `exit=1`, failure message `get() concurrent with invalidate() threw` caused by `NullPointerException ... because "this.session" is null`. Run it 3 times; it must fail every time. + +- [ ] **Step 3: Make every method read `session` once into a local and drop the interned-id lock** + +`session` becomes `protected volatile HttpSession session`. Each method: `HttpSession current = session; if (current == null) return ;` then operate on `current` only. Remove every `synchronized (… getId().intern())` block. `put` keeps `synchronized (this)` only around pinning the session from `request.getSession(true)` when none is pinned, and obtains the old value through the overridable `get(key)` (pinned by `MockSessionMap`). `invalidate()` sets `session = null` after `current.invalidate()`. + +- [ ] **Step 4: Run `SessionMapTest` and verify it passes** + +Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=SessionMapTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` +Expected: `exit=0`, the new test plus the 12 existing ones pass. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java +git commit -m "WW-3576 fix(core): read SessionMap's session once per call and drop the interned-id lock" +``` + +### Task 2: Invalidation detection and no `entries` cache + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java` +- Test: `core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java` + +**Interfaces:** +- Consumes: Task 1's pinned `session` field. +- Produces: `entries` field deprecated and unread; un-pinning on `IllegalStateException`. + +- [ ] **Step 1: Write the failing tests** + +```java +public void testSessionInvalidatedElsewhereReadsAsNoSession() { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession session = new MockHttpSession(); + session.setAttribute("key", "value"); + request.setSession(session); + SessionMap sessionMap = new SessionMap(request); + + session.invalidate(); + + assertNull(sessionMap.get("key")); + assertFalse(sessionMap.containsKey("key")); + assertTrue(sessionMap.entrySet().isEmpty()); + assertNull(sessionMap.remove("key")); + sessionMap.clear(); + sessionMap.invalidate(); +} + +public void testPutAfterInvalidationElsewhereUsesANewSession() { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession session = new MockHttpSession(); + request.setSession(session); + SessionMap sessionMap = new SessionMap(request); + + session.invalidate(); + sessionMap.put("key", "value"); + + HttpSession current = request.getSession(false); + assertNotNull(current); + assertNotSame(session, current); + assertEquals("value", current.getAttribute("key")); +} + +public void testAttributeSetDirectlyIsVisibleThroughEntrySet() { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession session = new MockHttpSession(); + request.setSession(session); + SessionMap sessionMap = new SessionMap(request); + assertTrue(sessionMap.entrySet().isEmpty()); + + session.setAttribute("key", "value"); + + assertEquals(1, sessionMap.entrySet().size()); +} + +public void testPinnedSessionNeverTouchesTheRequestAgain() { + MockHttpServletRequest request = new MockHttpServletRequest(); + request.setSession(new MockHttpSession()); + SessionMap sessionMap = new SessionMap(request); + request.close(); + + sessionMap.put("key", "value"); + assertEquals("value", sessionMap.get("key")); + assertEquals(1, sessionMap.entrySet().size()); + assertEquals("value", sessionMap.remove("key")); +} +``` + +- [ ] **Step 2: Run them and verify the first three fail** + +Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=SessionMapTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` +Expected: `exit=1`; `testSessionInvalidatedElsewhereReadsAsNoSession` errors with `IllegalStateException`, `testPutAfterInvalidationElsewhereUsesANewSession` errors with `IllegalStateException`, `testAttributeSetDirectlyIsVisibleThroughEntrySet` fails on the cached empty set. `testPinnedSessionNeverTouchesTheRequestAgain` already passes (it guards Task 1/2 against re-resolving through the request). + +- [ ] **Step 3: Implement detection and remove the cache** + +Each session call is wrapped so an `IllegalStateException` un-pins (`session = null` only if it still holds the same reference) and returns the "no session" answer (`null` / `false` / `Collections.emptySet()` / no-op). `put` on an un-pinned map goes through the same `request.getSession(true)` path as a first `put`. `entrySet()` builds a fresh `HashSet` on every call; `setValue` writes to the session captured when the set was built. `entries` becomes `@Deprecated(since = "7.5.0", forRemoval = true)` and is never read or written. + +- [ ] **Step 4: Run the tests and verify they pass** + +Run: `./mvnw -q test -DskipAssembly -pl core -Dtest='SessionMapTest,AttributeMapTest,DispatcherTest' -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` +Expected: `exit=0`. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java +git commit -m "WW-3576 fix(core): treat an invalidated session as absent and stop caching SessionMap entries" +``` + +The `execAndWait` session sandbox (former Tasks 3–6) was split out to [WW-5787](https://issues.apache.org/jira/browse/WW-5787) (8.0.0) after review. + +### Task 7: Reactor build and hand-off items + +- [ ] **Step 1: Full reactor test-compile** — `./mvnw -q test-compile -DskipAssembly > …/scratchpad/reactor.log 2>&1; echo exit=$?`, expected `exit=0` (the async plugin and other modules still compile against `SessionMap`). +- [ ] **Step 2: Ask Łukasz** before filing the 8.0.0 Task to remove the deprecated `SessionMap.entries` field. +- [ ] **Step 3: Record owed docs in memory** — 7.5.0 migration notes (subclass-visible `SessionMap` changes) and the struts-site `execAndWait` page. diff --git a/docs/superpowers/specs/2026-10-18-WW-3576-session-map-staleness-design.md b/docs/superpowers/specs/2026-10-18-WW-3576-session-map-staleness-design.md new file mode 100644 index 0000000000..87bb488cea --- /dev/null +++ b/docs/superpowers/specs/2026-10-18-WW-3576-session-map-staleness-design.md @@ -0,0 +1,107 @@ +# WW-3576 — SessionMap staleness + +Ticket: [WW-3576](https://issues.apache.org/jira/browse/WW-3576) — fix version 7.5.0. +Previous attempt: PR #1577 (closed 2026-10-17; changed the lock to `this` and swallowed `IllegalStateException`). + +## Intent + +`SessionMap` is a wrapper around `HttpSession`, but it caches state that belongs to the container: the +`session` reference and the `entries` snapshot. Any operation on the real session — invalidation by +another request, attributes set directly on `HttpSession` — makes the wrapper outdated. The ticket's +two symptoms are both staleness: + +1. **NPE race (confirmed on `main`).** Every method does `if (session == null) return;` then + `synchronized (session.getId().intern()) { session.… }`. The field is re-read inside the lock; a + concurrent `invalidate()` on the same map nulls it in between. Realistic trigger: `execAndWait`, + where the background thread binds the *same* `ActionContext` (and therefore the same `SessionMap`) + as the request that started it (`StrutsBackgroundProcess#beforeInvocation`). +2. **`IllegalStateException` "already invalidated"** (the reporter's stress-test symptom). Another + request invalidated the `HttpSession`; this request's `SessionMap` still holds the dead reference. + +Fix the staleness, not the locking: one map that caches nothing but the session it pinned and notices +when that session dies. + +## Goals + +- Request-thread `SessionMap` always reflects the pinned session's live attributes; an invalidated + session reads as "no session" instead of throwing. +- No map resolves a session through a request that has already ended. + +## Non-goals + +- Making the shared `ActionContext` safe for `execAndWait` background threads, including isolating + their session writes. A session sandbox with controlled write-back was designed and built on this + ticket, then split out: it changes observable `execAndWait` behaviour, so it moves to the 8.0.0 + rewrite, [WW-5787](https://issues.apache.org/jira/browse/WW-5787). + +## Anti-goals + +- Catching `IllegalStateException` and carrying on as if the session were alive (PR #1577 swallowed it in + every method and kept the dead reference). Catching it to *detect* invalidation, un-pin, and answer + "no session" is the design — including a `put` that discovers the invalidation, un-pins, and pins a + new session once through the normal `request.getSession(true)` path (no loop). +- Any request-thread map that resolves its session through a request after that request has ended. + +## Design + +### Request-scoped `SessionMap` + +- **Pins the session it first sees (decided)**, as today: `request.getSession(false)` at construction, + `request.getSession(true)` on the first `put` when none is pinned. It does **not** re-resolve through + the request on every call — code holding the map past its request (async-plugin `Callable`s, stored + `SessionAware` fields) must never resolve a session through a request that has already ended. +- **Invalidation is detected, not hidden:** an `IllegalStateException` from the pinned session means it + was invalidated (by another request or directly); the map un-pins it and answers as "no session" + (`null` / `false` / empty set / no-op). A later `put` obtains a session through + `request.getSession(true)` — **only while the request is live**. +- **Detached at request end (decided after the final review):** the request cleanup in the dispatcher + detaches every `SessionMap` built for that request (each registers itself on the request at + construction), not only the bound context's: this covers the original map left behind when + `createSession` replaces `ActionContext.SESSION`, and the maps of a nested forward. While the request + is in async mode, detach (of all request maps, including the bound context's) is deferred to the + async dispatch's cleanup. A detached map never calls its request again: with a live pinned session it + keeps working (async `Callable`s, stored `SessionAware` fields); a `put` with no live pinned session + throws `IllegalStateException`. Reason: a background thread (`execAndWait`, custom + `BackgroundProcess`) must not resolve a session through a request that has already ended; this applies + equally to a never-pinned map used after its request ended. Detach is package-private. +- The pinned reference is read once per call into a local (no check-then-reread), so a concurrent + `invalidate()` on the same map cannot NPE; how it is published (`volatile`) is the builder's. +- **No `entries` cache:** `entrySet()` is built fresh on every call; an entry's `setValue` writes + straight through to the pinned session. +- `invalidate()` invalidates the pinned session, if any, and un-pins it. +- `put`/`remove` keep obtaining the old value through the overridable `get(key)` — + `SessionMapTest.MockSessionMap` pins that subclass contract. +- **No locking at all.** Single attribute calls are thread-safe per the Servlet spec. `put`/`remove` + return an advisory old value; nothing in the framework relies on cross-request atomicity + (decided). The `session.getId().intern()` monitor is removed — it was a lock on the JVM-wide string + pool. +- `protected Set<…> entries` becomes `@Deprecated(since = "7.5.0", forRemoval = true)` and is no + longer read by `SessionMap`; file a WW Task at 8.0.0 for its removal (do not reference it in source + before it exists). Whether `protected HttpSession session` stays the pinned field or is deprecated + behind a private one is the builder's, provided subclass access keeps compiling in 7.5.0. Changed + subclass-visible behaviour goes in the 7.5.0 migration notes. +- `StrutsBackgroundProcess.done` becomes `volatile`: the polling request reads the flag the background + thread writes. + +## Testing + +Match the target file's JUnit style: `SessionMapTest` extends `junit.framework.TestCase` (JUnit 3 — +`testXxx()` names, no `@Test`). + +1. **RED first — deterministic NPE reproduction** on current `main`: one `SessionMap`, thread A in + `invalidate()` held by a latch inside `HttpSession#invalidate`, thread B calling `get()`; on current + code B blocks on the monitor and then NPEs. The assertion is behavioural ("`get()` concurrent with + `invalidate()` returns a value or null and never throws"), so it passes on the new code where B does + not block. No sleep-based stress tests. +2. **Reporter's case:** two maps over the same session; the session is invalidated through the other + one (or directly); `get`/`containsKey`/`entrySet`/`remove` return empty results, no + `IllegalStateException`; `put` creates a new session only because the request has none. +3. Staleness: attribute set directly on `HttpSession` is visible through an existing map and its + `entrySet()`. +4. Existing `SessionMapTest`, `ExecuteAndWaitInterceptorTest` and the tests constructing + `new SessionMap(request)` keep passing. + +## Left to the builder + +- Whether `CreateSessionInterceptor` is simplified in this change. +- Debug log wording.