From 710d9df737f7078d8d8f10074a1be0c85f4e13e9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 08:50:41 +0200 Subject: [PATCH 01/27] WW-3576 docs: design for SessionMap staleness and the execAndWait session sandbox Request-thread SessionMap pins its session, detects invalidation and drops the entries cache and the interned-id lock. execAndWait background processes work on a sandboxed session snapshot reconciled on the polling request, gated on the session id, with per-action conflict resolution. Enforcing the sandbox is WW-5787. Co-Authored-By: Claude Opus 5.5 --- ...18-WW-3576-session-map-staleness-design.md | 179 ++++++++++++++++++ 1 file changed, 179 insertions(+) create mode 100644 docs/superpowers/specs/2026-10-18-WW-3576-session-map-staleness-design.md 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..6ade85c848 --- /dev/null +++ b/docs/superpowers/specs/2026-10-18-WW-3576-session-map-staleness-design.md @@ -0,0 +1,179 @@ +# WW-3576 — SessionMap staleness and the execAndWait session snapshot + +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 (request threads), and one map that is stale *by design* and explicit about it +(the `execAndWait` background thread), with a controlled write-back. + +## Goals + +- Request-thread `SessionMap` always reflects the pinned session's live attributes; an invalidated + session reads as "no session" instead of throwing. +- `execAndWait` background code gets a session view that does not depend on the (recycled) request. +- Background writes reach the real session once, at a well-defined point, never across users. +- Per-action control over write-back conflicts via a `*Aware` interface. + +## Non-goals + +- Making the rest of the shared `ActionContext` (request, response, parameters) safe for background + threads. +- Live, mid-process propagation of background writes. Progress reporting already goes through the + action (`ExecuteAndWaitInterceptor` pushes `bp.getAction()` on every poll), not through the session. +- A global (application-wide) conflict strategy bean. +- Enforcing the sandbox (throwing on escapes) or isolating the background thread's value stack and + `ActionContext`. That is a rewrite of the `execAndWait` mechanism to run in a real sandbox, planned + for 8.0.0 as [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 retried the + write into a new session inside the catch). Catching it to *detect* invalidation and answer + "no session" is the design. +- Any request-thread map that resolves its session through a request after that request has ended. +- Any path by which background writes land in a session other than the one the process started in. + +## Design + +### 1. Request-scoped `SessionMap` (default) + +- **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 through a recycled request, which may be another user's. +- **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)` — the same path, and the same trust in the request, as a first `put` today. +- 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. + +### 2. Sandboxed background session (`execAndWait` only) + +A `SessionMap` subclass (so existing `((SessionMap) session).invalidate()` casts keep working) that +never touches the request: + +- **Taken on the request thread**, when the interceptor creates the `BackgroundProcess` (inside the + existing `synchronized (httpSession)` block) — never on the background thread, where the request + may already be recycled. Holds: a shallow copy of the session attributes, the session id at that + moment, and a log of its own writes (puts, removes, and an invalidate request). +- Reads return the copy with the logged writes applied. +- **Installed for the background thread:** the background thread binds an `ActionContext` that is a + copy of the invocation context with `withSession(snapshot)`, so the original request thread keeps + its own request map. If the action is `SessionAware`, it is re-injected with the snapshot before the + invocation runs. +- **Sandbox model (decided):** the background process works only on its sandboxed session; the + supported way to change the real session is to write to the sandbox and let write-back reconcile it + (with the per-action hook deciding conflicts). There is deliberately no callback handing background + code the real session. Supported entry points: `ActionContext.getContext().getSession()` and + `SessionAware`. +- **Sandbox escapes (decided: warn only in 7.5.0):** OGNL `#session` and + `invocation.getInvocationContext().getSession()` still reach the request's pinned map, and writes + through them go live immediately, as today. A write through a request map from an `execAndWait` + background thread logs one WARN per background process (action name, never attribute values), + pointing to the sandbox entry points. Documented as dangerous in the `ExecuteAndWaitInterceptor` / + `BackgroundProcess` Javadoc, the `execAndWait` docs on struts-site, and the 7.5.0 migration notes. How + the map recognises a background thread is the builder's. +- *(Builder-check call)* Handed over through a new **default method on `BackgroundProcess`** + (returning no snapshot by default) and a `StrutsBackgroundProcess` constructor taking it. Installing it + happens in `StrutsBackgroundProcess`'s private thread body, *before* the overridable + `beforeInvocation()`, so a subclass that skips `super` keeps it. A custom `BackgroundProcess` not + extending `StrutsBackgroundProcess` keeps today's shared-map behaviour — migration-notes line. +- Not serialized: held `transient` by the background process. A deserialized process already has a + null invocation and is restarted (WW-4900), so the snapshot is lost with it. + +### 3. Write-back + +- Happens on the **polling request that observes `bp.isDone()`** (the `else if (… bp.isDone())` branch + in `ExecuteAndWaitInterceptor#doIntercept`), on a request thread, inside the existing + `synchronized (httpSession)`, before the result is returned. +- **Session-id gate (decided, not overridable):** if the current session id differs from the snapshot's + start id, or there is no session, all background writes are discarded and logged at debug. +- Otherwise apply **only the logged writes**: keys the background never touched are left alone. +- A logged `invalidate()` is applied after the id gate passes; the conflict hook is not consulted for it. +- After write-back, a `SessionAware` action is re-injected with the request's session map so the + result rendering writes to the live session. +- If nobody polls after completion, the writes are never applied (the session expires with them). +- *(Builder-check call)* Write-back runs **before** `throw bp.getException()`: a failed background + action's writes are applied, matching today's behaviour where its writes were already live. +- *(Builder-check call)* The write log is an **ordered operation list** (put / remove / invalidate). + After a logged `invalidate()`, snapshot reads see an empty session. At write-back the real session is + invalidated, a fresh one is created on the polling request (`getSession(true)` — the background asked + for this, e.g. a login flow, so it is not the anti-goal's "absorb a write after *another* request + invalidated"), and the operations after the invalidate are applied to it without conflict checks. + +### 4. Conflicts and the per-action hook + +- **Conflict:** a key the background wrote (put or remove) whose value in the real session no longer + `equals` the snapshot value (absent counts as a value). Known limit: an object mutated in place + without a new `setAttribute` is the same reference, so it is not detected. +- **Default (decided):** keep the real session's value, drop the background's, log at debug. +- **Hook (decided: per action, `*Aware`):** an action implementing the new interface is consulted once + per conflicting key with: key, snapshot value, current value, background value. It must be able to + express: keep current, take background, set a different value, remove the key. Lives in + `org.apache.struts2.action` next to `SessionAware`/`ParameterNameAware`. Proposed names (builder may + refine): `BackgroundSessionConflictAware`, method + `resolveSessionConflict(SessionConflict)` returning a `SessionConflictResolution`. +- An exception thrown by the hook propagates like any other action exception from the polling request. + +## Testing + +Match the target file's JUnit style: `SessionMapTest` extends `junit.framework.TestCase`, +`ExecuteAndWaitInterceptorTest` extends `StrutsInternalTestCase` (both 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. Snapshot: reads/writes isolated from the live session while running; write-back applies only logged + keys; id change discards everything; default conflict keeps current; hook outcomes (all four); + logged invalidate applied; `SessionAware` re-injection in both directions. +5. Sandbox escape: a write through the request map from a background thread goes live and logs exactly + one WARN per process; writes through the sandbox log nothing. +6. Existing `SessionMapTest`, `ExecuteAndWaitInterceptorTest` and the tests constructing + `new SessionMap(request)` keep passing. + +## Left to the builder + +- Class names of the snapshot map and the conflict/resolution types; whether resolution is an enum plus + value or a small sealed hierarchy. +- How the `ActionContext` copy is made for the background thread. +- Whether `CreateSessionInterceptor` is simplified in this change. +- Debug log wording. From 0ee4abfb00e03ab3960b8b09b40a305f0083e381 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 08:53:51 +0200 Subject: [PATCH 02/27] WW-3576 docs: implementation plan for SessionMap staleness and the execAndWait sandbox Co-Authored-By: Claude Opus 5.5 --- ...026-10-18-WW-3576-session-map-staleness.md | 470 ++++++++++++++++++ ...18-WW-3576-session-map-staleness-design.md | 10 +- 2 files changed, 476 insertions(+), 4 deletions(-) create mode 100644 docs/superpowers/plans/2026-10-18-WW-3576-session-map-staleness.md 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..ee42fa18b6 --- /dev/null +++ b/docs/superpowers/plans/2026-10-18-WW-3576-session-map-staleness.md @@ -0,0 +1,470 @@ +# WW-3576 SessionMap Staleness and execAndWait Session Sandbox — 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) and give `execAndWait` background processes a sandboxed session reconciled on the polling request. + +**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. A new `SandboxedSessionMap` (snapshot + ordered operation log) is created by `ExecuteAndWaitInterceptor` on the request thread, installed by `StrutsBackgroundProcess` on the background thread, and reconciled by `SessionSandboxReconciler` on the polling request that sees `isDone()`, gated on the session id, with per-action conflict resolution through `BackgroundSessionConflictAware`. + +**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. +- The session-id gate is not overridable by the hook. + +## 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`). +- **`put(key, null)`** on the sandbox: Servlet semantics treat it as a removal, so it is logged as one — Task 3 `testPutNullIsLoggedAsRemove`. +- **`entrySet()` entry `setValue` on the sandbox**: must be logged like a `put`, not lost — Task 3 `testEntrySetValueIsLogged`. +- **Visibility between the background thread and the polling request**: the operation log written on one thread is read on another — Task 3 makes `SandboxedSessionMap` methods `synchronized`; Task 5 `testBackgroundWritesReachTheRealSession` exercises it across real threads. +- **Hook throws**: the exception propagates from the polling request, and no partial write-back is hidden — Task 4 `testHookExceptionPropagates`. + +--- + +### 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" +``` + +### Task 3: `SandboxedSessionMap` + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java` (add `protected SessionMap()` for subclasses that never touch a request) +- Create: `core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java` +- Create: `core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java` +- Test: `core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java` (extends `junit.framework.TestCase`) + +**Interfaces:** +- Produces: + - `public class SandboxedSessionMap extends SessionMap` — `public SandboxedSessionMap(HttpSession session, String processName)` copies all attributes (shallow) and records `session.getId()`; overrides `get`, `put`, `remove`, `containsKey`, `entrySet`, `clear`, `invalidate`, all `synchronized`. + - `public String getStartSessionId()`, `public String getProcessName()` + - `public synchronized List getOperations()` — unmodifiable copy, in order. + - `public synchronized Object getSnapshotValue(String key)` — the value at snapshot time, `null` if absent. + - `public sealed interface SessionOperation` with records `Put(String key, Object value)`, `Remove(String key)`, `Invalidate()`. + +- [ ] **Step 1: Write the failing tests** + +```java +public void testReadsSeeSnapshotPlusOwnWritesAndNeverTheLiveSession() { + MockHttpSession live = new MockHttpSession(); + live.setAttribute("a", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + + live.setAttribute("a", "changed"); + sandbox.put("b", "2"); + + assertEquals("1", sandbox.get("a")); + assertEquals("2", sandbox.get("b")); + assertNull(live.getAttribute("b")); + assertEquals(live.getId(), sandbox.getStartSessionId()); + assertEquals(List.of(new SessionOperation.Put("b", "2")), sandbox.getOperations()); +} + +public void testRemoveClearAndInvalidateAreLoggedInOrder() { + MockHttpSession live = new MockHttpSession(); + live.setAttribute("a", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + + sandbox.remove("a"); + sandbox.invalidate(); + sandbox.put("user", "u"); + + assertEquals(List.of(new SessionOperation.Remove("a"), new SessionOperation.Invalidate(), + new SessionOperation.Put("user", "u")), sandbox.getOperations()); + assertEquals(Map.of("user", "u"), new HashMap<>(sandbox)); + assertEquals("1", sandbox.getSnapshotValue("a")); +} + +public void testPutNullIsLoggedAsRemove() { + SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "p"); + sandbox.put("a", null); + assertEquals(List.of(new SessionOperation.Remove("a")), sandbox.getOperations()); +} + +public void testEntrySetValueIsLogged() { + MockHttpSession live = new MockHttpSession(); + live.setAttribute("a", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + + sandbox.entrySet().iterator().next().setValue("2"); + + assertEquals("2", sandbox.get("a")); + assertEquals(List.of(new SessionOperation.Put("a", "2")), sandbox.getOperations()); +} +``` + +Also: `clear()` logs one `Remove` per key present in the view at that moment. + +- [ ] **Step 2: Run them and verify they fail to compile** + +Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=SandboxedSessionMapTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` +Expected: `exit=1`, compilation error `cannot find symbol: class SandboxedSessionMap`. + +- [ ] **Step 3: Implement `SessionOperation`, `SandboxedSessionMap` and `protected SessionMap()`** + +View is a `LinkedHashMap` copied from the session; `invalidate()` clears the view and logs `Invalidate`. `SandboxedSessionMap` keeps its own fields and never touches `session`, `request` or the deprecated `entries`. It is not `Serializable`-relevant: the process holds it `transient` (Task 5). + +- [ ] **Step 4: Run the tests and verify they pass** + +Same command as Step 2. Expected: `exit=0`. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java +git commit -m "WW-3576 feat(core): add a sandboxed session map recording background writes" +``` + +### Task 4: Reconciliation and the per-action conflict hook + +**Files:** +- Create: `core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java` +- Create: `core/src/main/java/org/apache/struts2/action/SessionConflict.java` +- Create: `core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java` +- Create: `core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java` +- Test: `core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java` (extends `junit.framework.TestCase`) + +**Interfaces:** +- Consumes: Task 3's `SandboxedSessionMap`, `SessionOperation`. +- Produces: + - `public record SessionConflict(String key, Object snapshotValue, Object currentValue, Object backgroundValue)` — `null` means absent. + - `public final class SessionConflictResolution` — `static keepCurrent()`, `static useBackground()`, `static replaceWith(Object value)`, `static remove()`. + - `public interface BackgroundSessionConflictAware { SessionConflictResolution resolveSessionConflict(SessionConflict conflict); }` + - `public final class SessionSandboxReconciler` — `public static void reconcile(SandboxedSessionMap sandbox, HttpServletRequest request, Object action)`. + +- [ ] **Step 1: Write the failing tests** + +Fixture: `MockHttpServletRequest` with a `MockHttpSession live`; sandbox built from `live`. + +```java +public void testOnlyLoggedKeysAreApplied() // live "other" changed after snapshot, untouched by background → kept; background put "b" → applied +public void testSessionIdChangeDiscardsEverything() // live.invalidate(); request.getSession(true) new; background put "b" → new session has no "b"; one debug line names the process +public void testNoSessionDiscardsEverything() // live.invalidate(), no new session → reconcile returns, request.getSession(false) still null +public void testConflictKeepsCurrentByDefault() // snapshot a=1, live a=live, background a=bg → live a stays "live" +public void testHookUseBackground() // action returns useBackground() → a=bg +public void testHookReplaceWith() // replaceWith("merged") → a=merged +public void testHookRemove() // remove() → a absent +public void testHookSeesAllFourValues() // assert SessionConflict("a", "1", "live", "bg") +public void testRemovalOfAKeyChangedElsewhereIsAConflict() // snapshot a=1, live a=live, background remove a → default keeps "live" +public void testBackgroundInvalidateStartsAFreshSession() // background invalidate + put user=u → old live invalid, new session has only user=u, hook never called +public void testHookExceptionPropagates() // hook throws IllegalArgumentException → reconcile throws it +``` + +Each name above is one test with exactly the assertion in its comment. + +- [ ] **Step 2: Run them and verify they fail to compile** + +Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=SessionSandboxReconcilerTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` +Expected: `exit=1`, `cannot find symbol: class SessionSandboxReconciler`. + +- [ ] **Step 3: Implement the types and `reconcile`** + +Algorithm (not determined by the signature): + +``` +real = request.getSession(false) +if real == null or !real.getId().equals(sandbox.getStartSessionId()): LOG.debug(process name); return +ops = sandbox.getOperations(); last = index of last Invalidate, or -1 +if last >= 0: + real.invalidate(); target = request.getSession(true) + apply final value of every key touched after `last` to target (null → removeAttribute); no conflict checks + return +for each key touched in ops (in first-touch order), final = sandbox.get(key): + current = real.getAttribute(key); snapshot = sandbox.getSnapshotValue(key) + if Objects.equals(current, snapshot): write final (null → removeAttribute) + else if action instanceof BackgroundSessionConflictAware: apply its resolution + else: LOG.debug(process name, key); keep current +``` + +- [ ] **Step 4: Run the tests and verify they pass** + +Same command as Step 2. Expected: `exit=0`. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java core/src/main/java/org/apache/struts2/action/SessionConflict.java core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java +git commit -m "WW-3576 feat(core): reconcile a session sandbox with per-action conflict resolution" +``` + +### Task 5: Wire the sandbox into `execAndWait` + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java:64-92` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java:296-345` +- Test: `core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java` (extends `StrutsInternalTestCase`), plus a test action `core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java` implementing `SessionAware`, registered in `WaitConfigurationProvider` as `sessionAction` + +**Interfaces:** +- Consumes: Tasks 3–4. +- Produces: + - `BackgroundProcess`: `default BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { return this; }`, `default SandboxedSessionMap getSessionSandbox() { return null; }` + - `StrutsBackgroundProcess` overrides both; field `private transient SandboxedSessionMap sessionSandbox`. + +- [ ] **Step 1: Write the failing tests** + +`SessionWritingWaitAction.execute()` writes `"result" → "done"` through `ActionContext.getContext().getSession()` and `"aware" → "yes"` through its injected `SessionAware` map; a field `failWith` makes it throw after writing. + +```java +public void testBackgroundWritesReachTheRealSession() // first proxy → "wait"; wait for done; second proxy → "success"; httpSession has result=done and aware=yes +public void testBackgroundWritesAreInvisibleUntilPolled() // after done but before the second proxy: httpSession has neither "result" nor "aware" +public void testWritesOfAFailedActionAreApplied() // failWith set → second proxy throws; httpSession has result=done +public void testSessionAwareGetsTheLiveMapBackAfterWriteBack() // after second proxy, the action's session map is not a SandboxedSessionMap +``` + +The existing setUp puts a plain `HashMap` as the context session; these tests replace it with `new SessionMap(request)` so the context session is backed by `httpSession`. + +- [ ] **Step 2: Run them and verify they fail** + +Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=ExecuteAndWaitInterceptorTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` +Expected: `exit=1`; `testBackgroundWritesAreInvisibleUntilPolled` and `testSessionAwareGetsTheLiveMapBackAfterWriteBack` fail (writes are live today). + +- [ ] **Step 3: Implement the wiring** + +- Interceptor, creation branch: `new SandboxedSessionMap(httpSession, name)` and `getNewBackgroundProcess(...).withSessionSandbox(sandbox).prepare()`. +- Interceptor, `isDone()` branch, after `session.remove(bp_SessionKey)` and **before** `throw bp.getException()`: if `bp.getSessionSandbox()` is non-null, `SessionSandboxReconciler.reconcile(sandbox, ServletActionContext.getRequest(), bp.getAction())`, then re-inject `context.getSession()` into a `SessionAware` action. +- `StrutsBackgroundProcess` thread body: `beforeInvocation(); installSessionSandbox(); result = invocation.invokeActionOnly();` — `installSessionSandbox()` is private: when a sandbox is set, bind `ActionContext.of(new HashMap<>(invocation.getInvocationContext().getContextMap())).withSession(sandbox)` and call `withSession(sandbox)` on a `SessionAware` action. + +- [ ] **Step 4: Run the tests and verify they pass** + +Same command as Step 2. Expected: `exit=0`, existing tests (`testFromDeserializedSession`, token tests) still pass. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java +git commit -m "WW-3576 feat(core): run execAndWait background processes against a session sandbox" +``` + +### Task 6: Warn on sandbox escapes and document them + +**Files:** +- Modify: `core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java` +- Modify: `core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java` +- Modify: `core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java` +- Modify: Javadoc of `ExecuteAndWaitInterceptor` and `BackgroundProcess` +- Test: `core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java` + +**Interfaces:** +- Produces: `public static SandboxedSessionMap current()` (thread-local backed, read by `SessionMap`); `public static void runInSandbox(SandboxedSessionMap sandbox, Runnable work)` sets `current()` for the duration of `work` and clears it in `finally`; `void warnEscapeOnce()`. `StrutsBackgroundProcess`'s thread body runs `invokeActionOnly()` inside `runInSandbox` when a sandbox is set. + +- [ ] **Step 1: Write the failing test** + +```java +public void testWriteThroughRequestMapFromBackgroundThreadWarnsOnce() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession live = new MockHttpSession(); + request.setSession(live); + SessionMap requestMap = new SessionMap(request); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "longJob"); + + try (LogCapture logs = new LogCapture(SandboxedSessionMap.class)) { + Thread background = new Thread(() -> SandboxedSessionMap.runInSandbox(sandbox, () -> { + requestMap.put("a", "1"); + requestMap.put("b", "2"); + sandbox.put("c", "3"); + })); + background.start(); + background.join(5000); + + assertEquals("1", live.getAttribute("a")); + assertEquals(1, logs.messages().size()); + assertTrue(logs.messages().get(0).contains("longJob")); + assertFalse(logs.messages().get(0).contains("1")); + } +} +``` + +`runInSandbox(SandboxedSessionMap, Runnable)` is the package-level helper `StrutsBackgroundProcess` uses to set and clear `current()`. + +- [ ] **Step 2: Run it and verify it fails to compile** + +Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=SandboxedSessionMapTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` +Expected: `exit=1`, `cannot find symbol: method runInSandbox`. + +- [ ] **Step 3: Implement the escape warning and Javadoc** + +`SessionMap.put/remove/clear/invalidate`: when `SandboxedSessionMap.current()` is non-null and `this` is not a `SandboxedSessionMap`, call `warnEscapeOnce()`; the write still goes live. WARN (once per sandbox, `AtomicBoolean`): `Background process [{}] wrote to the request session, bypassing its session sandbox; use ActionContext.getContext().getSession() or SessionAware`. Javadoc on `ExecuteAndWaitInterceptor` and `BackgroundProcess`: the sandbox model, that `#session` and `invocation.getInvocationContext().getSession()` bypass it, and `BackgroundSessionConflictAware`. + +- [ ] **Step 4: Run the full core suite** + +Run: `./mvnw -q test -DskipAssembly -pl core > /private/tmp/claude-501/-Users-lukaszlenart-Projects-Apache-struts/9c5c77a5-710a-4c52-9f6b-ec54c8d4b6aa/scratchpad/core-tests.log 2>&1; echo exit=$?` +Expected: `exit=0`. + +- [ ] **Step 5: Commit** + +```bash +git add core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java +git commit -m "WW-3576 feat(core): warn when a background process writes around its session sandbox" +``` + +### 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, sandbox model, `#session` escape, custom `BackgroundProcess` keeps shared map) 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 index 6ade85c848..87c8ac39fe 100644 --- 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 @@ -104,10 +104,12 @@ never touches the request: pointing to the sandbox entry points. Documented as dangerous in the `ExecuteAndWaitInterceptor` / `BackgroundProcess` Javadoc, the `execAndWait` docs on struts-site, and the 7.5.0 migration notes. How the map recognises a background thread is the builder's. -- *(Builder-check call)* Handed over through a new **default method on `BackgroundProcess`** - (returning no snapshot by default) and a `StrutsBackgroundProcess` constructor taking it. Installing it - happens in `StrutsBackgroundProcess`'s private thread body, *before* the overridable - `beforeInvocation()`, so a subclass that skips `super` keeps it. A custom `BackgroundProcess` not +- *(Builder-check call, refined while planning)* Handed over through **default methods on + `BackgroundProcess`** — a fluent `withSessionSandbox(…)` (no-op by default) called by the interceptor + on whatever the overridable `getNewBackgroundProcess(…)` factory returns, and `getSessionSandbox()` + (null by default). Installing it happens in `StrutsBackgroundProcess`'s private thread body, *after* + the overridable `beforeInvocation()` (whose default binds the original context), so neither `super` + nor a subclass that skips it can undo it. A custom `BackgroundProcess` not extending `StrutsBackgroundProcess` keeps today's shared-map behaviour — migration-notes line. - Not serialized: held `transient` by the background process. A deserialized process already has a null invocation and is restarted (WW-4900), so the snapshot is lost with it. From e6526359f8f926f1f352d8c9a469f2b4654ff441 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 08:56:59 +0200 Subject: [PATCH 03/27] WW-3576 fix(core): read SessionMap's session once per call and drop the interned-id lock Co-Authored-By: Claude Opus 5.5 --- .../apache/struts2/dispatcher/SessionMap.java | 114 +++++++++--------- .../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 76af042bef..54318391bf 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 implements Serializa @Serial private static final long serialVersionUID = 4678843241638046854L; - protected HttpSession session; + protected volatile HttpSession session; protected Set> entries; protected HttpServletRequest request; @@ -62,15 +62,14 @@ public SessionMap(final HttpServletRequest request) { * 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 void invalidate() { */ @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(); - while (attributeNamesEnum.hasMoreElements()) { - session.removeAttribute(attributeNamesEnum.nextElement()); - } + entries = null; + final Enumeration attributeNamesEnum = current.getAttributeNames(); + while (attributeNamesEnum.hasMoreElements()) { + current.removeAttribute(attributeNamesEnum.nextElement()); } - } /** @@ -100,32 +97,33 @@ public void clear() { */ @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<>(); + Set> result = entries; + if (result == null) { + result = new HashSet<>(); - final Enumeration enumeration = session.getAttributeNames(); + final Enumeration 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 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); - } + return current.getAttribute(key != null ? key.toString() : null); } /** @@ -157,17 +154,20 @@ public Object get(final Object key) { */ @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 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; + 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 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); + 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 b15f8a308c..3edc9c3803 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.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 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<>(); + + 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); From 499415ea2c467cb3922e821874a9dbff67594345 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 08:59:09 +0200 Subject: [PATCH 04/27] WW-3576 fix(core): keep the pinned session in a local in SessionMap.put and harden the race test Co-Authored-By: Claude Opus 5.5 --- .../java/org/apache/struts2/dispatcher/SessionMap.java | 7 ++++--- .../java/org/apache/struts2/dispatcher/SessionMapTest.java | 7 +++++-- 2 files changed, 9 insertions(+), 5 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 54318391bf..07b1ddffdc 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -157,10 +157,11 @@ public Object put(final String key, final Object value) { HttpSession current = session; if (current == null) { synchronized (this) { - if (session == null) { - session = request.getSession(true); - } current = session; + if (current == null) { + current = request.getSession(true); + session = current; + } } } 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 3edc9c3803..5a602ebad9 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java @@ -296,12 +296,13 @@ public void invalidate() { 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 { sessionMap.get("key"); } catch (Throwable t) { failure.set(t); } + try { read.set(sessionMap.get("key")); } catch (Throwable t) { failure.set(t); } }); reader.start(); long deadline = System.currentTimeMillis() + 5000; @@ -313,7 +314,9 @@ public void invalidate() { invalidator.join(5000); reader.join(5000); - assertNull("get() concurrent with invalidate() threw", failure.get()); + assertFalse("reader did not finish", reader.isAlive()); + assertNull("get() concurrent with invalidate() threw: " + failure.get(), failure.get()); + assertEquals("value", read.get()); } @Override From a75621de5c69ed19db72653ee04b2a7a08075482 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 09:00:46 +0200 Subject: [PATCH 05/27] WW-3576 fix(core): treat an invalidated session as absent and stop caching SessionMap entries Co-Authored-By: Claude Opus 5.5 --- .../apache/struts2/dispatcher/SessionMap.java | 98 +++++++++++++------ .../struts2/dispatcher/SessionMapTest.java | 56 +++++++++++ 2 files changed, 125 insertions(+), 29 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 07b1ddffdc..8a61c9cdfd 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -40,6 +40,10 @@ public class SessionMap extends AbstractMap implements Serializa private static final long serialVersionUID = 4678843241638046854L; 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; @@ -67,14 +71,16 @@ public void invalidate() { return; } - current.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() { @@ -83,15 +89,18 @@ public void clear() { return; } - entries = null; - final Enumeration attributeNamesEnum = current.getAttributeNames(); - while (attributeNamesEnum.hasMoreElements()) { - current.removeAttribute(attributeNamesEnum.nextElement()); + try { + final Enumeration attributeNamesEnum = current.getAttributeNames(); + while (attributeNamesEnum.hasMoreElements()) { + 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. */ @@ -102,10 +111,8 @@ public Set> entrySet() { return Collections.emptySet(); } - Set> result = entries; - if (result == null) { - result = new HashSet<>(); - + final Set> result = new HashSet<>(); + try { final Enumeration enumeration = current.getAttributeNames(); while (enumeration.hasMoreElements()) { @@ -120,7 +127,9 @@ public Object setValue(final Object obj) { } }); } - entries = result; + } catch (IllegalStateException e) { + unpin(current); + return Collections.emptySet(); } return result; @@ -142,7 +151,12 @@ public Object get(final Object key) { return null; } - return current.getAttribute(key != null ? key.toString() : null); + try { + return current.getAttribute(key != null ? key.toString() : null); + } catch (IllegalStateException e) { + unpin(current); + return null; + } } /** @@ -156,21 +170,39 @@ public Object get(final Object key) { public Object put(final String key, final Object value) { HttpSession current = session; if (current == null) { - synchronized (this) { - current = session; - if (current == null) { - current = request.getSession(true); - session = current; - } - } + current = pin(); } final Object oldValue = get(key); - entries = null; - current.setAttribute(key, value); + try { + current.setAttribute(key, value); + } catch (IllegalStateException e) { + unpin(current); + pin().setAttribute(key, value); + return null; + } return oldValue; } + private HttpSession pin() { + synchronized (this) { + HttpSession current = session; + if (current == null) { + current = request.getSession(true); + session = current; + } + return current; + } + } + + private void unpin(final HttpSession stale) { + synchronized (this) { + if (session == stale) { + session = null; + } + } + } + /** * Removes the specified session attribute. * @@ -187,11 +219,14 @@ public Object remove(final Object key) { return null; } - entries = null; - final String keyAsString = (key != null ? key.toString() : null); final Object value = get(keyAsString); - current.removeAttribute(keyAsString); + try { + current.removeAttribute(keyAsString); + } catch (IllegalStateException e) { + unpin(current); + return null; + } return value; } @@ -214,6 +249,11 @@ public boolean containsKey(final Object key) { } final String keyAsString = (key != null ? key.toString() : null); - return (current.getAttribute(keyAsString) != null); + try { + return current.getAttribute(keyAsString) != null; + } catch (IllegalStateException e) { + unpin(current); + return false; + } } } 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 5a602ebad9..4f182ee5e0 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java @@ -319,6 +319,62 @@ public void invalidate() { assertEquals("value", read.get()); } + 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")); + } + @Override protected void setUp() throws Exception { sessionMock = new Mock(HttpSession.class); From f78c62d465e62440c52ed274d83aceffa5666d12 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 09:02:47 +0200 Subject: [PATCH 06/27] WW-3576 test(core): exercise each SessionMap operation against its own invalidated session Co-Authored-By: Claude Opus 5.5 --- .../struts2/dispatcher/SessionMapTest.java | 55 ++++++++++++++++--- 1 file changed, 48 insertions(+), 7 deletions(-) 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 4f182ee5e0..135ca1954f 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java @@ -319,21 +319,62 @@ public void invalidate() { assertEquals("value", read.get()); } - public void testSessionInvalidatedElsewhereReadsAsNoSession() { - MockHttpServletRequest request = new MockHttpServletRequest(); - MockHttpSession session = new MockHttpSession(); - session.setAttribute("key", "value"); - request.setSession(session); - SessionMap sessionMap = new SessionMap(request); + 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; + } - session.invalidate(); + 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() { From 62c8ce3f21ffc8b9348dc183bbc5d88e3d20ec81 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 09:03:29 +0200 Subject: [PATCH 07/27] WW-3576 docs: a put that finds its session invalidated pins a new one once Co-Authored-By: Claude Opus 5.5 --- .../2026-10-18-WW-3576-session-map-staleness-design.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) 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 index 87c8ac39fe..faa2ff1be0 100644 --- 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 @@ -43,9 +43,10 @@ when that session dies (request threads), and one map that is stale *by design* ## Anti-goals -- Catching `IllegalStateException` and carrying on as if the session were alive (PR #1577 retried the - write into a new session inside the catch). Catching it to *detect* invalidation and answer - "no session" is the design. +- 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. - Any path by which background writes land in a session other than the one the process started in. From 170bf18bf238d8f9d4f073b37ba6a603a65ee99f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 09:04:56 +0200 Subject: [PATCH 08/27] WW-3576 feat(core): add a sandboxed session map recording background writes Co-Authored-By: Claude Opus 5.5 --- .../apache/struts2/dispatcher/SessionMap.java | 3 + .../interceptor/exec/SandboxedSessionMap.java | 135 ++++++++++++++++++ .../interceptor/exec/SessionOperation.java | 31 ++++ .../exec/SandboxedSessionMapTest.java | 92 ++++++++++++ 4 files changed, 261 insertions(+) create mode 100644 core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java create mode 100644 core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java create mode 100644 core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java 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 8a61c9cdfd..d59ea0a79b 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -48,6 +48,9 @@ public class SessionMap extends AbstractMap implements Serializa protected HttpServletRequest request; + protected SessionMap() { + } + /** * Creates a new session map given a http servlet request. Note, the enumeration of request * attributes will occur when the map entries are asked for. diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java new file mode 100644 index 0000000000..32991ea553 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java @@ -0,0 +1,135 @@ +/* + * 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.interceptor.exec; + +import jakarta.servlet.http.HttpSession; +import org.apache.struts2.dispatcher.SessionMap; + +import java.io.Serial; +import java.util.AbstractMap.SimpleEntry; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Enumeration; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.Set; + +/** + * The session view an execAndWait background process works on: a shallow snapshot of the session taken at + * construction plus an ordered log of the process's own writes. It never touches the live session. + */ +public class SandboxedSessionMap extends SessionMap { + + @Serial + private static final long serialVersionUID = 2907451127739228064L; + + private final transient Map snapshot = new LinkedHashMap<>(); + private final transient Map view = new LinkedHashMap<>(); + private final transient List operations = new ArrayList<>(); + private final transient String startSessionId; + private final transient String processName; + + public SandboxedSessionMap(HttpSession session, String processName) { + super(); + this.startSessionId = session.getId(); + this.processName = processName; + Enumeration names = session.getAttributeNames(); + while (names.hasMoreElements()) { + String name = names.nextElement(); + snapshot.put(name, session.getAttribute(name)); + } + view.putAll(snapshot); + } + + public String getStartSessionId() { + return startSessionId; + } + + public String getProcessName() { + return processName; + } + + public synchronized List getOperations() { + return Collections.unmodifiableList(new ArrayList<>(operations)); + } + + public synchronized Object getSnapshotValue(String key) { + return snapshot.get(key); + } + + @Override + public synchronized Object get(Object key) { + return view.get(asKey(key)); + } + + @Override + public synchronized Object put(String key, Object value) { + if (value == null) { + return remove(key); + } + operations.add(new SessionOperation.Put(key, value)); + return view.put(key, value); + } + + @Override + public synchronized Object remove(Object key) { + String name = asKey(key); + operations.add(new SessionOperation.Remove(name)); + return view.remove(name); + } + + @Override + public synchronized boolean containsKey(Object key) { + return view.get(asKey(key)) != null; + } + + @Override + public synchronized Set> entrySet() { + Set> result = new HashSet<>(); + for (Map.Entry current : view.entrySet()) { + String key = current.getKey(); + result.add(new SimpleEntry<>(key, current.getValue()) { + @Override + public Object setValue(Object value) { + return put(key, value); + } + }); + } + return result; + } + + @Override + public synchronized void clear() { + for (String key : new ArrayList<>(view.keySet())) { + remove(key); + } + } + + @Override + public synchronized void invalidate() { + view.clear(); + operations.add(new SessionOperation.Invalidate()); + } + + private static String asKey(Object key) { + return key != null ? key.toString() : null; + } +} diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java new file mode 100644 index 0000000000..c079270404 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java @@ -0,0 +1,31 @@ +/* + * 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.interceptor.exec; + +public sealed interface SessionOperation { + + record Put(String key, Object value) implements SessionOperation { + } + + record Remove(String key) implements SessionOperation { + } + + record Invalidate() implements SessionOperation { + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java new file mode 100644 index 0000000000..e8269a3780 --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java @@ -0,0 +1,92 @@ +/* + * 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.interceptor.exec; + +import junit.framework.TestCase; +import org.springframework.mock.web.MockHttpSession; + +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +public class SandboxedSessionMapTest extends TestCase { + + public void testReadsSeeSnapshotPlusOwnWritesAndNeverTheLiveSession() { + MockHttpSession live = new MockHttpSession(); + live.setAttribute("a", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + + live.setAttribute("a", "changed"); + sandbox.put("b", "2"); + + assertEquals("1", sandbox.get("a")); + assertEquals("2", sandbox.get("b")); + assertNull(live.getAttribute("b")); + assertEquals(live.getId(), sandbox.getStartSessionId()); + assertEquals("p", sandbox.getProcessName()); + assertEquals(List.of(new SessionOperation.Put("b", "2")), sandbox.getOperations()); + } + + public void testRemoveClearAndInvalidateAreLoggedInOrder() { + MockHttpSession live = new MockHttpSession(); + live.setAttribute("a", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + + sandbox.remove("a"); + sandbox.invalidate(); + sandbox.put("user", "u"); + + assertEquals(List.of(new SessionOperation.Remove("a"), new SessionOperation.Invalidate(), + new SessionOperation.Put("user", "u")), sandbox.getOperations()); + assertEquals(Map.of("user", "u"), new HashMap<>(sandbox)); + assertEquals("1", sandbox.getSnapshotValue("a")); + } + + public void testPutNullIsLoggedAsRemove() { + SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "p"); + sandbox.put("a", null); + assertEquals(List.of(new SessionOperation.Remove("a")), sandbox.getOperations()); + } + + public void testEntrySetValueIsLogged() { + MockHttpSession live = new MockHttpSession(); + live.setAttribute("a", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + + sandbox.entrySet().iterator().next().setValue("2"); + + assertEquals("2", sandbox.get("a")); + assertEquals(List.of(new SessionOperation.Put("a", "2")), sandbox.getOperations()); + } + + public void testClearLogsOneRemovePerKeyPresent() { + MockHttpSession live = new MockHttpSession(); + live.setAttribute("a", "1"); + live.setAttribute("b", "2"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + + sandbox.clear(); + + assertTrue(sandbox.isEmpty()); + assertEquals(2, sandbox.getOperations().size()); + assertTrue(sandbox.getOperations().contains(new SessionOperation.Remove("a"))); + assertTrue(sandbox.getOperations().contains(new SessionOperation.Remove("b"))); + assertEquals("1", live.getAttribute("a")); + } +} From 456144416b23e4802547815c7686403e12c05314 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 09:08:46 +0200 Subject: [PATCH 09/27] WW-3576 feat(core): reconcile a session sandbox with per-action conflict resolution Co-Authored-By: Claude Opus 5.5 --- .../BackgroundSessionConflictAware.java | 37 ++++ .../struts2/action/SessionConflict.java | 31 +++ .../action/SessionConflictResolution.java | 80 ++++++++ .../exec/SessionSandboxReconciler.java | 113 +++++++++++ .../exec/SessionSandboxReconcilerTest.java | 181 ++++++++++++++++++ 5 files changed, 442 insertions(+) create mode 100644 core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java create mode 100644 core/src/main/java/org/apache/struts2/action/SessionConflict.java create mode 100644 core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java create mode 100644 core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java create mode 100644 core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java diff --git a/core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java b/core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java new file mode 100644 index 0000000000..adfd0f2fb3 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java @@ -0,0 +1,37 @@ +/* + * 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.action; + +/** + * Actions run by the execAndWait interceptor that want to decide how a session key is reconciled when the + * background process wrote it while another request changed it should implement this interface. + * + * Without it, the value the session holds at reconciliation is kept and the background's write is dropped. + */ +public interface BackgroundSessionConflictAware { + + /** + * Decides the value of one conflicting session key. Called once per key, on the polling request. + * + * @param conflict the key with its snapshot, current and background values. + * @return how the key is reconciled. + */ + SessionConflictResolution resolveSessionConflict(SessionConflict conflict); + +} diff --git a/core/src/main/java/org/apache/struts2/action/SessionConflict.java b/core/src/main/java/org/apache/struts2/action/SessionConflict.java new file mode 100644 index 0000000000..5af7ccaaea --- /dev/null +++ b/core/src/main/java/org/apache/struts2/action/SessionConflict.java @@ -0,0 +1,31 @@ +/* + * 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.action; + +/** + * A session key the execAndWait background process wrote while another request changed it. A {@code null} + * value means the key was absent. + * + * @param key the session attribute name. + * @param snapshotValue the value when the background process started. + * @param currentValue the value the session holds now. + * @param backgroundValue the value the background process left, {@code null} if it removed the key. + */ +public record SessionConflict(String key, Object snapshotValue, Object currentValue, Object backgroundValue) { +} diff --git a/core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java b/core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java new file mode 100644 index 0000000000..cf3e1bef6e --- /dev/null +++ b/core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java @@ -0,0 +1,80 @@ +/* + * 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.action; + +/** + * How a {@link BackgroundSessionConflictAware} action reconciles one conflicting session key. + */ +public final class SessionConflictResolution { + + private static final SessionConflictResolution KEEP_CURRENT = new SessionConflictResolution(Kind.KEEP_CURRENT, null); + private static final SessionConflictResolution USE_BACKGROUND = new SessionConflictResolution(Kind.USE_BACKGROUND, null); + private static final SessionConflictResolution REMOVE = new SessionConflictResolution(Kind.REPLACE, null); + + private enum Kind { KEEP_CURRENT, USE_BACKGROUND, REPLACE } + + private final Kind kind; + private final Object replacement; + + private SessionConflictResolution(Kind kind, Object replacement) { + this.kind = kind; + this.replacement = replacement; + } + + /** + * @return a resolution keeping the value the session holds now. + */ + public static SessionConflictResolution keepCurrent() { + return KEEP_CURRENT; + } + + /** + * @return a resolution taking the background process's value, removing the key if it removed it. + */ + public static SessionConflictResolution useBackground() { + return USE_BACKGROUND; + } + + /** + * @param value the value to store, {@code null} removes the key. + * @return a resolution storing the given value. + */ + public static SessionConflictResolution replaceWith(Object value) { + return new SessionConflictResolution(Kind.REPLACE, value); + } + + /** + * @return a resolution removing the key. + */ + public static SessionConflictResolution remove() { + return REMOVE; + } + + /** + * @param conflict the conflict this resolution answers. + * @return the value the key holds after reconciliation, {@code null} meaning removed. + */ + public Object resolve(SessionConflict conflict) { + return switch (kind) { + case KEEP_CURRENT -> conflict.currentValue(); + case USE_BACKGROUND -> conflict.backgroundValue(); + case REPLACE -> replacement; + }; + } +} diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java new file mode 100644 index 0000000000..0f76f283b2 --- /dev/null +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java @@ -0,0 +1,113 @@ +/* + * 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.interceptor.exec; + +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpSession; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; +import org.apache.struts2.action.BackgroundSessionConflictAware; +import org.apache.struts2.action.SessionConflict; + +import java.util.LinkedHashSet; +import java.util.List; +import java.util.Objects; +import java.util.Set; + +/** + * Applies the writes an execAndWait background process logged in its {@link SandboxedSessionMap} to the + * session of the polling request. Writes are discarded when that session is not the one the process started + * with. A key changed elsewhere since the snapshot keeps its current value unless the action implements + * {@link BackgroundSessionConflictAware}. + */ +public final class SessionSandboxReconciler { + + private static final Logger LOG = LogManager.getLogger(SessionSandboxReconciler.class); + + private SessionSandboxReconciler() { + } + + public static void reconcile(SandboxedSessionMap sandbox, HttpServletRequest request, Object action) { + HttpSession real = request.getSession(false); + if (real == null || !real.getId().equals(sandbox.getStartSessionId())) { + LOG.debug("Discarding the session writes of background process [{}]: its session is gone", sandbox.getProcessName()); + return; + } + List operations = sandbox.getOperations(); + int lastInvalidate = lastInvalidateIndex(operations); + if (lastInvalidate >= 0) { + real.invalidate(); + HttpSession fresh = request.getSession(true); + for (String key : touchedKeys(operations.subList(lastInvalidate + 1, operations.size()))) { + write(fresh, key, sandbox.get(key)); + } + return; + } + for (String key : touchedKeys(operations)) { + reconcileKey(sandbox, real, key, action); + } + } + + private static void reconcileKey(SandboxedSessionMap sandbox, HttpSession real, String key, Object action) { + Object current = real.getAttribute(key); + Object snapshot = sandbox.getSnapshotValue(key); + Object background = sandbox.get(key); + if (Objects.equals(current, snapshot)) { + write(real, key, background); + } else if (action instanceof BackgroundSessionConflictAware aware) { + SessionConflict conflict = new SessionConflict(key, snapshot, current, background); + Object resolved = aware.resolveSessionConflict(conflict).resolve(conflict); + if (resolved != current) { + write(real, key, resolved); + } + } else { + LOG.debug("Background process [{}] keeps the current value of session key [{}] changed by another request", + sandbox.getProcessName(), key); + } + } + + private static int lastInvalidateIndex(List operations) { + for (int i = operations.size() - 1; i >= 0; i--) { + if (operations.get(i) instanceof SessionOperation.Invalidate) { + return i; + } + } + return -1; + } + + private static Set touchedKeys(List operations) { + Set keys = new LinkedHashSet<>(); + for (SessionOperation operation : operations) { + if (operation instanceof SessionOperation.Put put) { + keys.add(put.key()); + } else if (operation instanceof SessionOperation.Remove removal) { + keys.add(removal.key()); + } + } + return keys; + } + + private static void write(HttpSession session, String key, Object value) { + if (value == null) { + session.removeAttribute(key); + } else { + session.setAttribute(key, value); + } + } +} diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java new file mode 100644 index 0000000000..9d9dcd4d7f --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java @@ -0,0 +1,181 @@ +/* + * 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.interceptor.exec; + +import jakarta.servlet.http.HttpSession; +import junit.framework.TestCase; +import org.apache.struts2.action.BackgroundSessionConflictAware; +import org.apache.struts2.action.SessionConflict; +import org.apache.struts2.action.SessionConflictResolution; +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpSession; + +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.function.Function; + +public class SessionSandboxReconcilerTest extends TestCase { + + private MockHttpSession live; + private MockHttpServletRequest request; + + @Override + protected void setUp() throws Exception { + super.setUp(); + live = new MockHttpSession(); + request = new MockHttpServletRequest(); + request.setSession(live); + } + + public void testOnlyLoggedKeysAreApplied() { + live.setAttribute("other", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + live.setAttribute("other", "changed"); + sandbox.put("b", "2"); + + SessionSandboxReconciler.reconcile(sandbox, request, new Object()); + + assertEquals("changed", live.getAttribute("other")); + assertEquals("2", live.getAttribute("b")); + } + + public void testSessionIdChangeDiscardsEverything() { + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + sandbox.put("b", "2"); + live.invalidate(); + HttpSession fresh = request.getSession(true); + + SessionSandboxReconciler.reconcile(sandbox, request, new Object()); + + assertNull(fresh.getAttribute("b")); + } + + public void testNoSessionDiscardsEverything() { + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + sandbox.put("b", "2"); + live.invalidate(); + + SessionSandboxReconciler.reconcile(sandbox, request, new Object()); + + assertNull(request.getSession(false)); + } + + public void testConflictKeepsCurrentByDefault() { + SandboxedSessionMap sandbox = conflictOnA(); + + SessionSandboxReconciler.reconcile(sandbox, request, new Object()); + + assertEquals("live", live.getAttribute("a")); + } + + public void testHookUseBackground() { + SandboxedSessionMap sandbox = conflictOnA(); + + SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> SessionConflictResolution.useBackground())); + + assertEquals("bg", live.getAttribute("a")); + } + + public void testHookReplaceWith() { + SandboxedSessionMap sandbox = conflictOnA(); + + SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> SessionConflictResolution.replaceWith("merged"))); + + assertEquals("merged", live.getAttribute("a")); + } + + public void testHookRemove() { + SandboxedSessionMap sandbox = conflictOnA(); + + SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> SessionConflictResolution.remove())); + + assertNull(live.getAttribute("a")); + } + + public void testHookSeesAllFourValues() { + SandboxedSessionMap sandbox = conflictOnA(); + List seen = new ArrayList<>(); + + SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { + seen.add(conflict); + return SessionConflictResolution.keepCurrent(); + })); + + assertEquals(List.of(new SessionConflict("a", "1", "live", "bg")), seen); + } + + public void testRemovalOfAKeyChangedElsewhereIsAConflict() { + live.setAttribute("a", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + live.setAttribute("a", "live"); + sandbox.remove("a"); + + SessionSandboxReconciler.reconcile(sandbox, request, new Object()); + + assertEquals("live", live.getAttribute("a")); + } + + public void testBackgroundInvalidateStartsAFreshSession() { + live.setAttribute("a", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + live.setAttribute("a", "live"); + sandbox.invalidate(); + sandbox.put("user", "u"); + List seen = new ArrayList<>(); + + SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { + seen.add(conflict); + return SessionConflictResolution.keepCurrent(); + })); + + HttpSession fresh = request.getSession(false); + assertTrue(live.isInvalid()); + assertNotSame(live, fresh); + assertEquals(List.of("user"), Collections.list(fresh.getAttributeNames())); + assertEquals("u", fresh.getAttribute("user")); + assertEquals(List.of(), seen); + } + + public void testHookExceptionPropagates() { + SandboxedSessionMap sandbox = conflictOnA(); + IllegalArgumentException thrown = new IllegalArgumentException("boom"); + + try { + SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { + throw thrown; + })); + fail("expected the hook's exception"); + } catch (IllegalArgumentException e) { + assertSame(thrown, e); + } + } + + private SandboxedSessionMap conflictOnA() { + live.setAttribute("a", "1"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + live.setAttribute("a", "live"); + sandbox.put("a", "bg"); + return sandbox; + } + + private static BackgroundSessionConflictAware resolving(Function resolver) { + return resolver::apply; + } +} From a573fc13e0e8532a3551b99b6d062fe22bc0efb5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 09:15:05 +0200 Subject: [PATCH 10/27] WW-3576 feat(core): run execAndWait background processes against a session sandbox Co-Authored-By: Claude Opus 5.5 --- .../ExecuteAndWaitInterceptor.java | 18 +++- .../interceptor/exec/BackgroundProcess.java | 8 ++ .../exec/StrutsBackgroundProcess.java | 26 ++++++ .../ExecuteAndWaitInterceptorTest.java | 82 +++++++++++++++++++ .../interceptor/SessionWritingWaitAction.java | 60 ++++++++++++++ 5 files changed, 193 insertions(+), 1 deletion(-) create mode 100644 core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java diff --git a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java index 5a073147fc..7572a6d4e7 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java @@ -22,6 +22,7 @@ import org.apache.struts2.ActionContext; import org.apache.struts2.ActionInvocation; import org.apache.struts2.ActionProxy; +import org.apache.struts2.action.SessionAware; import org.apache.struts2.config.entities.ResultConfig; import org.apache.struts2.inject.Container; import org.apache.struts2.inject.Inject; @@ -31,6 +32,8 @@ import org.apache.struts2.ServletActionContext; import org.apache.struts2.interceptor.exec.BackgroundProcess; import org.apache.struts2.interceptor.exec.ExecutorProvider; +import org.apache.struts2.interceptor.exec.SandboxedSessionMap; +import org.apache.struts2.interceptor.exec.SessionSandboxReconciler; import org.apache.struts2.interceptor.exec.StrutsBackgroundProcess; import org.apache.struts2.interceptor.exec.StrutsExecutorProvider; import org.apache.struts2.util.TokenHelper; @@ -295,7 +298,8 @@ protected String doIntercept(ActionInvocation actionInvocation) throws Exception if ((!executeAfterValidationPass || secondTime) && bp == null) { LOG.trace("BackgroundProcess instance is null (create new instance) - executeAfterValidationPass: {}, secondTime: {}.", executeAfterValidationPass, secondTime); - bp = getNewBackgroundProcess(name, actionInvocation, threadPriority).prepare(); + SandboxedSessionMap sandbox = new SandboxedSessionMap(httpSession, name); + bp = getNewBackgroundProcess(name, actionInvocation, threadPriority).withSessionSandbox(sandbox).prepare(); session.put(bp_SessionKey, bp); if (executor == null || executor.isShutdown()) { LOG.warn("Executor is shutting down (or null), cannot execute a new process, invoke next ActionInvocation step and return."); @@ -334,6 +338,7 @@ protected String doIntercept(ActionInvocation actionInvocation) throws Exception LOG.trace("BackgroundProcess instance is done (remove key, return result) - executeAfterValidationPass: {}, secondTime: {}.", executeAfterValidationPass, secondTime); session.remove(bp_SessionKey); actionInvocation.getStack().push(bp.getAction()); + writeBackSessionSandbox(bp, context); // if an exception occurred during action execution, throw it here if (bp.getException() != null) { @@ -352,6 +357,17 @@ protected String doIntercept(ActionInvocation actionInvocation) throws Exception } } + private void writeBackSessionSandbox(BackgroundProcess bp, ActionContext context) { + SandboxedSessionMap sandbox = bp.getSessionSandbox(); + if (sandbox == null) { + return; + } + SessionSandboxReconciler.reconcile(sandbox, ServletActionContext.getRequest(), bp.getAction()); + if (bp.getAction() instanceof SessionAware sessionAware) { + sessionAware.withSession(context.getSession()); + } + } + /** *

* Performs the initial delay. diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java index a9ca48d20c..7b1392ac8e 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java @@ -38,4 +38,12 @@ public interface BackgroundProcess extends Runnable { Exception getException(); boolean isDone(); + + default BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { + return this; + } + + default SandboxedSessionMap getSessionSandbox() { + return null; + } } 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..8c41c3a8a8 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 @@ -22,9 +22,11 @@ import org.apache.struts2.ActionInvocation; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import org.apache.struts2.action.SessionAware; import java.io.Serial; import java.io.Serializable; +import java.util.HashMap; /** * Background thread to be executed by the ExecuteAndWaitInterceptor. @@ -43,6 +45,7 @@ public class StrutsBackgroundProcess implements BackgroundProcess, Serializable //WW-4900 transient since 2.5.15 protected transient ActionInvocation invocation; protected transient Exception exception; + private transient SandboxedSessionMap sessionSandbox; protected String result; protected boolean done; @@ -66,6 +69,7 @@ public BackgroundProcess prepare() { processThread = new Thread(() -> { try { beforeInvocation(); + installSessionSandbox(); result = invocation.invokeActionOnly(); } catch (Exception e) { LOG.warn("Exception during invokeActionOnly() execution", e); @@ -91,6 +95,28 @@ public BackgroundProcess prepare() { return this; } + @Override + public BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { + this.sessionSandbox = sandbox; + return this; + } + + @Override + public SandboxedSessionMap getSessionSandbox() { + return sessionSandbox; + } + + private void installSessionSandbox() { + if (sessionSandbox == null) { + return; + } + ActionContext.bind(ActionContext.of(new HashMap<>(invocation.getInvocationContext().getContextMap())) + .withSession(sessionSandbox)); + if (invocation.getAction() instanceof SessionAware sessionAware) { + sessionAware.withSession(sessionSandbox); + } + } + @Override public void run() { if (processThread == null) { diff --git a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java index 7701173ebf..5216a65641 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java @@ -39,10 +39,14 @@ import jakarta.servlet.http.HttpSession; import org.apache.struts2.StrutsInternalTestCase; import org.apache.struts2.dispatcher.HttpParameters; +import org.apache.struts2.dispatcher.SessionMap; +import org.apache.struts2.interceptor.exec.BackgroundProcess; import org.apache.struts2.interceptor.exec.ExecutorProvider; +import org.apache.struts2.interceptor.exec.SandboxedSessionMap; import org.apache.struts2.interceptor.parameter.ParametersInterceptor; import org.apache.struts2.views.jsp.StrutsMockHttpServletRequest; import org.apache.struts2.views.jsp.StrutsMockHttpSession; +import org.springframework.mock.web.MockHttpSession; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -67,6 +71,7 @@ public class ExecuteAndWaitInterceptorTest extends StrutsInternalTestCase { private ParametersInterceptor parametersInterceptor; private TokenInterceptor tokenInterceptor; private TokenSessionStoreInterceptor tokenSessionInterceptor; + private ServletConfigInterceptor servletConfigInterceptor; public void testOneWait() throws Exception { waitInterceptor.setDelay(0); @@ -275,6 +280,74 @@ private void assertWaitPageRefreshAccepted(String actionName) throws Exception { assertNull(session.get(tokenSessionName)); } + public void testBackgroundWritesReachTheRealSession() throws Exception { + useLiveSession(); + + assertEquals("wait", buildProxy("sessionAction").execute()); + awaitDone("sessionAction"); + + assertEquals("success", buildProxy("sessionAction").execute()); + assertEquals("done", httpSession.getAttribute("result")); + assertEquals("yes", httpSession.getAttribute("aware")); + } + + public void testBackgroundWritesAreInvisibleUntilPolled() throws Exception { + useLiveSession(); + + assertEquals("wait", buildProxy("sessionAction").execute()); + awaitDone("sessionAction"); + + assertNull(httpSession.getAttribute("result")); + assertNull(httpSession.getAttribute("aware")); + } + + public void testWritesOfAFailedActionAreApplied() throws Exception { + useLiveSession(); + Map requestParams = new HashMap<>(); + requestParams.put("failWith", new String[]{"boom"}); + withRequestParams(requestParams); + + assertEquals("wait", buildProxy("sessionAction").execute()); + awaitDone("sessionAction"); + + ActionProxy poll = buildProxy("sessionAction"); + try { + poll.execute(); + fail("the background failure should be rethrown by the polling request"); + } catch (IllegalStateException failure) { + assertEquals("boom", failure.getMessage()); + } + assertEquals("done", httpSession.getAttribute("result")); + } + + public void testSessionAwareGetsTheLiveMapBackAfterWriteBack() throws Exception { + useLiveSession(); + + assertEquals("wait", buildProxy("sessionAction").execute()); + SessionWritingWaitAction action = (SessionWritingWaitAction) awaitDone("sessionAction").getAction(); + + assertEquals("success", buildProxy("sessionAction").execute()); + assertFalse(action.getSession() instanceof SandboxedSessionMap); + assertSame(session, action.getSession()); + } + + private void useLiveSession() { + httpSession = new MockHttpSession(); + request.setSession(httpSession); + session = new SessionMap(request); + context = ActionContext.of(context).withSession(session).getContextMap(); + } + + private BackgroundProcess awaitDone(String actionName) throws InterruptedException { + BackgroundProcess process = (BackgroundProcess) httpSession.getAttribute(ExecuteAndWaitInterceptor.KEY + actionName); + long deadline = System.currentTimeMillis() + 5000; + while (!process.isDone() && System.currentTimeMillis() < deadline) { + Thread.sleep(20); + } + assertTrue("background process did not finish in time", process.isDone()); + return process; + } + private void withRequestParams(Map requestParams) { context = ActionContext.of(context).withParameters(HttpParameters.create(requestParams).build()).getContextMap(); } @@ -306,6 +379,7 @@ protected void setUp() throws Exception { container.inject(parametersInterceptor); container.inject(tokenInterceptor); container.inject(tokenSessionInterceptor); + container.inject(servletConfigInterceptor); waitInterceptor.init(); parametersInterceptor.init(); @@ -339,6 +413,7 @@ public void loadPackages() throws ConfigurationException { parametersInterceptor = new ParametersInterceptor(); tokenInterceptor = new TokenInterceptor(); tokenSessionInterceptor = new TokenSessionStoreInterceptor(); + servletConfigInterceptor = new ServletConfigInterceptor(); PackageConfig wait = new PackageConfig.Builder("") .addActionConfig("action1", new ActionConfig.Builder("", "action1", ExecuteAndWaitDelayAction.class.getName()) .addResultConfig(new ResultConfig.Builder(Action.SUCCESS, MockResult.class.getName()).build()) @@ -347,6 +422,13 @@ public void loadPackages() throws ConfigurationException { .addInterceptor(new InterceptorMapping("execAndWait", waitInterceptor)) .build()) .addActionConfig("plainWait", waitActionConfig("plainWait")) + .addActionConfig("sessionAction", new ActionConfig.Builder("", "sessionAction", SessionWritingWaitAction.class.getName()) + .addResultConfig(new ResultConfig.Builder(Action.SUCCESS, MockResult.class.getName()).build()) + .addResultConfig(new ResultConfig.Builder(ExecuteAndWaitInterceptor.WAIT, MockResult.class.getName()).build()) + .addInterceptor(new InterceptorMapping("servletConfig", servletConfigInterceptor)) + .addInterceptor(new InterceptorMapping("params", parametersInterceptor)) + .addInterceptor(new InterceptorMapping("execAndWait", waitInterceptor)) + .build()) .addActionConfig("tokenWait", waitActionConfig("tokenWait", new InterceptorMapping("token", tokenInterceptor))) .addActionConfig("tokenSessionWait", waitActionConfig("tokenSessionWait", new InterceptorMapping("tokenSession", tokenSessionInterceptor))) .build(); diff --git a/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java b/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java new file mode 100644 index 0000000000..4610d3786e --- /dev/null +++ b/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java @@ -0,0 +1,60 @@ +/* + * 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.interceptor; + +import org.apache.struts2.ActionContext; +import org.apache.struts2.action.Action; +import org.apache.struts2.action.SessionAware; +import org.apache.struts2.interceptor.parameter.StrutsParameter; + +import java.util.Map; + +/** + * Used by ExecuteAndWaitInterceptorTest to write to the session from the background thread. + */ +public class SessionWritingWaitAction implements Action, SessionAware { + + private Map session; + private String failWith; + + @Override + public String execute() throws Exception { + Thread.sleep(200); + ActionContext.getContext().getSession().put("result", "done"); + session.put("aware", "yes"); + if (failWith != null) { + throw new IllegalStateException(failWith); + } + return SUCCESS; + } + + @Override + public void withSession(Map session) { + this.session = session; + } + + public Map getSession() { + return session; + } + + @StrutsParameter + public void setFailWith(String failWith) { + this.failWith = failWith; + } +} From c09388b5161004121dd299e4dd87245924476ae6 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 09:19:33 +0200 Subject: [PATCH 11/27] WW-3576 feat(core): warn when a background process writes around its session sandbox Co-Authored-By: Claude Opus 5.5 --- .../apache/struts2/dispatcher/SessionMap.java | 12 +++++++++ .../ExecuteAndWaitInterceptor.java | 10 +++++++ .../interceptor/exec/BackgroundProcess.java | 10 ++++++- .../interceptor/exec/SandboxedSessionMap.java | 27 +++++++++++++++++++ .../exec/StrutsBackgroundProcess.java | 21 ++++++++++++++- .../exec/SandboxedSessionMapTest.java | 27 +++++++++++++++++++ 6 files changed, 105 insertions(+), 2 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 d59ea0a79b..9f587b723e 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -20,6 +20,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpSession; +import org.apache.struts2.interceptor.exec.SandboxedSessionMap; import java.io.Serial; import java.io.Serializable; @@ -69,6 +70,7 @@ public SessionMap(final HttpServletRequest request) { * Invalidate the http session. */ public void invalidate() { + warnIfSandboxBypassed(); HttpSession current = session; if (current == null) { return; @@ -87,6 +89,7 @@ public void invalidate() { */ @Override public void clear() { + warnIfSandboxBypassed(); HttpSession current = session; if (current == null) { return; @@ -171,6 +174,7 @@ public Object get(final Object key) { */ @Override public Object put(final String key, final Object value) { + warnIfSandboxBypassed(); HttpSession current = session; if (current == null) { current = pin(); @@ -187,6 +191,13 @@ public Object put(final String key, final Object value) { return oldValue; } + private void warnIfSandboxBypassed() { + SandboxedSessionMap sandbox = SandboxedSessionMap.current(); + if (sandbox != null && !(this instanceof SandboxedSessionMap)) { + sandbox.warnEscapeOnce(); + } + } + private HttpSession pin() { synchronized (this) { HttpSession current = session; @@ -217,6 +228,7 @@ private void unpin(final HttpSession stale) { */ @Override public Object remove(final Object key) { + warnIfSandboxBypassed(); HttpSession current = session; if (current == null) { return null; diff --git a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java index 7572a6d4e7..3885d52fab 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java @@ -86,6 +86,16 @@ * rather than calling ActionContext.getSession(). *

* + *

The background process works on a session sandbox, a snapshot of the session taken when the process starts. Use + * ActionContext.getContext().getSession() or {@link SessionAware} to reach it. Its writes are applied to + * the real session by the polling request that sees the process finish, only if the session id is unchanged. + * Conflicts keep the real session's value by default and can be resolved per action with + * {@link org.apache.struts2.action.BackgroundSessionConflictAware}. OGNL #session and + * invocation.getInvocationContext().getSession() bypass the sandbox, write to the live session and log a + * warning. A custom {@link BackgroundProcess} that does not extend {@link StrutsBackgroundProcess}, or a subclass + * overriding prepare(), does not get a sandbox. + *

+ * *

The thread kicked off by this interceptor will be named in the form actionNameBackgroundProcess. * For example, the search action would run as a thread named searchBackgroundProcess. *

diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java index 7b1392ac8e..b17100b093 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java @@ -23,7 +23,15 @@ /** * Interface used to create a background process which will be executed by - * {@link ExecuteAndWaitInterceptor} + * {@link ExecuteAndWaitInterceptor}. + * + *

A process built on {@link StrutsBackgroundProcess} works on a session sandbox, a snapshot of the session taken + * when the process starts, and receives it through {@link #withSessionSandbox(SandboxedSessionMap)}. Its writes are + * applied to the real session by the polling request that sees the process finish, and only if the session id is + * unchanged; conflicts keep the real session's value unless the action implements + * {@link org.apache.struts2.action.BackgroundSessionConflictAware}. A custom implementation that does not extend + * {@link StrutsBackgroundProcess}, or a subclass overriding {@link StrutsBackgroundProcess#prepare()}, gets no + * sandbox.

*/ public interface BackgroundProcess extends Runnable { diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java index 32991ea553..e9e8b2619c 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java @@ -19,6 +19,8 @@ package org.apache.struts2.interceptor.exec; import jakarta.servlet.http.HttpSession; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.apache.struts2.dispatcher.SessionMap; import java.io.Serial; @@ -31,6 +33,7 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; /** * The session view an execAndWait background process works on: a shallow snapshot of the session taken at @@ -41,11 +44,15 @@ public class SandboxedSessionMap extends SessionMap { @Serial private static final long serialVersionUID = 2907451127739228064L; + private static final Logger LOG = LogManager.getLogger(SandboxedSessionMap.class); + private static final ThreadLocal CURRENT = new ThreadLocal<>(); + private final transient Map snapshot = new LinkedHashMap<>(); private final transient Map view = new LinkedHashMap<>(); private final transient List operations = new ArrayList<>(); private final transient String startSessionId; private final transient String processName; + private final transient AtomicBoolean escapeWarned = new AtomicBoolean(); public SandboxedSessionMap(HttpSession session, String processName) { super(); @@ -59,6 +66,26 @@ public SandboxedSessionMap(HttpSession session, String processName) { view.putAll(snapshot); } + public static SandboxedSessionMap current() { + return CURRENT.get(); + } + + public static void runInSandbox(SandboxedSessionMap sandbox, Runnable work) { + CURRENT.set(sandbox); + try { + work.run(); + } finally { + CURRENT.remove(); + } + } + + public void warnEscapeOnce() { + if (escapeWarned.compareAndSet(false, true)) { + LOG.warn("Background process [{}] wrote to the request session, bypassing its session sandbox; " + + "use ActionContext.getContext().getSession() or SessionAware", processName); + } + } + public String getStartSessionId() { return startSessionId; } 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 8c41c3a8a8..d05220a74c 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 @@ -70,7 +70,7 @@ public BackgroundProcess prepare() { try { beforeInvocation(); installSessionSandbox(); - result = invocation.invokeActionOnly(); + result = invokeAction(); } catch (Exception e) { LOG.warn("Exception during invokeActionOnly() execution", e); exception = e; @@ -106,6 +106,25 @@ public SandboxedSessionMap getSessionSandbox() { return sessionSandbox; } + private String invokeAction() throws Exception { + if (sessionSandbox == null) { + return invocation.invokeActionOnly(); + } + String[] outcome = new String[1]; + Exception[] failure = new Exception[1]; + SandboxedSessionMap.runInSandbox(sessionSandbox, () -> { + try { + outcome[0] = invocation.invokeActionOnly(); + } catch (Exception e) { + failure[0] = e; + } + }); + if (failure[0] != null) { + throw failure[0]; + } + return outcome[0]; + } + private void installSessionSandbox() { if (sessionSandbox == null) { return; diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java index e8269a3780..115ce1da2e 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java @@ -19,6 +19,9 @@ package org.apache.struts2.interceptor.exec; import junit.framework.TestCase; +import org.apache.struts2.dispatcher.SessionMap; +import org.apache.struts2.test.LogCapture; +import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpSession; import java.util.HashMap; @@ -89,4 +92,28 @@ public void testClearLogsOneRemovePerKeyPresent() { assertTrue(sandbox.getOperations().contains(new SessionOperation.Remove("b"))); assertEquals("1", live.getAttribute("a")); } + + public void testWriteThroughRequestMapFromBackgroundThreadWarnsOnce() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest(); + MockHttpSession live = new MockHttpSession(); + request.setSession(live); + SessionMap requestMap = new SessionMap(request); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "longJob"); + + try (LogCapture logs = new LogCapture(SandboxedSessionMap.class)) { + Thread background = new Thread(() -> SandboxedSessionMap.runInSandbox(sandbox, () -> { + requestMap.put("a", "1"); + requestMap.put("b", "2"); + sandbox.put("c", "3"); + })); + background.start(); + background.join(5000); + + assertEquals("1", live.getAttribute("a")); + assertEquals(1, logs.messages().size()); + assertTrue(logs.messages().get(0).contains("longJob")); + assertFalse(logs.messages().get(0).contains("1")); + } + assertNull(SandboxedSessionMap.current()); + } } From 04d8d18a58758228a6d4f2c93312f06067296849 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 10:26:34 +0200 Subject: [PATCH 12/27] WW-3576 docs: detach the request SessionMap at request end Co-Authored-By: Claude Opus 5.5 --- .../2026-10-18-WW-3576-session-map-staleness-design.md | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) 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 index faa2ff1be0..abf454b6cf 100644 --- 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 @@ -61,7 +61,14 @@ when that session dies (request threads), and one map that is stale *by design* - **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)` — the same path, and the same trust in the request, as a first `put` today. + `request.getSession(true)` — **only while the request is live**. +- **Detached at request end (decided after the final review):** the request cleanup in the dispatcher + marks the request's `SessionMap` detached. 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 (sandbox escape, custom + `BackgroundProcess`) reaching `request.getSession(true)` after the request was recycled would write + into another user's session. This also closes the same path for a never-pinned map used after its + request ended, which predates this change. 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 From 32212876dd54ccecc9702ba6201f64b0a26dba0f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 10:30:42 +0200 Subject: [PATCH 13/27] WW-3576 fix(core): detach the request SessionMap at request end A detached map never calls its request again: with a live pinned session it keeps working, and a put with no live pinned session throws IllegalStateException instead of resolving a session through a request that has already ended. Co-Authored-By: Claude Opus 5.5 --- .../struts2/dispatcher/PrepareOperations.java | 8 +++ .../apache/struts2/dispatcher/SessionMap.java | 10 +++- .../dispatcher/PrepareOperationsTest.java | 27 ++++++++++ .../struts2/dispatcher/SessionMapTest.java | 49 +++++++++++++++++++ 4 files changed, 93 insertions(+), 1 deletion(-) 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..95fa4bf92e 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 { + detachSessionMap(); ActionContext.clear(); Dispatcher.clearInstance(); ContainerHolder.clear(); @@ -83,6 +84,13 @@ public void cleanupRequest(final HttpServletRequest request) { }); } + 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 9f587b723e..3448ffffae 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -47,7 +47,7 @@ public class SessionMap extends AbstractMap implements Serializa @Deprecated(since = "7.5.0", forRemoval = true) protected Set> entries; protected HttpServletRequest request; - + private volatile boolean detached; protected SessionMap() { } @@ -66,6 +66,10 @@ public SessionMap(final HttpServletRequest request) { this.session = request.getSession(false); } + void detach() { + detached = true; + } + /** * Invalidate the http session. */ @@ -202,6 +206,10 @@ private HttpSession pin() { synchronized (this) { 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; } 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..902e7ee8cc 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,31 @@ 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 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 135ca1954f..3f7c3a8fc3 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java @@ -416,6 +416,55 @@ public void testPinnedSessionNeverTouchesTheRequestAgain() { 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); From 6fba74972a34c4598d4aca1fa006f8cf452f3b43 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 10:34:20 +0200 Subject: [PATCH 14/27] WW-3576 refactor(core): narrow the sandbox API and break the dispatcher cycle SessionMap no longer imports the sandbox: it owns a thread-local live-write listener that the sandbox sets while the background action runs, so current() and warnEscapeOnce() become private. The sandbox's operation log, snapshot accessors and runInSandbox (now Callable-based) are package-private, SessionOperation too; the remaining public members carry Javadoc. An entry's setValue now notifies the listener like put/remove/clear/invalidate. Co-Authored-By: Claude Opus 5.5 --- .../apache/struts2/dispatcher/SessionMap.java | 40 +++++++++++++---- .../interceptor/exec/BackgroundProcess.java | 12 +++++ .../interceptor/exec/SandboxedSessionMap.java | 45 +++++++++++-------- .../interceptor/exec/SessionOperation.java | 2 +- .../exec/SessionSandboxReconciler.java | 8 ++++ .../exec/StrutsBackgroundProcess.java | 14 +----- .../exec/SandboxedSessionMapTest.java | 43 ++++++++++++++---- 7 files changed, 115 insertions(+), 49 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 3448ffffae..801f9c3e30 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -20,7 +20,6 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpSession; -import org.apache.struts2.interceptor.exec.SandboxedSessionMap; import java.io.Serial; import java.io.Serializable; @@ -29,6 +28,7 @@ import java.util.Enumeration; import java.util.HashSet; import java.util.Set; +import java.util.concurrent.Callable; /** * A simple implementation of the {@link java.util.Map} interface to handle a collection of HTTP session @@ -40,6 +40,8 @@ public class SessionMap extends AbstractMap implements Serializa @Serial private static final long serialVersionUID = 4678843241638046854L; + private static final ThreadLocal LIVE_WRITE_LISTENER = new ThreadLocal<>(); + protected volatile HttpSession session; /** * @deprecated no longer read or written; {@link #entrySet()} builds a fresh set on every call. @@ -74,7 +76,7 @@ void detach() { * Invalidate the http session. */ public void invalidate() { - warnIfSandboxBypassed(); + notifyLiveWrite(); HttpSession current = session; if (current == null) { return; @@ -93,7 +95,7 @@ public void invalidate() { */ @Override public void clear() { - warnIfSandboxBypassed(); + notifyLiveWrite(); HttpSession current = session; if (current == null) { return; @@ -131,6 +133,7 @@ public Set> entrySet() { result.add(new StringObjectEntry(key, value) { @Override public Object setValue(final Object obj) { + notifyLiveWrite(); current.setAttribute(key, obj); return value; @@ -178,7 +181,7 @@ public Object get(final Object key) { */ @Override public Object put(final String key, final Object value) { - warnIfSandboxBypassed(); + notifyLiveWrite(); HttpSession current = session; if (current == null) { current = pin(); @@ -195,10 +198,29 @@ public Object put(final String key, final Object value) { return oldValue; } - private void warnIfSandboxBypassed() { - SandboxedSessionMap sandbox = SandboxedSessionMap.current(); - if (sandbox != null && !(this instanceof SandboxedSessionMap)) { - sandbox.warnEscapeOnce(); + /** + * Runs the work with a listener notified, on the calling thread, whenever a {@link SessionMap} writes to the live + * session. A subclass that overrides the write methods without calling them here never notifies it. + * + * @param listener notified on each live write made while the work runs + * @param work the work to run + * @param the type of the work's result + * @return the work's result + * @throws Exception whatever the work throws + */ + protected static T callWithLiveWriteListener(final Runnable listener, final Callable work) throws Exception { + LIVE_WRITE_LISTENER.set(listener); + try { + return work.call(); + } finally { + LIVE_WRITE_LISTENER.remove(); + } + } + + private static void notifyLiveWrite() { + Runnable listener = LIVE_WRITE_LISTENER.get(); + if (listener != null) { + listener.run(); } } @@ -236,7 +258,7 @@ private void unpin(final HttpSession stale) { */ @Override public Object remove(final Object key) { - warnIfSandboxBypassed(); + notifyLiveWrite(); HttpSession current = session; if (current == null) { return null; diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java index b17100b093..9a87e49588 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java @@ -47,10 +47,22 @@ public interface BackgroundProcess extends Runnable { boolean isDone(); + /** + * Hands the process the session sandbox to run against. Called on the request thread before {@link #prepare()}; + * the default ignores the sandbox. + * + * @param sandbox the snapshot of the request's session + * @return this process + */ default BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { return this; } + /** + * Returns the sandbox whose writes are applied to the session once the process is done. + * + * @return the sandbox, or {@code null} when the process runs without one + */ default SandboxedSessionMap getSessionSandbox() { return null; } diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java index e9e8b2619c..81a315da73 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java @@ -33,11 +33,13 @@ import java.util.List; import java.util.Map; import java.util.Set; +import java.util.concurrent.Callable; import java.util.concurrent.atomic.AtomicBoolean; /** * The session view an execAndWait background process works on: a shallow snapshot of the session taken at - * construction plus an ordered log of the process's own writes. It never touches the live session. + * construction plus an ordered log of the process's own writes. It never touches the live session; the writes are + * applied to it later by {@link SessionSandboxReconciler}. */ public class SandboxedSessionMap extends SessionMap { @@ -45,7 +47,6 @@ public class SandboxedSessionMap extends SessionMap { private static final long serialVersionUID = 2907451127739228064L; private static final Logger LOG = LogManager.getLogger(SandboxedSessionMap.class); - private static final ThreadLocal CURRENT = new ThreadLocal<>(); private final transient Map snapshot = new LinkedHashMap<>(); private final transient Map view = new LinkedHashMap<>(); @@ -54,6 +55,12 @@ public class SandboxedSessionMap extends SessionMap { private final transient String processName; private final transient AtomicBoolean escapeWarned = new AtomicBoolean(); + /** + * Snapshots the attributes of the given session. Must be called on the request thread that owns the session. + * + * @param session the live session to snapshot + * @param processName the name of the background process, used in log messages + */ public SandboxedSessionMap(HttpSession session, String processName) { super(); this.startSessionId = session.getId(); @@ -66,39 +73,30 @@ public SandboxedSessionMap(HttpSession session, String processName) { view.putAll(snapshot); } - public static SandboxedSessionMap current() { - return CURRENT.get(); + T runInSandbox(Callable work) throws Exception { + return callWithLiveWriteListener(this::warnEscapeOnce, work); } - public static void runInSandbox(SandboxedSessionMap sandbox, Runnable work) { - CURRENT.set(sandbox); - try { - work.run(); - } finally { - CURRENT.remove(); - } - } - - public void warnEscapeOnce() { + private void warnEscapeOnce() { if (escapeWarned.compareAndSet(false, true)) { LOG.warn("Background process [{}] wrote to the request session, bypassing its session sandbox; " + "use ActionContext.getContext().getSession() or SessionAware", processName); } } - public String getStartSessionId() { + String getStartSessionId() { return startSessionId; } - public String getProcessName() { + String getProcessName() { return processName; } - public synchronized List getOperations() { + synchronized List getOperations() { return Collections.unmodifiableList(new ArrayList<>(operations)); } - public synchronized Object getSnapshotValue(String key) { + synchronized Object getSnapshotValue(String key) { return snapshot.get(key); } @@ -107,6 +105,13 @@ public synchronized Object get(Object key) { return view.get(asKey(key)); } + /** + * Records the write in the sandbox; a {@code null} value is recorded as a removal. + * + * @param key the name of the session attribute + * @param value the value to set + * @return the previous value in the sandbox + */ @Override public synchronized Object put(String key, Object value) { if (value == null) { @@ -150,6 +155,10 @@ public synchronized void clear() { } } + /** + * Empties the sandbox and records the invalidation; the live session is invalidated only when the writes are + * applied. + */ @Override public synchronized void invalidate() { view.clear(); diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java index c079270404..342b701839 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java @@ -18,7 +18,7 @@ */ package org.apache.struts2.interceptor.exec; -public sealed interface SessionOperation { +sealed interface SessionOperation { record Put(String key, Object value) implements SessionOperation { } diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java index 0f76f283b2..f34a9462e6 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java @@ -43,6 +43,14 @@ public final class SessionSandboxReconciler { private SessionSandboxReconciler() { } + /** + * Applies the sandbox's writes to the session of the given request. Must be called on that request's thread. + * + * @param sandbox the sandbox the background process worked on + * @param request the polling request that saw the process finish + * @param action the background process's action, consulted when it implements + * {@link BackgroundSessionConflictAware} + */ public static void reconcile(SandboxedSessionMap sandbox, HttpServletRequest request, Object action) { HttpSession real = request.getSession(false); if (real == null || !real.getId().equals(sandbox.getStartSessionId())) { 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 d05220a74c..6ad0f29677 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 @@ -110,19 +110,7 @@ private String invokeAction() throws Exception { if (sessionSandbox == null) { return invocation.invokeActionOnly(); } - String[] outcome = new String[1]; - Exception[] failure = new Exception[1]; - SandboxedSessionMap.runInSandbox(sessionSandbox, () -> { - try { - outcome[0] = invocation.invokeActionOnly(); - } catch (Exception e) { - failure[0] = e; - } - }); - if (failure[0] != null) { - throw failure[0]; - } - return outcome[0]; + return sessionSandbox.runInSandbox(invocation::invokeActionOnly); } private void installSessionSandbox() { diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java index 115ce1da2e..c000da4ad3 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java @@ -27,6 +27,7 @@ import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.atomic.AtomicReference; public class SandboxedSessionMapTest extends TestCase { @@ -99,21 +100,47 @@ public void testWriteThroughRequestMapFromBackgroundThreadWarnsOnce() throws Exc request.setSession(live); SessionMap requestMap = new SessionMap(request); SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "longJob"); + SandboxedSessionMap finished = new SandboxedSessionMap(live, "finishedJob"); + AtomicReference failure = new AtomicReference<>(); try (LogCapture logs = new LogCapture(SandboxedSessionMap.class)) { - Thread background = new Thread(() -> SandboxedSessionMap.runInSandbox(sandbox, () -> { - requestMap.put("a", "1"); - requestMap.put("b", "2"); - sandbox.put("c", "3"); - })); + Thread background = new Thread(() -> { + try { + sandbox.runInSandbox(() -> { + requestMap.put("a", "s3cr3t"); + requestMap.put("b", "s3cr3t"); + sandbox.put("c", "3"); + return null; + }); + finished.runInSandbox(() -> null); + requestMap.put("d", "s3cr3t"); + } catch (Exception e) { + failure.set(e); + } + }); background.start(); background.join(5000); - assertEquals("1", live.getAttribute("a")); + assertNull(failure.get()); + assertEquals("s3cr3t", live.getAttribute("a")); + assertEquals("s3cr3t", live.getAttribute("d")); assertEquals(1, logs.messages().size()); assertTrue(logs.messages().get(0).contains("longJob")); - assertFalse(logs.messages().get(0).contains("1")); + assertFalse(logs.messages().get(0).contains("s3cr3t")); + } + } + + public void testRunInSandboxReturnsTheResultAndPropagatesFailures() throws Exception { + SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "p"); + + assertEquals("done", sandbox.runInSandbox(() -> "done")); + try { + sandbox.runInSandbox(() -> { + throw new IllegalStateException("boom"); + }); + fail("the work's failure should propagate"); + } catch (IllegalStateException expected) { + assertEquals("boom", expected.getMessage()); } - assertNull(SandboxedSessionMap.current()); } } From 52a21921e18fc8d96e7b4752f44960cbd4fc252e Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 10:34:27 +0200 Subject: [PATCH 15/27] WW-3576 fix(core): copy the background context on the request thread The background process's context map is copied when the sandbox is handed over, on the request thread, instead of on the background thread while the request may still be rendering the wait page. An OgnlContext is copied as an OgnlContext. A context bound by an overridden beforeInvocation() is honoured and copied on the background thread. done is volatile, since it gates the write-back. Co-Authored-By: Claude Opus 5.5 --- .../exec/StrutsBackgroundProcess.java | 30 +++++++++- .../exec/StrutsBackgroundProcessTest.java | 58 +++++++++++++++++++ 2 files changed, 85 insertions(+), 3 deletions(-) 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 6ad0f29677..5bf7e6a11c 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 @@ -18,6 +18,8 @@ */ package org.apache.struts2.interceptor.exec; +import ognl.Ognl; +import ognl.OgnlContext; import org.apache.struts2.ActionContext; import org.apache.struts2.ActionInvocation; import org.apache.logging.log4j.LogManager; @@ -27,6 +29,7 @@ import java.io.Serial; import java.io.Serializable; import java.util.HashMap; +import java.util.Map; /** * Background thread to be executed by the ExecuteAndWaitInterceptor. @@ -46,9 +49,10 @@ public class StrutsBackgroundProcess implements BackgroundProcess, Serializable protected transient ActionInvocation invocation; protected transient Exception exception; private transient SandboxedSessionMap sessionSandbox; + private transient Map sandboxContext; protected String result; - protected boolean done; + protected volatile boolean done; /** * Constructs a background process @@ -98,6 +102,7 @@ public BackgroundProcess prepare() { @Override public BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { this.sessionSandbox = sandbox; + this.sandboxContext = copyOf(invocation.getInvocationContext().getContextMap()); return this; } @@ -117,13 +122,32 @@ private void installSessionSandbox() { if (sessionSandbox == null) { return; } - ActionContext.bind(ActionContext.of(new HashMap<>(invocation.getInvocationContext().getContextMap())) - .withSession(sessionSandbox)); + ActionContext.bind(ActionContext.of(backgroundContextMap()).withSession(sessionSandbox)); if (invocation.getAction() instanceof SessionAware sessionAware) { sessionAware.withSession(sessionSandbox); } } + private Map backgroundContextMap() { + ActionContext bound = ActionContext.getContext(); + if (bound == null || bound.getContextMap() == invocation.getInvocationContext().getContextMap()) { + return sandboxContext; + } + return copyOf(bound.getContextMap()); + } + + private static Map copyOf(Map context) { + if (context instanceof OgnlContext ognlContext) { + OgnlContext copy = Ognl.createDefaultContext(ognlContext.getRoot(), ognlContext.getMemberAccess(), + ognlContext.getClassResolver(), ognlContext.getTypeConverter()); + copy.setTraceEvaluations(ognlContext.isTraceEvaluations()); + copy.setKeepLastEvaluation(ognlContext.isKeepLastEvaluation()); + copy.putAll(ognlContext.getValues()); + return copy; + } + return new HashMap<>(context); + } + @Override public void run() { if (processThread == null) { diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java index ad3409bd1b..39c30db57f 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java @@ -24,7 +24,9 @@ import java.io.ObjectOutputStream; import java.security.SecureRandom; import java.util.ArrayList; +import java.util.HashMap; import java.util.List; +import java.util.Map; import java.util.Random; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; @@ -34,9 +36,14 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; +import java.util.concurrent.atomic.AtomicReference; +import ognl.OgnlContext; import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.util.ValueStack; +import org.apache.struts2.util.ValueStackFactory; import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.mock.web.MockHttpSession; import org.apache.struts2.ActionContext; import org.apache.struts2.ActionInvocation; @@ -161,6 +168,57 @@ public void testUnpreparedProcess() throws ExecutionException, InterruptedExcept assertEquals("Background thread Unprepared has not been prepared!", bp.getException().getMessage()); } + public void testSandboxContextIsCopiedAtHandoverAndStaysAnOgnlContext() { + ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack(); + ActionContext requestContext = ActionContext.of(stack.getContext()).withSession(new HashMap<>()); + AtomicReference> seen = new AtomicReference<>(); + MockActionInvocationWithActionInvoker invocation = new MockActionInvocationWithActionInvoker(() -> { + seen.set(ActionContext.getContext().getContextMap()); + return "done"; + }); + invocation.setInvocationContext(requestContext); + SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "copy"); + + BackgroundProcess bp = new StrutsBackgroundProcess(invocation, "copy", Thread.NORM_PRIORITY) + .withSessionSandbox(sandbox) + .prepare(); + requestContext.put("afterHandover", "x"); + executor.execute(bp); + await().atMost(5, TimeUnit.SECONDS).until(bp::isDone); + + assertNull(bp.getException()); + Map background = seen.get(); + assertTrue(background instanceof OgnlContext); + assertNotSame(requestContext.getContextMap(), background); + assertFalse(background.containsKey("afterHandover")); + assertSame(sandbox, ActionContext.of(background).getSession()); + assertSame(stack, ActionContext.of(background).getValueStack()); + assertNotSame(sandbox, requestContext.getSession()); + } + + public void testSandboxHonoursTheContextBoundByBeforeInvocation() { + AtomicReference> seen = new AtomicReference<>(); + MockActionInvocationWithActionInvoker invocation = new MockActionInvocationWithActionInvoker(() -> { + seen.set(ActionContext.getContext().getContextMap()); + return "done"; + }); + invocation.setInvocationContext(ActionContext.of()); + SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "custom"); + + BackgroundProcess bp = new StrutsBackgroundProcess(invocation, "custom", Thread.NORM_PRIORITY) { + @Override + protected void beforeInvocation() { + ActionContext.bind(ActionContext.of().with("custom", "yes")); + } + }.withSessionSandbox(sandbox).prepare(); + executor.execute(bp); + await().atMost(5, TimeUnit.SECONDS).until(bp::isDone); + + assertNull(bp.getException()); + assertEquals("yes", seen.get().get("custom")); + assertSame(sandbox, ActionContext.of(seen.get()).getSession()); + } + private static class MockActionInvocationWithActionInvoker extends MockActionInvocation { private final Callable actionInvoker; From 7160db170852ab9be8cd694786fea56823ff63cf Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 10:34:27 +0200 Subject: [PATCH 16/27] WW-3576 docs: drop the stale execAndWait ActionContext warning The removed paragraph contradicted the sandbox paragraph. The invisibility test now fails when the background action failed or wrote nothing. Co-Authored-By: Claude Opus 5.5 --- .../struts2/interceptor/ExecuteAndWaitInterceptor.java | 5 ----- .../struts2/interceptor/ExecuteAndWaitInterceptorTest.java | 4 +++- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java index 3885d52fab..44f5212a8c 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java @@ -81,11 +81,6 @@ * millis we ensure the user is presented fast search results immediately and for the slow results a wait page is used. *

* - *

Important: Because the action will be running in a separate thread, you can't use ActionContext because it - * is a ThreadLocal. This means if you need to access, for example, session data, you need to implement SessionAware - * rather than calling ActionContext.getSession(). - *

- * *

The background process works on a session sandbox, a snapshot of the session taken when the process starts. Use * ActionContext.getContext().getSession() or {@link SessionAware} to reach it. Its writes are applied to * the real session by the polling request that sees the process finish, only if the session id is unchanged. diff --git a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java index 5216a65641..3e429b8e93 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java @@ -295,8 +295,10 @@ public void testBackgroundWritesAreInvisibleUntilPolled() throws Exception { useLiveSession(); assertEquals("wait", buildProxy("sessionAction").execute()); - awaitDone("sessionAction"); + BackgroundProcess process = awaitDone("sessionAction"); + assertNull(process.getException()); + assertEquals("done", process.getSessionSandbox().get("result")); assertNull(httpSession.getAttribute("result")); assertNull(httpSession.getAttribute("aware")); } From e79f945762b4daab2c43937cfcef326f808b1aaf Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 12:27:09 +0200 Subject: [PATCH 17/27] WW-3576 fix(core): detach every request SessionMap at request end Co-Authored-By: Claude Opus 5.5 --- .../struts2/dispatcher/PrepareOperations.java | 1 + .../apache/struts2/dispatcher/SessionMap.java | 30 ++++++++++++++++++ .../dispatcher/PrepareOperationsTest.java | 31 +++++++++++++++++++ .../struts2/dispatcher/SessionMapTest.java | 2 ++ .../CreateSessionInterceptorTest.java | 3 ++ .../debugging/DebuggingInterceptorTest.java | 12 +++++-- 6 files changed, 77 insertions(+), 2 deletions(-) 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 95fa4bf92e..d9398e3010 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 { + SessionMap.detachAll(request); detachSessionMap(); ActionContext.clear(); Dispatcher.clearInstance(); 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 801f9c3e30..d7346a22c1 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -29,6 +29,8 @@ import java.util.HashSet; import java.util.Set; import java.util.concurrent.Callable; +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 @@ -40,6 +42,8 @@ public class SessionMap extends AbstractMap implements Serializa @Serial private static final long serialVersionUID = 4678843241638046854L; + private static final String REQUEST_MAPS = SessionMap.class.getName() + ".maps"; + private static final ThreadLocal LIVE_WRITE_LISTENER = new ThreadLocal<>(); protected volatile HttpSession session; @@ -66,12 +70,38 @@ 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. */ 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 902e7ee8cc..b01b87cad5 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java @@ -75,6 +75,37 @@ public void testRequestCleanupDetachesTheContextSessionMap() { 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 testRequestCleanupWithoutActionContext() { 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 3f7c3a8fc3..f1bb72edc6 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java @@ -471,6 +471,8 @@ protected void setUp() throws Exception { 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; From aa6277d75631c9c7d3c5232cf6be3ade3b0abded Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 12:27:09 +0200 Subject: [PATCH 18/27] WW-3576 docs: request end detaches every SessionMap built for the request Co-Authored-By: Claude Opus 5.5 --- .../specs/2026-10-18-WW-3576-session-map-staleness-design.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 index abf454b6cf..2fc7a9de03 100644 --- 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 @@ -63,7 +63,9 @@ when that session dies (request threads), and one map that is stale *by design* (`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 - marks the request's `SessionMap` detached. A detached map never calls its request again: with a live + 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. 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 (sandbox escape, custom `BackgroundProcess`) reaching `request.getSession(true)` after the request was recycled would write From 8b1b440e89c038d32b440b49115b8ec0ea4f597f Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 12:33:38 +0200 Subject: [PATCH 19/27] WW-3576 fix(core): keep request SessionMaps attached while async is started Co-Authored-By: Claude Opus 5.5 --- .../struts2/dispatcher/PrepareOperations.java | 4 ++- .../dispatcher/PrepareOperationsTest.java | 29 +++++++++++++++++++ ...18-WW-3576-session-map-staleness-design.md | 3 +- 3 files changed, 34 insertions(+), 2 deletions(-) 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 d9398e3010..4d07c211ae 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/PrepareOperations.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/PrepareOperations.java @@ -75,7 +75,9 @@ public void cleanupRequest(final HttpServletRequest request) { try { dispatcher.cleanUpRequest(request); } finally { - SessionMap.detachAll(request); + if (!request.isAsyncStarted()) { + SessionMap.detachAll(request); + } detachSessionMap(); ActionContext.clear(); Dispatcher.clearInstance(); 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 b01b87cad5..105017f0ee 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java @@ -106,6 +106,35 @@ public void testRequestCleanupDetachesEveryRequestSessionMap() { 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 testRequestCleanupWithoutActionContext() { HttpServletRequest req = new MockHttpServletRequest(); PrepareOperations prepare = new PrepareOperations(dispatcher); 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 index 2fc7a9de03..fbaaaebd46 100644 --- 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 @@ -65,7 +65,8 @@ when that session dies (request threads), and one map that is stale *by design* - **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. A detached map never calls its request again: with a live + `createSession` replaces `ActionContext.SESSION`, and the maps of a nested forward. Detach is deferred while the request is in async mode, until 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 (sandbox escape, custom `BackgroundProcess`) reaching `request.getSession(true)` after the request was recycled would write From d313368985f2962494bbe16378f365a791b14ef5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 12:37:46 +0200 Subject: [PATCH 20/27] WW-3576 fix(core): keep the bound SessionMap attached while async is started Co-Authored-By: Claude Opus 5.5 --- .../struts2/dispatcher/PrepareOperations.java | 12 ++++--- .../dispatcher/PrepareOperationsTest.java | 32 +++++++++++++++++++ ...18-WW-3576-session-map-staleness-design.md | 2 +- 3 files changed, 41 insertions(+), 5 deletions(-) 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 4d07c211ae..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,10 +75,7 @@ public void cleanupRequest(final HttpServletRequest request) { try { dispatcher.cleanUpRequest(request); } finally { - if (!request.isAsyncStarted()) { - SessionMap.detachAll(request); - } - detachSessionMap(); + detachSessionMaps(request); ActionContext.clear(); Dispatcher.clearInstance(); ContainerHolder.clear(); @@ -87,6 +84,13 @@ 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) { 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 105017f0ee..2b320cbe01 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/PrepareOperationsTest.java @@ -135,6 +135,38 @@ public void testAsyncDispatchCleanupDetachesTheSessionMapsKeptAttached() { 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); 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 index fbaaaebd46..0e79e03cd1 100644 --- 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 @@ -65,7 +65,7 @@ when that session dies (request threads), and one map that is stale *by design* - **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. Detach is deferred while the request is in async mode, until the + `createSession` replaces `ActionContext.SESSION`, and the maps of a nested forward. Detach (of all request maps, including the bound context's) is deferred while the request is in async mode, until 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 (sandbox escape, custom From 8bd88a7c0a1f063dba351d793ff4a08b36ef982e Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 12:38:02 +0200 Subject: [PATCH 21/27] WW-3576 docs: reflow the async detach clause Co-Authored-By: Claude Opus 5.5 --- .../2026-10-18-WW-3576-session-map-staleness-design.md | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) 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 index 0e79e03cd1..07c8654cf8 100644 --- 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 @@ -65,10 +65,11 @@ when that session dies (request threads), and one map that is stale *by design* - **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. Detach (of all request maps, including the bound context's) is deferred while the request is in async mode, until 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 (sandbox escape, custom + `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 (sandbox escape, custom `BackgroundProcess`) reaching `request.getSession(true)` after the request was recycled would write into another user's session. This also closes the same path for a never-pinned map used after its request ended, which predates this change. Detach is package-private. From fb355c30c9ad4fca12d7c14e782a3631f2982db7 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 12:50:29 +0200 Subject: [PATCH 22/27] WW-3576 docs: tighten the detach rationale wording Co-Authored-By: Claude Opus 5.5 --- .../2026-10-18-WW-3576-session-map-staleness-design.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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 index 07c8654cf8..08dadb2fd8 100644 --- 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 @@ -57,7 +57,7 @@ when that session dies (request threads), and one map that is stale *by design* - **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 through a recycled request, which may be another user's. + `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 @@ -70,9 +70,8 @@ when that session dies (request threads), and one map that is stale *by design* 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 (sandbox escape, custom - `BackgroundProcess`) reaching `request.getSession(true)` after the request was recycled would write - into another user's session. This also closes the same path for a never-pinned map used after its - request ended, which predates this change. Detach is package-private. + `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 From fd5988af8cb17be9ee78e86c4f6e2d312dd28a52 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 13:01:16 +0200 Subject: [PATCH 23/27] WW-3576 chore(core): address Sonar findings on the SessionMap changes Suppress S3077 on the pinned session field, as for the localized-text caches: volatile only publishes the reference. Drop a redundant import and poll with Awaitility instead of Thread.sleep in the new tests. Co-Authored-By: Claude Opus 5.5 --- .../java/org/apache/struts2/dispatcher/SessionMap.java | 2 ++ .../struts2/interceptor/exec/SandboxedSessionMap.java | 1 - .../org/apache/struts2/dispatcher/SessionMapTest.java | 10 +++++----- .../interceptor/ExecuteAndWaitInterceptorTest.java | 9 ++++----- 4 files changed, 11 insertions(+), 11 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 d7346a22c1..b43630d559 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -46,6 +46,8 @@ public class SessionMap extends AbstractMap implements Serializa private static final ThreadLocal LIVE_WRITE_LISTENER = new ThreadLocal<>(); + // volatile only publishes the pinned reference; HttpSession itself is thread-safe per the Servlet spec + @SuppressWarnings("java:S3077") protected volatile HttpSession session; /** * @deprecated no longer read or written; {@link #entrySet()} builds a fresh set on every call. diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java index 81a315da73..8c82745a4d 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java @@ -24,7 +24,6 @@ import org.apache.struts2.dispatcher.SessionMap; import java.io.Serial; -import java.util.AbstractMap.SimpleEntry; import java.util.ArrayList; import java.util.Collections; import java.util.Enumeration; 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 f1bb72edc6..a925ca74db 100644 --- a/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/dispatcher/SessionMapTest.java @@ -25,6 +25,7 @@ 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; @@ -41,6 +42,8 @@ import com.mockobjects.dynamic.Mock; import java.util.AbstractMap; +import static org.awaitility.Awaitility.await; + /** */ @@ -305,11 +308,8 @@ public void invalidate() { try { read.set(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); - } + await().atMost(5, TimeUnit.SECONDS) + .until(() -> reader.getState() == Thread.State.BLOCKED || reader.getState() == Thread.State.TERMINATED); release.countDown(); invalidator.join(5000); reader.join(5000); diff --git a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java index 3e429b8e93..2757f1e65e 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java @@ -56,6 +56,9 @@ import java.util.Map; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; +import java.util.concurrent.TimeUnit; + +import static org.awaitility.Awaitility.await; /** * Test case for ExecuteAndWaitInterceptor. @@ -342,11 +345,7 @@ private void useLiveSession() { private BackgroundProcess awaitDone(String actionName) throws InterruptedException { BackgroundProcess process = (BackgroundProcess) httpSession.getAttribute(ExecuteAndWaitInterceptor.KEY + actionName); - long deadline = System.currentTimeMillis() + 5000; - while (!process.isDone() && System.currentTimeMillis() < deadline) { - Thread.sleep(20); - } - assertTrue("background process did not finish in time", process.isDone()); + await().atMost(5, TimeUnit.SECONDS).until(process::isDone); return process; } From 0c1cc429e7213eacff62b32635f0a80c5009f816 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 14:13:00 +0200 Subject: [PATCH 24/27] WW-3576 chore(core): resolve the remaining Sonar findings BackgroundProcess.getSessionSandbox() returns Optional instead of null (the method is new in this change, so nothing depends on the null contract). Suppress S1948 on SessionMap.session (non-transient as before), S2160 on SandboxedSessionMap (Map equality by contents) and S2925 on the test action's simulated work; drop an exception the polling helper no longer throws. Co-Authored-By: Claude Opus 5.5 --- .../org/apache/struts2/dispatcher/SessionMap.java | 5 +++-- .../interceptor/ExecuteAndWaitInterceptor.java | 14 ++++++-------- .../interceptor/exec/BackgroundProcess.java | 8 +++++--- .../interceptor/exec/SandboxedSessionMap.java | 1 + .../interceptor/exec/StrutsBackgroundProcess.java | 5 +++-- .../interceptor/ExecuteAndWaitInterceptorTest.java | 4 ++-- .../interceptor/SessionWritingWaitAction.java | 1 + 7 files changed, 21 insertions(+), 17 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 b43630d559..5e4c7d2bd2 100644 --- a/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java +++ b/core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java @@ -46,8 +46,9 @@ public class SessionMap extends AbstractMap implements Serializa private static final ThreadLocal LIVE_WRITE_LISTENER = new ThreadLocal<>(); - // volatile only publishes the pinned reference; HttpSession itself is thread-safe per the Servlet spec - @SuppressWarnings("java:S3077") + // 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. diff --git a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java index 44f5212a8c..6f31e63db5 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java @@ -363,14 +363,12 @@ protected String doIntercept(ActionInvocation actionInvocation) throws Exception } private void writeBackSessionSandbox(BackgroundProcess bp, ActionContext context) { - SandboxedSessionMap sandbox = bp.getSessionSandbox(); - if (sandbox == null) { - return; - } - SessionSandboxReconciler.reconcile(sandbox, ServletActionContext.getRequest(), bp.getAction()); - if (bp.getAction() instanceof SessionAware sessionAware) { - sessionAware.withSession(context.getSession()); - } + bp.getSessionSandbox().ifPresent(sandbox -> { + SessionSandboxReconciler.reconcile(sandbox, ServletActionContext.getRequest(), bp.getAction()); + if (bp.getAction() instanceof SessionAware sessionAware) { + sessionAware.withSession(context.getSession()); + } + }); } /** diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java index 9a87e49588..8546ee2a69 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java @@ -21,6 +21,8 @@ import org.apache.struts2.ActionInvocation; import org.apache.struts2.interceptor.ExecuteAndWaitInterceptor; +import java.util.Optional; + /** * Interface used to create a background process which will be executed by * {@link ExecuteAndWaitInterceptor}. @@ -61,9 +63,9 @@ default BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { /** * Returns the sandbox whose writes are applied to the session once the process is done. * - * @return the sandbox, or {@code null} when the process runs without one + * @return the sandbox, or empty when the process runs without one */ - default SandboxedSessionMap getSessionSandbox() { - return null; + default Optional getSessionSandbox() { + return Optional.empty(); } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java index 8c82745a4d..934ad6d375 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java @@ -40,6 +40,7 @@ * construction plus an ordered log of the process's own writes. It never touches the live session; the writes are * applied to it later by {@link SessionSandboxReconciler}. */ +@SuppressWarnings("java:S2160") // equal by contents, as every Map public class SandboxedSessionMap extends SessionMap { @Serial 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 5bf7e6a11c..45ed30a8f1 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 @@ -30,6 +30,7 @@ import java.io.Serializable; import java.util.HashMap; import java.util.Map; +import java.util.Optional; /** * Background thread to be executed by the ExecuteAndWaitInterceptor. @@ -107,8 +108,8 @@ public BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { } @Override - public SandboxedSessionMap getSessionSandbox() { - return sessionSandbox; + public Optional getSessionSandbox() { + return Optional.ofNullable(sessionSandbox); } private String invokeAction() throws Exception { diff --git a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java index 2757f1e65e..b31a6967bc 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java @@ -301,7 +301,7 @@ public void testBackgroundWritesAreInvisibleUntilPolled() throws Exception { BackgroundProcess process = awaitDone("sessionAction"); assertNull(process.getException()); - assertEquals("done", process.getSessionSandbox().get("result")); + assertEquals("done", process.getSessionSandbox().orElseThrow().get("result")); assertNull(httpSession.getAttribute("result")); assertNull(httpSession.getAttribute("aware")); } @@ -343,7 +343,7 @@ private void useLiveSession() { context = ActionContext.of(context).withSession(session).getContextMap(); } - private BackgroundProcess awaitDone(String actionName) throws InterruptedException { + private BackgroundProcess awaitDone(String actionName) { BackgroundProcess process = (BackgroundProcess) httpSession.getAttribute(ExecuteAndWaitInterceptor.KEY + actionName); await().atMost(5, TimeUnit.SECONDS).until(process::isDone); return process; diff --git a/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java b/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java index 4610d3786e..505e21632d 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java +++ b/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java @@ -34,6 +34,7 @@ public class SessionWritingWaitAction implements Action, SessionAware { private String failWith; @Override + @SuppressWarnings("java:S2925") // simulates the long-running work execAndWait exists for public String execute() throws Exception { Thread.sleep(200); ActionContext.getContext().getSession().put("result", "done"); From a713fe0b5d1805359989550ef5a294f60a2989e4 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 16:37:15 +0200 Subject: [PATCH 25/27] WW-3576 fix(core): address review findings on the execAndWait write-back - A sandbox entry's setValue updates the entry as well as the sandbox. - Write-back discards the remaining writes when the session is invalidated while it runs, instead of failing the polling request; an exception from the conflict hook still propagates. - When write-back replaces the session, the polling context gets a new request SessionMap and session helper before re-injection. - The default beforeInvocation() binds the handoff copy, so a super-calling override edits the context the action runs with. Co-Authored-By: Claude Opus 5.5 --- .../ExecuteAndWaitInterceptor.java | 11 ++- .../interceptor/exec/SandboxedSessionMap.java | 1 + .../exec/SessionSandboxReconciler.java | 72 +++++++++++++------ .../exec/StrutsBackgroundProcess.java | 5 +- .../ExecuteAndWaitInterceptorTest.java | 19 +++++ .../interceptor/SessionWritingWaitAction.java | 10 +++ .../exec/SandboxedSessionMapTest.java | 4 +- .../exec/SessionSandboxReconcilerTest.java | 66 +++++++++++++++++ .../exec/StrutsBackgroundProcessTest.java | 26 +++++++ ...18-WW-3576-session-map-staleness-design.md | 9 ++- 10 files changed, 196 insertions(+), 27 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java index 6f31e63db5..8d51e99288 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java @@ -26,10 +26,13 @@ import org.apache.struts2.config.entities.ResultConfig; import org.apache.struts2.inject.Container; import org.apache.struts2.inject.Inject; +import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpSession; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.ServletActionContext; +import org.apache.struts2.dispatcher.DispatcherConstants; +import org.apache.struts2.dispatcher.SessionMap; import org.apache.struts2.interceptor.exec.BackgroundProcess; import org.apache.struts2.interceptor.exec.ExecutorProvider; import org.apache.struts2.interceptor.exec.SandboxedSessionMap; @@ -364,7 +367,13 @@ protected String doIntercept(ActionInvocation actionInvocation) throws Exception private void writeBackSessionSandbox(BackgroundProcess bp, ActionContext context) { bp.getSessionSandbox().ifPresent(sandbox -> { - SessionSandboxReconciler.reconcile(sandbox, ServletActionContext.getRequest(), bp.getAction()); + HttpServletRequest request = ServletActionContext.getRequest(); + HttpSession before = request.getSession(false); + SessionSandboxReconciler.reconcile(sandbox, request, bp.getAction()); + if (request.getSession(false) != before) { + SessionMap sessionMap = new SessionMap(request); + context.withSession(sessionMap).with(DispatcherConstants.SESSION, sessionMap); + } if (bp.getAction() instanceof SessionAware sessionAware) { sessionAware.withSession(context.getSession()); } diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java index 934ad6d375..b0af1b7981 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java @@ -141,6 +141,7 @@ public synchronized Set> entrySet() { result.add(new SimpleEntry<>(key, current.getValue()) { @Override public Object setValue(Object value) { + super.setValue(value); return put(key, value); } }); diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java index f34a9462e6..76256fdc80 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java @@ -60,34 +60,59 @@ public static void reconcile(SandboxedSessionMap sandbox, HttpServletRequest req List operations = sandbox.getOperations(); int lastInvalidate = lastInvalidateIndex(operations); if (lastInvalidate >= 0) { - real.invalidate(); - HttpSession fresh = request.getSession(true); - for (String key : touchedKeys(operations.subList(lastInvalidate + 1, operations.size()))) { - write(fresh, key, sandbox.get(key)); - } + replaceSession(sandbox, request, real, operations.subList(lastInvalidate + 1, operations.size())); return; } for (String key : touchedKeys(operations)) { - reconcileKey(sandbox, real, key, action); + if (!reconcileKey(sandbox, real, key, action)) { + discardRemaining(sandbox); + return; + } + } + } + + private static void replaceSession(SandboxedSessionMap sandbox, HttpServletRequest request, HttpSession real, + List operationsAfterInvalidate) { + try { + real.invalidate(); + } catch (IllegalStateException e) { + discardRemaining(sandbox); + return; + } + HttpSession fresh = request.getSession(true); + for (String key : touchedKeys(operationsAfterInvalidate)) { + if (!write(fresh, key, sandbox.get(key))) { + discardRemaining(sandbox); + return; + } } } - private static void reconcileKey(SandboxedSessionMap sandbox, HttpSession real, String key, Object action) { - Object current = real.getAttribute(key); + private static boolean reconcileKey(SandboxedSessionMap sandbox, HttpSession real, String key, Object action) { + Object current; + try { + current = real.getAttribute(key); + } catch (IllegalStateException e) { + return false; + } Object snapshot = sandbox.getSnapshotValue(key); Object background = sandbox.get(key); if (Objects.equals(current, snapshot)) { - write(real, key, background); - } else if (action instanceof BackgroundSessionConflictAware aware) { + return write(real, key, background); + } + if (action instanceof BackgroundSessionConflictAware aware) { SessionConflict conflict = new SessionConflict(key, snapshot, current, background); Object resolved = aware.resolveSessionConflict(conflict).resolve(conflict); - if (resolved != current) { - write(real, key, resolved); - } - } else { - LOG.debug("Background process [{}] keeps the current value of session key [{}] changed by another request", - sandbox.getProcessName(), key); + return resolved == current || write(real, key, resolved); } + LOG.debug("Background process [{}] keeps the current value of session key [{}] changed by another request", + sandbox.getProcessName(), key); + return true; + } + + private static void discardRemaining(SandboxedSessionMap sandbox) { + LOG.debug("Discarding the remaining session writes of background process [{}]: its session was invalidated", + sandbox.getProcessName()); } private static int lastInvalidateIndex(List operations) { @@ -111,11 +136,16 @@ private static Set touchedKeys(List operations) { return keys; } - private static void write(HttpSession session, String key, Object value) { - if (value == null) { - session.removeAttribute(key); - } else { - session.setAttribute(key, value); + private static boolean write(HttpSession session, String key, Object value) { + try { + if (value == null) { + session.removeAttribute(key); + } else { + session.setAttribute(key, value); + } + return true; + } catch (IllegalStateException e) { + 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 45ed30a8f1..4dd9a9b1f2 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 @@ -131,7 +131,8 @@ private void installSessionSandbox() { private Map backgroundContextMap() { ActionContext bound = ActionContext.getContext(); - if (bound == null || bound.getContextMap() == invocation.getInvocationContext().getContextMap()) { + if (bound == null || bound.getContextMap() == sandboxContext + || bound.getContextMap() == invocation.getInvocationContext().getContextMap()) { return sandboxContext; } return copyOf(bound.getContextMap()); @@ -166,7 +167,7 @@ public void run() { * @throws Exception any exception thrown will be thrown, in turn, by the ExecuteAndWaitInterceptor */ protected void beforeInvocation() throws Exception { - ActionContext.bind(invocation.getInvocationContext()); + ActionContext.bind(sandboxContext != null ? ActionContext.of(sandboxContext) : invocation.getInvocationContext()); } /** diff --git a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java index b31a6967bc..ff1132c19f 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java @@ -38,6 +38,7 @@ import org.apache.struts2.util.location.LocatableProperties; import jakarta.servlet.http.HttpSession; import org.apache.struts2.StrutsInternalTestCase; +import org.apache.struts2.dispatcher.DispatcherConstants; import org.apache.struts2.dispatcher.HttpParameters; import org.apache.struts2.dispatcher.SessionMap; import org.apache.struts2.interceptor.exec.BackgroundProcess; @@ -336,6 +337,24 @@ public void testSessionAwareGetsTheLiveMapBackAfterWriteBack() throws Exception assertSame(session, action.getSession()); } + public void testPollingContextSeesTheSessionStartedByABackgroundInvalidate() throws Exception { + useLiveSession(); + Map requestParams = new HashMap<>(); + requestParams.put("invalidateFirst", new String[]{"true"}); + withRequestParams(requestParams); + + assertEquals("wait", buildProxy("sessionAction").execute()); + SessionWritingWaitAction action = (SessionWritingWaitAction) awaitDone("sessionAction").getAction(); + + ActionProxy poll = buildProxy("sessionAction"); + assertEquals("success", poll.execute()); + ActionContext pollContext = poll.getInvocation().getInvocationContext(); + assertTrue(((MockHttpSession) httpSession).isInvalid()); + assertEquals("done", pollContext.getSession().get("result")); + assertEquals("yes", action.getSession().get("aware")); + assertSame(pollContext.getSession(), pollContext.get(DispatcherConstants.SESSION)); + } + private void useLiveSession() { httpSession = new MockHttpSession(); request.setSession(httpSession); diff --git a/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java b/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java index 505e21632d..5f3dbaefe3 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java +++ b/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java @@ -21,6 +21,7 @@ import org.apache.struts2.ActionContext; import org.apache.struts2.action.Action; import org.apache.struts2.action.SessionAware; +import org.apache.struts2.dispatcher.SessionMap; import org.apache.struts2.interceptor.parameter.StrutsParameter; import java.util.Map; @@ -32,11 +33,15 @@ public class SessionWritingWaitAction implements Action, SessionAware { private Map session; private String failWith; + private boolean invalidateFirst; @Override @SuppressWarnings("java:S2925") // simulates the long-running work execAndWait exists for public String execute() throws Exception { Thread.sleep(200); + if (invalidateFirst) { + ((SessionMap) session).invalidate(); + } ActionContext.getContext().getSession().put("result", "done"); session.put("aware", "yes"); if (failWith != null) { @@ -58,4 +63,9 @@ public Map getSession() { public void setFailWith(String failWith) { this.failWith = failWith; } + + @StrutsParameter + public void setInvalidateFirst(boolean invalidateFirst) { + this.invalidateFirst = invalidateFirst; + } } diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java index c000da4ad3..c74c8542bc 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java @@ -73,8 +73,10 @@ public void testEntrySetValueIsLogged() { live.setAttribute("a", "1"); SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - sandbox.entrySet().iterator().next().setValue("2"); + Map.Entry entry = sandbox.entrySet().iterator().next(); + assertEquals("1", entry.setValue("2")); + assertEquals("2", entry.getValue()); assertEquals("2", sandbox.get("a")); assertEquals(List.of(new SessionOperation.Put("a", "2")), sandbox.getOperations()); } diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java index 9d9dcd4d7f..7a0fb735d3 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java @@ -167,6 +167,58 @@ public void testHookExceptionPropagates() { } } + public void testSessionInvalidatedAfterTheIdCheckDiscardsTheWrites() { + InvalidatedOnIdCheck session = new InvalidatedOnIdCheck(); + request.setSession(session); + SandboxedSessionMap sandbox = new SandboxedSessionMap(session, "p"); + sandbox.put("a", "bg"); + session.armed = true; + + SessionSandboxReconciler.reconcile(sandbox, request, new Object()); + + assertTrue(session.isInvalid()); + assertNull(request.getSession(false)); + } + + public void testBackgroundInvalidateOfASessionInvalidatedAfterTheIdCheckDiscardsTheWrites() { + InvalidatedOnIdCheck session = new InvalidatedOnIdCheck(); + request.setSession(session); + SandboxedSessionMap sandbox = new SandboxedSessionMap(session, "p"); + sandbox.invalidate(); + sandbox.put("user", "u"); + session.armed = true; + + SessionSandboxReconciler.reconcile(sandbox, request, new Object()); + + assertNull(request.getSession(false)); + } + + public void testSessionInvalidatedByTheHookDiscardsTheRemainingWrites() { + SandboxedSessionMap sandbox = conflictOnA(); + sandbox.put("b", "2"); + + SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { + live.invalidate(); + return SessionConflictResolution.useBackground(); + })); + + assertNull(request.getSession(false)); + } + + public void testHookIllegalStateExceptionPropagates() { + SandboxedSessionMap sandbox = conflictOnA(); + IllegalStateException thrown = new IllegalStateException("hook"); + + try { + SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { + throw thrown; + })); + fail("expected the hook's exception"); + } catch (IllegalStateException e) { + assertSame(thrown, e); + } + } + private SandboxedSessionMap conflictOnA() { live.setAttribute("a", "1"); SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); @@ -178,4 +230,18 @@ private SandboxedSessionMap conflictOnA() { private static BackgroundSessionConflictAware resolving(Function resolver) { return resolver::apply; } + + private static class InvalidatedOnIdCheck extends MockHttpSession { + private boolean armed; + + @Override + public String getId() { + String id = super.getId(); + if (armed) { + armed = false; + invalidate(); + } + return id; + } + } } diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java index 39c30db57f..51f29e8bfe 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java @@ -219,6 +219,32 @@ protected void beforeInvocation() { assertSame(sandbox, ActionContext.of(seen.get()).getSession()); } + public void testSandboxKeepsEditsOfASuperCallingBeforeInvocation() { + AtomicReference> seen = new AtomicReference<>(); + MockActionInvocationWithActionInvoker invocation = new MockActionInvocationWithActionInvoker(() -> { + seen.set(ActionContext.getContext().getContextMap()); + return "done"; + }); + ActionContext requestContext = ActionContext.of(); + invocation.setInvocationContext(requestContext); + SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "edited"); + + BackgroundProcess bp = new StrutsBackgroundProcess(invocation, "edited", Thread.NORM_PRIORITY) { + @Override + protected void beforeInvocation() throws Exception { + super.beforeInvocation(); + ActionContext.getContext().with("edited", "yes"); + } + }.withSessionSandbox(sandbox).prepare(); + executor.execute(bp); + await().atMost(5, TimeUnit.SECONDS).until(bp::isDone); + + assertNull(bp.getException()); + assertEquals("yes", seen.get().get("edited")); + assertSame(sandbox, ActionContext.of(seen.get()).getSession()); + assertFalse(requestContext.getContextMap().containsKey("edited")); + } + private static class MockActionInvocationWithActionInvoker extends MockActionInvocation { private final Callable actionInvoker; 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 index 08dadb2fd8..2dfc042086 100644 --- 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 @@ -119,8 +119,9 @@ never touches the request: `BackgroundProcess`** — a fluent `withSessionSandbox(…)` (no-op by default) called by the interceptor on whatever the overridable `getNewBackgroundProcess(…)` factory returns, and `getSessionSandbox()` (null by default). Installing it happens in `StrutsBackgroundProcess`'s private thread body, *after* - the overridable `beforeInvocation()` (whose default binds the original context), so neither `super` - nor a subclass that skips it can undo it. A custom `BackgroundProcess` not + the overridable `beforeInvocation()` (whose default binds the handoff copy, so a super-calling + override edits the context the action will use), so neither `super` nor a subclass that skips it can + undo it. A custom `BackgroundProcess` not extending `StrutsBackgroundProcess` keeps today's shared-map behaviour — migration-notes line. - Not serialized: held `transient` by the background process. A deserialized process already has a null invocation and is restarted (WW-4900), so the snapshot is lost with it. @@ -137,6 +138,10 @@ never touches the request: - After write-back, a `SessionAware` action is re-injected with the request's session map so the result rendering writes to the live session. - If nobody polls after completion, the writes are never applied (the session expires with them). +- If another request invalidates the session during write-back, the remaining writes are discarded and + no replacement session is created; an exception from the conflict hook still propagates. +- When write-back replaces the session (logged `invalidate()`), the polling context gets a new request + `SessionMap` (and `DispatcherConstants.SESSION` helper) before re-injection. - *(Builder-check call)* Write-back runs **before** `throw bp.getException()`: a failed background action's writes are applied, matching today's behaviour where its writes were already live. - *(Builder-check call)* The write log is an **ordered operation list** (put / remove / invalidate). From 82b447504278c2d6ec8cc7d480cf89e402e9faf5 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Sun, 18 Oct 2026 16:41:41 +0200 Subject: [PATCH 26/27] WW-3576 fix(core): discard write-back only for an invalidated session An IllegalStateException during write-back discards the remaining writes only when the session is really invalidated; one thrown by a listener on a live session propagates. A sandbox entry's setValue returns the entry's previous value. Co-Authored-By: Claude Opus 5.5 --- .../interceptor/exec/SandboxedSessionMap.java | 5 +++-- .../exec/SessionSandboxReconciler.java | 12 +++++++++++ .../exec/SessionSandboxReconcilerTest.java | 20 +++++++++++++++++++ 3 files changed, 35 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java index b0af1b7981..617842e51c 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java @@ -141,8 +141,9 @@ public synchronized Set> entrySet() { result.add(new SimpleEntry<>(key, current.getValue()) { @Override public Object setValue(Object value) { - super.setValue(value); - return put(key, value); + Object old = super.setValue(value); + put(key, value); + return old; } }); } diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java index 76256fdc80..249628f674 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java @@ -76,6 +76,7 @@ private static void replaceSession(SandboxedSessionMap sandbox, HttpServletReque try { real.invalidate(); } catch (IllegalStateException e) { + requireInvalidated(real, e); discardRemaining(sandbox); return; } @@ -93,6 +94,7 @@ private static boolean reconcileKey(SandboxedSessionMap sandbox, HttpSession rea try { current = real.getAttribute(key); } catch (IllegalStateException e) { + requireInvalidated(real, e); return false; } Object snapshot = sandbox.getSnapshotValue(key); @@ -145,7 +147,17 @@ private static boolean write(HttpSession session, String key, Object value) { } return true; } catch (IllegalStateException e) { + requireInvalidated(session, e); return false; } } + + private static void requireInvalidated(HttpSession session, IllegalStateException failure) { + try { + session.getCreationTime(); + } catch (IllegalStateException invalidated) { + return; + } + throw failure; + } } diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java index 7a0fb735d3..02869d11cf 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java @@ -19,6 +19,8 @@ package org.apache.struts2.interceptor.exec; import jakarta.servlet.http.HttpSession; +import jakarta.servlet.http.HttpSessionBindingEvent; +import jakarta.servlet.http.HttpSessionBindingListener; import junit.framework.TestCase; import org.apache.struts2.action.BackgroundSessionConflictAware; import org.apache.struts2.action.SessionConflict; @@ -205,6 +207,24 @@ public void testSessionInvalidatedByTheHookDiscardsTheRemainingWrites() { assertNull(request.getSession(false)); } + public void testListenerIllegalStateExceptionOnALiveSessionPropagates() { + IllegalStateException thrown = new IllegalStateException("listener"); + SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); + sandbox.put("a", new HttpSessionBindingListener() { + @Override + public void valueBound(HttpSessionBindingEvent event) { + throw thrown; + } + }); + + try { + SessionSandboxReconciler.reconcile(sandbox, request, new Object()); + fail("expected the listener's exception"); + } catch (IllegalStateException e) { + assertSame(thrown, e); + } + } + public void testHookIllegalStateExceptionPropagates() { SandboxedSessionMap sandbox = conflictOnA(); IllegalStateException thrown = new IllegalStateException("hook"); From a66f76b0e8cc5e5775cfa07fc244503c50c7c2c9 Mon Sep 17 00:00:00 2001 From: Lukasz Lenart Date: Mon, 19 Oct 2026 11:46:35 +0200 Subject: [PATCH 27/27] WW-3576 refactor(core): move the execAndWait session sandbox to WW-5787 The sandbox changes observable execAndWait behaviour in a minor release: background session writes are deferred until the job finishes, the job no longer sees session changes from other requests, and a session-id rotation discards its writes. It moves to the 8.0.0 rewrite (WW-5787). WW-3576 keeps the SessionMap staleness fix, the request registry with detach at request end, and the volatile done flag on StrutsBackgroundProcess. ExecuteAndWaitInterceptor, BackgroundProcess and StrutsBackgroundProcess otherwise return to main. Co-Authored-By: Claude Opus 5.5 --- .../BackgroundSessionConflictAware.java | 37 --- .../struts2/action/SessionConflict.java | 31 -- .../action/SessionConflictResolution.java | 80 ----- .../apache/struts2/dispatcher/SessionMap.java | 37 --- .../ExecuteAndWaitInterceptor.java | 36 +-- .../interceptor/exec/BackgroundProcess.java | 32 +- .../interceptor/exec/SandboxedSessionMap.java | 173 ----------- .../interceptor/exec/SessionOperation.java | 31 -- .../exec/SessionSandboxReconciler.java | 163 ---------- .../exec/StrutsBackgroundProcess.java | 63 +--- .../ExecuteAndWaitInterceptorTest.java | 102 ------- .../interceptor/SessionWritingWaitAction.java | 71 ----- .../exec/SandboxedSessionMapTest.java | 148 --------- .../exec/SessionSandboxReconcilerTest.java | 267 ---------------- .../exec/StrutsBackgroundProcessTest.java | 84 ------ ...026-10-18-WW-3576-session-map-staleness.md | 285 +----------------- ...18-WW-3576-session-map-staleness-design.md | 118 +------- 17 files changed, 26 insertions(+), 1732 deletions(-) delete mode 100644 core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java delete mode 100644 core/src/main/java/org/apache/struts2/action/SessionConflict.java delete mode 100644 core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java delete mode 100644 core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java delete mode 100644 core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java delete mode 100644 core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java delete mode 100644 core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java delete mode 100644 core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java delete mode 100644 core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java diff --git a/core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java b/core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java deleted file mode 100644 index adfd0f2fb3..0000000000 --- a/core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java +++ /dev/null @@ -1,37 +0,0 @@ -/* - * 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.action; - -/** - * Actions run by the execAndWait interceptor that want to decide how a session key is reconciled when the - * background process wrote it while another request changed it should implement this interface. - * - * Without it, the value the session holds at reconciliation is kept and the background's write is dropped. - */ -public interface BackgroundSessionConflictAware { - - /** - * Decides the value of one conflicting session key. Called once per key, on the polling request. - * - * @param conflict the key with its snapshot, current and background values. - * @return how the key is reconciled. - */ - SessionConflictResolution resolveSessionConflict(SessionConflict conflict); - -} diff --git a/core/src/main/java/org/apache/struts2/action/SessionConflict.java b/core/src/main/java/org/apache/struts2/action/SessionConflict.java deleted file mode 100644 index 5af7ccaaea..0000000000 --- a/core/src/main/java/org/apache/struts2/action/SessionConflict.java +++ /dev/null @@ -1,31 +0,0 @@ -/* - * 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.action; - -/** - * A session key the execAndWait background process wrote while another request changed it. A {@code null} - * value means the key was absent. - * - * @param key the session attribute name. - * @param snapshotValue the value when the background process started. - * @param currentValue the value the session holds now. - * @param backgroundValue the value the background process left, {@code null} if it removed the key. - */ -public record SessionConflict(String key, Object snapshotValue, Object currentValue, Object backgroundValue) { -} diff --git a/core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java b/core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java deleted file mode 100644 index cf3e1bef6e..0000000000 --- a/core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java +++ /dev/null @@ -1,80 +0,0 @@ -/* - * 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.action; - -/** - * How a {@link BackgroundSessionConflictAware} action reconciles one conflicting session key. - */ -public final class SessionConflictResolution { - - private static final SessionConflictResolution KEEP_CURRENT = new SessionConflictResolution(Kind.KEEP_CURRENT, null); - private static final SessionConflictResolution USE_BACKGROUND = new SessionConflictResolution(Kind.USE_BACKGROUND, null); - private static final SessionConflictResolution REMOVE = new SessionConflictResolution(Kind.REPLACE, null); - - private enum Kind { KEEP_CURRENT, USE_BACKGROUND, REPLACE } - - private final Kind kind; - private final Object replacement; - - private SessionConflictResolution(Kind kind, Object replacement) { - this.kind = kind; - this.replacement = replacement; - } - - /** - * @return a resolution keeping the value the session holds now. - */ - public static SessionConflictResolution keepCurrent() { - return KEEP_CURRENT; - } - - /** - * @return a resolution taking the background process's value, removing the key if it removed it. - */ - public static SessionConflictResolution useBackground() { - return USE_BACKGROUND; - } - - /** - * @param value the value to store, {@code null} removes the key. - * @return a resolution storing the given value. - */ - public static SessionConflictResolution replaceWith(Object value) { - return new SessionConflictResolution(Kind.REPLACE, value); - } - - /** - * @return a resolution removing the key. - */ - public static SessionConflictResolution remove() { - return REMOVE; - } - - /** - * @param conflict the conflict this resolution answers. - * @return the value the key holds after reconciliation, {@code null} meaning removed. - */ - public Object resolve(SessionConflict conflict) { - return switch (kind) { - case KEEP_CURRENT -> conflict.currentValue(); - case USE_BACKGROUND -> conflict.backgroundValue(); - case REPLACE -> replacement; - }; - } -} 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 5e4c7d2bd2..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,7 +28,6 @@ import java.util.Enumeration; import java.util.HashSet; import java.util.Set; -import java.util.concurrent.Callable; import java.util.concurrent.ConcurrentLinkedQueue; import java.util.Queue; @@ -44,8 +43,6 @@ public class SessionMap extends AbstractMap implements Serializa private static final String REQUEST_MAPS = SessionMap.class.getName() + ".maps"; - private static final ThreadLocal LIVE_WRITE_LISTENER = new ThreadLocal<>(); - // 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"}) @@ -58,9 +55,6 @@ public class SessionMap extends AbstractMap implements Serializa protected HttpServletRequest request; private volatile boolean detached; - protected SessionMap() { - } - /** * Creates a new session map given a http servlet request. Note, the enumeration of request * attributes will occur when the map entries are asked for. @@ -109,7 +103,6 @@ public String toString() { * Invalidate the http session. */ public void invalidate() { - notifyLiveWrite(); HttpSession current = session; if (current == null) { return; @@ -128,7 +121,6 @@ public void invalidate() { */ @Override public void clear() { - notifyLiveWrite(); HttpSession current = session; if (current == null) { return; @@ -166,7 +158,6 @@ public Set> entrySet() { result.add(new StringObjectEntry(key, value) { @Override public Object setValue(final Object obj) { - notifyLiveWrite(); current.setAttribute(key, obj); return value; @@ -214,7 +205,6 @@ public Object get(final Object key) { */ @Override public Object put(final String key, final Object value) { - notifyLiveWrite(); HttpSession current = session; if (current == null) { current = pin(); @@ -231,32 +221,6 @@ public Object put(final String key, final Object value) { return oldValue; } - /** - * Runs the work with a listener notified, on the calling thread, whenever a {@link SessionMap} writes to the live - * session. A subclass that overrides the write methods without calling them here never notifies it. - * - * @param listener notified on each live write made while the work runs - * @param work the work to run - * @param the type of the work's result - * @return the work's result - * @throws Exception whatever the work throws - */ - protected static T callWithLiveWriteListener(final Runnable listener, final Callable work) throws Exception { - LIVE_WRITE_LISTENER.set(listener); - try { - return work.call(); - } finally { - LIVE_WRITE_LISTENER.remove(); - } - } - - private static void notifyLiveWrite() { - Runnable listener = LIVE_WRITE_LISTENER.get(); - if (listener != null) { - listener.run(); - } - } - private HttpSession pin() { synchronized (this) { HttpSession current = session; @@ -291,7 +255,6 @@ private void unpin(final HttpSession stale) { */ @Override public Object remove(final Object key) { - notifyLiveWrite(); HttpSession current = session; if (current == null) { return null; diff --git a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java index 8d51e99288..5a073147fc 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java +++ b/core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java @@ -22,21 +22,15 @@ import org.apache.struts2.ActionContext; import org.apache.struts2.ActionInvocation; import org.apache.struts2.ActionProxy; -import org.apache.struts2.action.SessionAware; import org.apache.struts2.config.entities.ResultConfig; import org.apache.struts2.inject.Container; import org.apache.struts2.inject.Inject; -import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpSession; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; import org.apache.struts2.ServletActionContext; -import org.apache.struts2.dispatcher.DispatcherConstants; -import org.apache.struts2.dispatcher.SessionMap; import org.apache.struts2.interceptor.exec.BackgroundProcess; import org.apache.struts2.interceptor.exec.ExecutorProvider; -import org.apache.struts2.interceptor.exec.SandboxedSessionMap; -import org.apache.struts2.interceptor.exec.SessionSandboxReconciler; import org.apache.struts2.interceptor.exec.StrutsBackgroundProcess; import org.apache.struts2.interceptor.exec.StrutsExecutorProvider; import org.apache.struts2.util.TokenHelper; @@ -84,14 +78,9 @@ * millis we ensure the user is presented fast search results immediately and for the slow results a wait page is used. *

* - *

The background process works on a session sandbox, a snapshot of the session taken when the process starts. Use - * ActionContext.getContext().getSession() or {@link SessionAware} to reach it. Its writes are applied to - * the real session by the polling request that sees the process finish, only if the session id is unchanged. - * Conflicts keep the real session's value by default and can be resolved per action with - * {@link org.apache.struts2.action.BackgroundSessionConflictAware}. OGNL #session and - * invocation.getInvocationContext().getSession() bypass the sandbox, write to the live session and log a - * warning. A custom {@link BackgroundProcess} that does not extend {@link StrutsBackgroundProcess}, or a subclass - * overriding prepare(), does not get a sandbox. + *

Important: Because the action will be running in a separate thread, you can't use ActionContext because it + * is a ThreadLocal. This means if you need to access, for example, session data, you need to implement SessionAware + * rather than calling ActionContext.getSession(). *

* *

The thread kicked off by this interceptor will be named in the form actionNameBackgroundProcess. @@ -306,8 +295,7 @@ protected String doIntercept(ActionInvocation actionInvocation) throws Exception if ((!executeAfterValidationPass || secondTime) && bp == null) { LOG.trace("BackgroundProcess instance is null (create new instance) - executeAfterValidationPass: {}, secondTime: {}.", executeAfterValidationPass, secondTime); - SandboxedSessionMap sandbox = new SandboxedSessionMap(httpSession, name); - bp = getNewBackgroundProcess(name, actionInvocation, threadPriority).withSessionSandbox(sandbox).prepare(); + bp = getNewBackgroundProcess(name, actionInvocation, threadPriority).prepare(); session.put(bp_SessionKey, bp); if (executor == null || executor.isShutdown()) { LOG.warn("Executor is shutting down (or null), cannot execute a new process, invoke next ActionInvocation step and return."); @@ -346,7 +334,6 @@ protected String doIntercept(ActionInvocation actionInvocation) throws Exception LOG.trace("BackgroundProcess instance is done (remove key, return result) - executeAfterValidationPass: {}, secondTime: {}.", executeAfterValidationPass, secondTime); session.remove(bp_SessionKey); actionInvocation.getStack().push(bp.getAction()); - writeBackSessionSandbox(bp, context); // if an exception occurred during action execution, throw it here if (bp.getException() != null) { @@ -365,21 +352,6 @@ protected String doIntercept(ActionInvocation actionInvocation) throws Exception } } - private void writeBackSessionSandbox(BackgroundProcess bp, ActionContext context) { - bp.getSessionSandbox().ifPresent(sandbox -> { - HttpServletRequest request = ServletActionContext.getRequest(); - HttpSession before = request.getSession(false); - SessionSandboxReconciler.reconcile(sandbox, request, bp.getAction()); - if (request.getSession(false) != before) { - SessionMap sessionMap = new SessionMap(request); - context.withSession(sessionMap).with(DispatcherConstants.SESSION, sessionMap); - } - if (bp.getAction() instanceof SessionAware sessionAware) { - sessionAware.withSession(context.getSession()); - } - }); - } - /** *

* Performs the initial delay. diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java index 8546ee2a69..a9ca48d20c 100644 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java +++ b/core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java @@ -21,19 +21,9 @@ import org.apache.struts2.ActionInvocation; import org.apache.struts2.interceptor.ExecuteAndWaitInterceptor; -import java.util.Optional; - /** * Interface used to create a background process which will be executed by - * {@link ExecuteAndWaitInterceptor}. - * - *

A process built on {@link StrutsBackgroundProcess} works on a session sandbox, a snapshot of the session taken - * when the process starts, and receives it through {@link #withSessionSandbox(SandboxedSessionMap)}. Its writes are - * applied to the real session by the polling request that sees the process finish, and only if the session id is - * unchanged; conflicts keep the real session's value unless the action implements - * {@link org.apache.struts2.action.BackgroundSessionConflictAware}. A custom implementation that does not extend - * {@link StrutsBackgroundProcess}, or a subclass overriding {@link StrutsBackgroundProcess#prepare()}, gets no - * sandbox.

+ * {@link ExecuteAndWaitInterceptor} */ public interface BackgroundProcess extends Runnable { @@ -48,24 +38,4 @@ public interface BackgroundProcess extends Runnable { Exception getException(); boolean isDone(); - - /** - * Hands the process the session sandbox to run against. Called on the request thread before {@link #prepare()}; - * the default ignores the sandbox. - * - * @param sandbox the snapshot of the request's session - * @return this process - */ - default BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { - return this; - } - - /** - * Returns the sandbox whose writes are applied to the session once the process is done. - * - * @return the sandbox, or empty when the process runs without one - */ - default Optional getSessionSandbox() { - return Optional.empty(); - } } diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java deleted file mode 100644 index 617842e51c..0000000000 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java +++ /dev/null @@ -1,173 +0,0 @@ -/* - * 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.interceptor.exec; - -import jakarta.servlet.http.HttpSession; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.apache.struts2.dispatcher.SessionMap; - -import java.io.Serial; -import java.util.ArrayList; -import java.util.Collections; -import java.util.Enumeration; -import java.util.HashSet; -import java.util.LinkedHashMap; -import java.util.List; -import java.util.Map; -import java.util.Set; -import java.util.concurrent.Callable; -import java.util.concurrent.atomic.AtomicBoolean; - -/** - * The session view an execAndWait background process works on: a shallow snapshot of the session taken at - * construction plus an ordered log of the process's own writes. It never touches the live session; the writes are - * applied to it later by {@link SessionSandboxReconciler}. - */ -@SuppressWarnings("java:S2160") // equal by contents, as every Map -public class SandboxedSessionMap extends SessionMap { - - @Serial - private static final long serialVersionUID = 2907451127739228064L; - - private static final Logger LOG = LogManager.getLogger(SandboxedSessionMap.class); - - private final transient Map snapshot = new LinkedHashMap<>(); - private final transient Map view = new LinkedHashMap<>(); - private final transient List operations = new ArrayList<>(); - private final transient String startSessionId; - private final transient String processName; - private final transient AtomicBoolean escapeWarned = new AtomicBoolean(); - - /** - * Snapshots the attributes of the given session. Must be called on the request thread that owns the session. - * - * @param session the live session to snapshot - * @param processName the name of the background process, used in log messages - */ - public SandboxedSessionMap(HttpSession session, String processName) { - super(); - this.startSessionId = session.getId(); - this.processName = processName; - Enumeration names = session.getAttributeNames(); - while (names.hasMoreElements()) { - String name = names.nextElement(); - snapshot.put(name, session.getAttribute(name)); - } - view.putAll(snapshot); - } - - T runInSandbox(Callable work) throws Exception { - return callWithLiveWriteListener(this::warnEscapeOnce, work); - } - - private void warnEscapeOnce() { - if (escapeWarned.compareAndSet(false, true)) { - LOG.warn("Background process [{}] wrote to the request session, bypassing its session sandbox; " - + "use ActionContext.getContext().getSession() or SessionAware", processName); - } - } - - String getStartSessionId() { - return startSessionId; - } - - String getProcessName() { - return processName; - } - - synchronized List getOperations() { - return Collections.unmodifiableList(new ArrayList<>(operations)); - } - - synchronized Object getSnapshotValue(String key) { - return snapshot.get(key); - } - - @Override - public synchronized Object get(Object key) { - return view.get(asKey(key)); - } - - /** - * Records the write in the sandbox; a {@code null} value is recorded as a removal. - * - * @param key the name of the session attribute - * @param value the value to set - * @return the previous value in the sandbox - */ - @Override - public synchronized Object put(String key, Object value) { - if (value == null) { - return remove(key); - } - operations.add(new SessionOperation.Put(key, value)); - return view.put(key, value); - } - - @Override - public synchronized Object remove(Object key) { - String name = asKey(key); - operations.add(new SessionOperation.Remove(name)); - return view.remove(name); - } - - @Override - public synchronized boolean containsKey(Object key) { - return view.get(asKey(key)) != null; - } - - @Override - public synchronized Set> entrySet() { - Set> result = new HashSet<>(); - for (Map.Entry current : view.entrySet()) { - String key = current.getKey(); - result.add(new SimpleEntry<>(key, current.getValue()) { - @Override - public Object setValue(Object value) { - Object old = super.setValue(value); - put(key, value); - return old; - } - }); - } - return result; - } - - @Override - public synchronized void clear() { - for (String key : new ArrayList<>(view.keySet())) { - remove(key); - } - } - - /** - * Empties the sandbox and records the invalidation; the live session is invalidated only when the writes are - * applied. - */ - @Override - public synchronized void invalidate() { - view.clear(); - operations.add(new SessionOperation.Invalidate()); - } - - private static String asKey(Object key) { - return key != null ? key.toString() : null; - } -} diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java deleted file mode 100644 index 342b701839..0000000000 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java +++ /dev/null @@ -1,31 +0,0 @@ -/* - * 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.interceptor.exec; - -sealed interface SessionOperation { - - record Put(String key, Object value) implements SessionOperation { - } - - record Remove(String key) implements SessionOperation { - } - - record Invalidate() implements SessionOperation { - } -} diff --git a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java b/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java deleted file mode 100644 index 249628f674..0000000000 --- a/core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java +++ /dev/null @@ -1,163 +0,0 @@ -/* - * 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.interceptor.exec; - -import jakarta.servlet.http.HttpServletRequest; -import jakarta.servlet.http.HttpSession; -import org.apache.logging.log4j.LogManager; -import org.apache.logging.log4j.Logger; -import org.apache.struts2.action.BackgroundSessionConflictAware; -import org.apache.struts2.action.SessionConflict; - -import java.util.LinkedHashSet; -import java.util.List; -import java.util.Objects; -import java.util.Set; - -/** - * Applies the writes an execAndWait background process logged in its {@link SandboxedSessionMap} to the - * session of the polling request. Writes are discarded when that session is not the one the process started - * with. A key changed elsewhere since the snapshot keeps its current value unless the action implements - * {@link BackgroundSessionConflictAware}. - */ -public final class SessionSandboxReconciler { - - private static final Logger LOG = LogManager.getLogger(SessionSandboxReconciler.class); - - private SessionSandboxReconciler() { - } - - /** - * Applies the sandbox's writes to the session of the given request. Must be called on that request's thread. - * - * @param sandbox the sandbox the background process worked on - * @param request the polling request that saw the process finish - * @param action the background process's action, consulted when it implements - * {@link BackgroundSessionConflictAware} - */ - public static void reconcile(SandboxedSessionMap sandbox, HttpServletRequest request, Object action) { - HttpSession real = request.getSession(false); - if (real == null || !real.getId().equals(sandbox.getStartSessionId())) { - LOG.debug("Discarding the session writes of background process [{}]: its session is gone", sandbox.getProcessName()); - return; - } - List operations = sandbox.getOperations(); - int lastInvalidate = lastInvalidateIndex(operations); - if (lastInvalidate >= 0) { - replaceSession(sandbox, request, real, operations.subList(lastInvalidate + 1, operations.size())); - return; - } - for (String key : touchedKeys(operations)) { - if (!reconcileKey(sandbox, real, key, action)) { - discardRemaining(sandbox); - return; - } - } - } - - private static void replaceSession(SandboxedSessionMap sandbox, HttpServletRequest request, HttpSession real, - List operationsAfterInvalidate) { - try { - real.invalidate(); - } catch (IllegalStateException e) { - requireInvalidated(real, e); - discardRemaining(sandbox); - return; - } - HttpSession fresh = request.getSession(true); - for (String key : touchedKeys(operationsAfterInvalidate)) { - if (!write(fresh, key, sandbox.get(key))) { - discardRemaining(sandbox); - return; - } - } - } - - private static boolean reconcileKey(SandboxedSessionMap sandbox, HttpSession real, String key, Object action) { - Object current; - try { - current = real.getAttribute(key); - } catch (IllegalStateException e) { - requireInvalidated(real, e); - return false; - } - Object snapshot = sandbox.getSnapshotValue(key); - Object background = sandbox.get(key); - if (Objects.equals(current, snapshot)) { - return write(real, key, background); - } - if (action instanceof BackgroundSessionConflictAware aware) { - SessionConflict conflict = new SessionConflict(key, snapshot, current, background); - Object resolved = aware.resolveSessionConflict(conflict).resolve(conflict); - return resolved == current || write(real, key, resolved); - } - LOG.debug("Background process [{}] keeps the current value of session key [{}] changed by another request", - sandbox.getProcessName(), key); - return true; - } - - private static void discardRemaining(SandboxedSessionMap sandbox) { - LOG.debug("Discarding the remaining session writes of background process [{}]: its session was invalidated", - sandbox.getProcessName()); - } - - private static int lastInvalidateIndex(List operations) { - for (int i = operations.size() - 1; i >= 0; i--) { - if (operations.get(i) instanceof SessionOperation.Invalidate) { - return i; - } - } - return -1; - } - - private static Set touchedKeys(List operations) { - Set keys = new LinkedHashSet<>(); - for (SessionOperation operation : operations) { - if (operation instanceof SessionOperation.Put put) { - keys.add(put.key()); - } else if (operation instanceof SessionOperation.Remove removal) { - keys.add(removal.key()); - } - } - return keys; - } - - private static boolean write(HttpSession session, String key, Object value) { - try { - if (value == null) { - session.removeAttribute(key); - } else { - session.setAttribute(key, value); - } - return true; - } catch (IllegalStateException e) { - requireInvalidated(session, e); - return false; - } - } - - private static void requireInvalidated(HttpSession session, IllegalStateException failure) { - try { - session.getCreationTime(); - } catch (IllegalStateException invalidated) { - return; - } - throw failure; - } -} 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 4dd9a9b1f2..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 @@ -18,19 +18,13 @@ */ package org.apache.struts2.interceptor.exec; -import ognl.Ognl; -import ognl.OgnlContext; import org.apache.struts2.ActionContext; import org.apache.struts2.ActionInvocation; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; -import org.apache.struts2.action.SessionAware; import java.io.Serial; import java.io.Serializable; -import java.util.HashMap; -import java.util.Map; -import java.util.Optional; /** * Background thread to be executed by the ExecuteAndWaitInterceptor. @@ -49,8 +43,6 @@ public class StrutsBackgroundProcess implements BackgroundProcess, Serializable //WW-4900 transient since 2.5.15 protected transient ActionInvocation invocation; protected transient Exception exception; - private transient SandboxedSessionMap sessionSandbox; - private transient Map sandboxContext; protected String result; protected volatile boolean done; @@ -74,8 +66,7 @@ public BackgroundProcess prepare() { processThread = new Thread(() -> { try { beforeInvocation(); - installSessionSandbox(); - result = invokeAction(); + result = invocation.invokeActionOnly(); } catch (Exception e) { LOG.warn("Exception during invokeActionOnly() execution", e); exception = e; @@ -100,56 +91,6 @@ public BackgroundProcess prepare() { return this; } - @Override - public BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { - this.sessionSandbox = sandbox; - this.sandboxContext = copyOf(invocation.getInvocationContext().getContextMap()); - return this; - } - - @Override - public Optional getSessionSandbox() { - return Optional.ofNullable(sessionSandbox); - } - - private String invokeAction() throws Exception { - if (sessionSandbox == null) { - return invocation.invokeActionOnly(); - } - return sessionSandbox.runInSandbox(invocation::invokeActionOnly); - } - - private void installSessionSandbox() { - if (sessionSandbox == null) { - return; - } - ActionContext.bind(ActionContext.of(backgroundContextMap()).withSession(sessionSandbox)); - if (invocation.getAction() instanceof SessionAware sessionAware) { - sessionAware.withSession(sessionSandbox); - } - } - - private Map backgroundContextMap() { - ActionContext bound = ActionContext.getContext(); - if (bound == null || bound.getContextMap() == sandboxContext - || bound.getContextMap() == invocation.getInvocationContext().getContextMap()) { - return sandboxContext; - } - return copyOf(bound.getContextMap()); - } - - private static Map copyOf(Map context) { - if (context instanceof OgnlContext ognlContext) { - OgnlContext copy = Ognl.createDefaultContext(ognlContext.getRoot(), ognlContext.getMemberAccess(), - ognlContext.getClassResolver(), ognlContext.getTypeConverter()); - copy.setTraceEvaluations(ognlContext.isTraceEvaluations()); - copy.setKeepLastEvaluation(ognlContext.isKeepLastEvaluation()); - copy.putAll(ognlContext.getValues()); - return copy; - } - return new HashMap<>(context); - } - @Override public void run() { if (processThread == null) { @@ -167,7 +108,7 @@ public void run() { * @throws Exception any exception thrown will be thrown, in turn, by the ExecuteAndWaitInterceptor */ protected void beforeInvocation() throws Exception { - ActionContext.bind(sandboxContext != null ? ActionContext.of(sandboxContext) : invocation.getInvocationContext()); + ActionContext.bind(invocation.getInvocationContext()); } /** diff --git a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java index ff1132c19f..7701173ebf 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java @@ -38,16 +38,11 @@ import org.apache.struts2.util.location.LocatableProperties; import jakarta.servlet.http.HttpSession; import org.apache.struts2.StrutsInternalTestCase; -import org.apache.struts2.dispatcher.DispatcherConstants; import org.apache.struts2.dispatcher.HttpParameters; -import org.apache.struts2.dispatcher.SessionMap; -import org.apache.struts2.interceptor.exec.BackgroundProcess; import org.apache.struts2.interceptor.exec.ExecutorProvider; -import org.apache.struts2.interceptor.exec.SandboxedSessionMap; import org.apache.struts2.interceptor.parameter.ParametersInterceptor; import org.apache.struts2.views.jsp.StrutsMockHttpServletRequest; import org.apache.struts2.views.jsp.StrutsMockHttpSession; -import org.springframework.mock.web.MockHttpSession; import java.io.ByteArrayInputStream; import java.io.ByteArrayOutputStream; @@ -57,9 +52,6 @@ import java.util.Map; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; - -import static org.awaitility.Awaitility.await; /** * Test case for ExecuteAndWaitInterceptor. @@ -75,7 +67,6 @@ public class ExecuteAndWaitInterceptorTest extends StrutsInternalTestCase { private ParametersInterceptor parametersInterceptor; private TokenInterceptor tokenInterceptor; private TokenSessionStoreInterceptor tokenSessionInterceptor; - private ServletConfigInterceptor servletConfigInterceptor; public void testOneWait() throws Exception { waitInterceptor.setDelay(0); @@ -284,90 +275,6 @@ private void assertWaitPageRefreshAccepted(String actionName) throws Exception { assertNull(session.get(tokenSessionName)); } - public void testBackgroundWritesReachTheRealSession() throws Exception { - useLiveSession(); - - assertEquals("wait", buildProxy("sessionAction").execute()); - awaitDone("sessionAction"); - - assertEquals("success", buildProxy("sessionAction").execute()); - assertEquals("done", httpSession.getAttribute("result")); - assertEquals("yes", httpSession.getAttribute("aware")); - } - - public void testBackgroundWritesAreInvisibleUntilPolled() throws Exception { - useLiveSession(); - - assertEquals("wait", buildProxy("sessionAction").execute()); - BackgroundProcess process = awaitDone("sessionAction"); - - assertNull(process.getException()); - assertEquals("done", process.getSessionSandbox().orElseThrow().get("result")); - assertNull(httpSession.getAttribute("result")); - assertNull(httpSession.getAttribute("aware")); - } - - public void testWritesOfAFailedActionAreApplied() throws Exception { - useLiveSession(); - Map requestParams = new HashMap<>(); - requestParams.put("failWith", new String[]{"boom"}); - withRequestParams(requestParams); - - assertEquals("wait", buildProxy("sessionAction").execute()); - awaitDone("sessionAction"); - - ActionProxy poll = buildProxy("sessionAction"); - try { - poll.execute(); - fail("the background failure should be rethrown by the polling request"); - } catch (IllegalStateException failure) { - assertEquals("boom", failure.getMessage()); - } - assertEquals("done", httpSession.getAttribute("result")); - } - - public void testSessionAwareGetsTheLiveMapBackAfterWriteBack() throws Exception { - useLiveSession(); - - assertEquals("wait", buildProxy("sessionAction").execute()); - SessionWritingWaitAction action = (SessionWritingWaitAction) awaitDone("sessionAction").getAction(); - - assertEquals("success", buildProxy("sessionAction").execute()); - assertFalse(action.getSession() instanceof SandboxedSessionMap); - assertSame(session, action.getSession()); - } - - public void testPollingContextSeesTheSessionStartedByABackgroundInvalidate() throws Exception { - useLiveSession(); - Map requestParams = new HashMap<>(); - requestParams.put("invalidateFirst", new String[]{"true"}); - withRequestParams(requestParams); - - assertEquals("wait", buildProxy("sessionAction").execute()); - SessionWritingWaitAction action = (SessionWritingWaitAction) awaitDone("sessionAction").getAction(); - - ActionProxy poll = buildProxy("sessionAction"); - assertEquals("success", poll.execute()); - ActionContext pollContext = poll.getInvocation().getInvocationContext(); - assertTrue(((MockHttpSession) httpSession).isInvalid()); - assertEquals("done", pollContext.getSession().get("result")); - assertEquals("yes", action.getSession().get("aware")); - assertSame(pollContext.getSession(), pollContext.get(DispatcherConstants.SESSION)); - } - - private void useLiveSession() { - httpSession = new MockHttpSession(); - request.setSession(httpSession); - session = new SessionMap(request); - context = ActionContext.of(context).withSession(session).getContextMap(); - } - - private BackgroundProcess awaitDone(String actionName) { - BackgroundProcess process = (BackgroundProcess) httpSession.getAttribute(ExecuteAndWaitInterceptor.KEY + actionName); - await().atMost(5, TimeUnit.SECONDS).until(process::isDone); - return process; - } - private void withRequestParams(Map requestParams) { context = ActionContext.of(context).withParameters(HttpParameters.create(requestParams).build()).getContextMap(); } @@ -399,7 +306,6 @@ protected void setUp() throws Exception { container.inject(parametersInterceptor); container.inject(tokenInterceptor); container.inject(tokenSessionInterceptor); - container.inject(servletConfigInterceptor); waitInterceptor.init(); parametersInterceptor.init(); @@ -433,7 +339,6 @@ public void loadPackages() throws ConfigurationException { parametersInterceptor = new ParametersInterceptor(); tokenInterceptor = new TokenInterceptor(); tokenSessionInterceptor = new TokenSessionStoreInterceptor(); - servletConfigInterceptor = new ServletConfigInterceptor(); PackageConfig wait = new PackageConfig.Builder("") .addActionConfig("action1", new ActionConfig.Builder("", "action1", ExecuteAndWaitDelayAction.class.getName()) .addResultConfig(new ResultConfig.Builder(Action.SUCCESS, MockResult.class.getName()).build()) @@ -442,13 +347,6 @@ public void loadPackages() throws ConfigurationException { .addInterceptor(new InterceptorMapping("execAndWait", waitInterceptor)) .build()) .addActionConfig("plainWait", waitActionConfig("plainWait")) - .addActionConfig("sessionAction", new ActionConfig.Builder("", "sessionAction", SessionWritingWaitAction.class.getName()) - .addResultConfig(new ResultConfig.Builder(Action.SUCCESS, MockResult.class.getName()).build()) - .addResultConfig(new ResultConfig.Builder(ExecuteAndWaitInterceptor.WAIT, MockResult.class.getName()).build()) - .addInterceptor(new InterceptorMapping("servletConfig", servletConfigInterceptor)) - .addInterceptor(new InterceptorMapping("params", parametersInterceptor)) - .addInterceptor(new InterceptorMapping("execAndWait", waitInterceptor)) - .build()) .addActionConfig("tokenWait", waitActionConfig("tokenWait", new InterceptorMapping("token", tokenInterceptor))) .addActionConfig("tokenSessionWait", waitActionConfig("tokenSessionWait", new InterceptorMapping("tokenSession", tokenSessionInterceptor))) .build(); diff --git a/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java b/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java deleted file mode 100644 index 5f3dbaefe3..0000000000 --- a/core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java +++ /dev/null @@ -1,71 +0,0 @@ -/* - * 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.interceptor; - -import org.apache.struts2.ActionContext; -import org.apache.struts2.action.Action; -import org.apache.struts2.action.SessionAware; -import org.apache.struts2.dispatcher.SessionMap; -import org.apache.struts2.interceptor.parameter.StrutsParameter; - -import java.util.Map; - -/** - * Used by ExecuteAndWaitInterceptorTest to write to the session from the background thread. - */ -public class SessionWritingWaitAction implements Action, SessionAware { - - private Map session; - private String failWith; - private boolean invalidateFirst; - - @Override - @SuppressWarnings("java:S2925") // simulates the long-running work execAndWait exists for - public String execute() throws Exception { - Thread.sleep(200); - if (invalidateFirst) { - ((SessionMap) session).invalidate(); - } - ActionContext.getContext().getSession().put("result", "done"); - session.put("aware", "yes"); - if (failWith != null) { - throw new IllegalStateException(failWith); - } - return SUCCESS; - } - - @Override - public void withSession(Map session) { - this.session = session; - } - - public Map getSession() { - return session; - } - - @StrutsParameter - public void setFailWith(String failWith) { - this.failWith = failWith; - } - - @StrutsParameter - public void setInvalidateFirst(boolean invalidateFirst) { - this.invalidateFirst = invalidateFirst; - } -} diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java deleted file mode 100644 index c74c8542bc..0000000000 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java +++ /dev/null @@ -1,148 +0,0 @@ -/* - * 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.interceptor.exec; - -import junit.framework.TestCase; -import org.apache.struts2.dispatcher.SessionMap; -import org.apache.struts2.test.LogCapture; -import org.springframework.mock.web.MockHttpServletRequest; -import org.springframework.mock.web.MockHttpSession; - -import java.util.HashMap; -import java.util.List; -import java.util.Map; -import java.util.concurrent.atomic.AtomicReference; - -public class SandboxedSessionMapTest extends TestCase { - - public void testReadsSeeSnapshotPlusOwnWritesAndNeverTheLiveSession() { - MockHttpSession live = new MockHttpSession(); - live.setAttribute("a", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - - live.setAttribute("a", "changed"); - sandbox.put("b", "2"); - - assertEquals("1", sandbox.get("a")); - assertEquals("2", sandbox.get("b")); - assertNull(live.getAttribute("b")); - assertEquals(live.getId(), sandbox.getStartSessionId()); - assertEquals("p", sandbox.getProcessName()); - assertEquals(List.of(new SessionOperation.Put("b", "2")), sandbox.getOperations()); - } - - public void testRemoveClearAndInvalidateAreLoggedInOrder() { - MockHttpSession live = new MockHttpSession(); - live.setAttribute("a", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - - sandbox.remove("a"); - sandbox.invalidate(); - sandbox.put("user", "u"); - - assertEquals(List.of(new SessionOperation.Remove("a"), new SessionOperation.Invalidate(), - new SessionOperation.Put("user", "u")), sandbox.getOperations()); - assertEquals(Map.of("user", "u"), new HashMap<>(sandbox)); - assertEquals("1", sandbox.getSnapshotValue("a")); - } - - public void testPutNullIsLoggedAsRemove() { - SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "p"); - sandbox.put("a", null); - assertEquals(List.of(new SessionOperation.Remove("a")), sandbox.getOperations()); - } - - public void testEntrySetValueIsLogged() { - MockHttpSession live = new MockHttpSession(); - live.setAttribute("a", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - - Map.Entry entry = sandbox.entrySet().iterator().next(); - - assertEquals("1", entry.setValue("2")); - assertEquals("2", entry.getValue()); - assertEquals("2", sandbox.get("a")); - assertEquals(List.of(new SessionOperation.Put("a", "2")), sandbox.getOperations()); - } - - public void testClearLogsOneRemovePerKeyPresent() { - MockHttpSession live = new MockHttpSession(); - live.setAttribute("a", "1"); - live.setAttribute("b", "2"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - - sandbox.clear(); - - assertTrue(sandbox.isEmpty()); - assertEquals(2, sandbox.getOperations().size()); - assertTrue(sandbox.getOperations().contains(new SessionOperation.Remove("a"))); - assertTrue(sandbox.getOperations().contains(new SessionOperation.Remove("b"))); - assertEquals("1", live.getAttribute("a")); - } - - public void testWriteThroughRequestMapFromBackgroundThreadWarnsOnce() throws Exception { - MockHttpServletRequest request = new MockHttpServletRequest(); - MockHttpSession live = new MockHttpSession(); - request.setSession(live); - SessionMap requestMap = new SessionMap(request); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "longJob"); - SandboxedSessionMap finished = new SandboxedSessionMap(live, "finishedJob"); - AtomicReference failure = new AtomicReference<>(); - - try (LogCapture logs = new LogCapture(SandboxedSessionMap.class)) { - Thread background = new Thread(() -> { - try { - sandbox.runInSandbox(() -> { - requestMap.put("a", "s3cr3t"); - requestMap.put("b", "s3cr3t"); - sandbox.put("c", "3"); - return null; - }); - finished.runInSandbox(() -> null); - requestMap.put("d", "s3cr3t"); - } catch (Exception e) { - failure.set(e); - } - }); - background.start(); - background.join(5000); - - assertNull(failure.get()); - assertEquals("s3cr3t", live.getAttribute("a")); - assertEquals("s3cr3t", live.getAttribute("d")); - assertEquals(1, logs.messages().size()); - assertTrue(logs.messages().get(0).contains("longJob")); - assertFalse(logs.messages().get(0).contains("s3cr3t")); - } - } - - public void testRunInSandboxReturnsTheResultAndPropagatesFailures() throws Exception { - SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "p"); - - assertEquals("done", sandbox.runInSandbox(() -> "done")); - try { - sandbox.runInSandbox(() -> { - throw new IllegalStateException("boom"); - }); - fail("the work's failure should propagate"); - } catch (IllegalStateException expected) { - assertEquals("boom", expected.getMessage()); - } - } -} diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java deleted file mode 100644 index 02869d11cf..0000000000 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java +++ /dev/null @@ -1,267 +0,0 @@ -/* - * 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.interceptor.exec; - -import jakarta.servlet.http.HttpSession; -import jakarta.servlet.http.HttpSessionBindingEvent; -import jakarta.servlet.http.HttpSessionBindingListener; -import junit.framework.TestCase; -import org.apache.struts2.action.BackgroundSessionConflictAware; -import org.apache.struts2.action.SessionConflict; -import org.apache.struts2.action.SessionConflictResolution; -import org.springframework.mock.web.MockHttpServletRequest; -import org.springframework.mock.web.MockHttpSession; - -import java.util.ArrayList; -import java.util.Collections; -import java.util.List; -import java.util.function.Function; - -public class SessionSandboxReconcilerTest extends TestCase { - - private MockHttpSession live; - private MockHttpServletRequest request; - - @Override - protected void setUp() throws Exception { - super.setUp(); - live = new MockHttpSession(); - request = new MockHttpServletRequest(); - request.setSession(live); - } - - public void testOnlyLoggedKeysAreApplied() { - live.setAttribute("other", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - live.setAttribute("other", "changed"); - sandbox.put("b", "2"); - - SessionSandboxReconciler.reconcile(sandbox, request, new Object()); - - assertEquals("changed", live.getAttribute("other")); - assertEquals("2", live.getAttribute("b")); - } - - public void testSessionIdChangeDiscardsEverything() { - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - sandbox.put("b", "2"); - live.invalidate(); - HttpSession fresh = request.getSession(true); - - SessionSandboxReconciler.reconcile(sandbox, request, new Object()); - - assertNull(fresh.getAttribute("b")); - } - - public void testNoSessionDiscardsEverything() { - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - sandbox.put("b", "2"); - live.invalidate(); - - SessionSandboxReconciler.reconcile(sandbox, request, new Object()); - - assertNull(request.getSession(false)); - } - - public void testConflictKeepsCurrentByDefault() { - SandboxedSessionMap sandbox = conflictOnA(); - - SessionSandboxReconciler.reconcile(sandbox, request, new Object()); - - assertEquals("live", live.getAttribute("a")); - } - - public void testHookUseBackground() { - SandboxedSessionMap sandbox = conflictOnA(); - - SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> SessionConflictResolution.useBackground())); - - assertEquals("bg", live.getAttribute("a")); - } - - public void testHookReplaceWith() { - SandboxedSessionMap sandbox = conflictOnA(); - - SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> SessionConflictResolution.replaceWith("merged"))); - - assertEquals("merged", live.getAttribute("a")); - } - - public void testHookRemove() { - SandboxedSessionMap sandbox = conflictOnA(); - - SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> SessionConflictResolution.remove())); - - assertNull(live.getAttribute("a")); - } - - public void testHookSeesAllFourValues() { - SandboxedSessionMap sandbox = conflictOnA(); - List seen = new ArrayList<>(); - - SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { - seen.add(conflict); - return SessionConflictResolution.keepCurrent(); - })); - - assertEquals(List.of(new SessionConflict("a", "1", "live", "bg")), seen); - } - - public void testRemovalOfAKeyChangedElsewhereIsAConflict() { - live.setAttribute("a", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - live.setAttribute("a", "live"); - sandbox.remove("a"); - - SessionSandboxReconciler.reconcile(sandbox, request, new Object()); - - assertEquals("live", live.getAttribute("a")); - } - - public void testBackgroundInvalidateStartsAFreshSession() { - live.setAttribute("a", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - live.setAttribute("a", "live"); - sandbox.invalidate(); - sandbox.put("user", "u"); - List seen = new ArrayList<>(); - - SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { - seen.add(conflict); - return SessionConflictResolution.keepCurrent(); - })); - - HttpSession fresh = request.getSession(false); - assertTrue(live.isInvalid()); - assertNotSame(live, fresh); - assertEquals(List.of("user"), Collections.list(fresh.getAttributeNames())); - assertEquals("u", fresh.getAttribute("user")); - assertEquals(List.of(), seen); - } - - public void testHookExceptionPropagates() { - SandboxedSessionMap sandbox = conflictOnA(); - IllegalArgumentException thrown = new IllegalArgumentException("boom"); - - try { - SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { - throw thrown; - })); - fail("expected the hook's exception"); - } catch (IllegalArgumentException e) { - assertSame(thrown, e); - } - } - - public void testSessionInvalidatedAfterTheIdCheckDiscardsTheWrites() { - InvalidatedOnIdCheck session = new InvalidatedOnIdCheck(); - request.setSession(session); - SandboxedSessionMap sandbox = new SandboxedSessionMap(session, "p"); - sandbox.put("a", "bg"); - session.armed = true; - - SessionSandboxReconciler.reconcile(sandbox, request, new Object()); - - assertTrue(session.isInvalid()); - assertNull(request.getSession(false)); - } - - public void testBackgroundInvalidateOfASessionInvalidatedAfterTheIdCheckDiscardsTheWrites() { - InvalidatedOnIdCheck session = new InvalidatedOnIdCheck(); - request.setSession(session); - SandboxedSessionMap sandbox = new SandboxedSessionMap(session, "p"); - sandbox.invalidate(); - sandbox.put("user", "u"); - session.armed = true; - - SessionSandboxReconciler.reconcile(sandbox, request, new Object()); - - assertNull(request.getSession(false)); - } - - public void testSessionInvalidatedByTheHookDiscardsTheRemainingWrites() { - SandboxedSessionMap sandbox = conflictOnA(); - sandbox.put("b", "2"); - - SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { - live.invalidate(); - return SessionConflictResolution.useBackground(); - })); - - assertNull(request.getSession(false)); - } - - public void testListenerIllegalStateExceptionOnALiveSessionPropagates() { - IllegalStateException thrown = new IllegalStateException("listener"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - sandbox.put("a", new HttpSessionBindingListener() { - @Override - public void valueBound(HttpSessionBindingEvent event) { - throw thrown; - } - }); - - try { - SessionSandboxReconciler.reconcile(sandbox, request, new Object()); - fail("expected the listener's exception"); - } catch (IllegalStateException e) { - assertSame(thrown, e); - } - } - - public void testHookIllegalStateExceptionPropagates() { - SandboxedSessionMap sandbox = conflictOnA(); - IllegalStateException thrown = new IllegalStateException("hook"); - - try { - SessionSandboxReconciler.reconcile(sandbox, request, resolving(conflict -> { - throw thrown; - })); - fail("expected the hook's exception"); - } catch (IllegalStateException e) { - assertSame(thrown, e); - } - } - - private SandboxedSessionMap conflictOnA() { - live.setAttribute("a", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - live.setAttribute("a", "live"); - sandbox.put("a", "bg"); - return sandbox; - } - - private static BackgroundSessionConflictAware resolving(Function resolver) { - return resolver::apply; - } - - private static class InvalidatedOnIdCheck extends MockHttpSession { - private boolean armed; - - @Override - public String getId() { - String id = super.getId(); - if (armed) { - armed = false; - invalidate(); - } - return id; - } - } -} diff --git a/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java b/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java index 51f29e8bfe..ad3409bd1b 100644 --- a/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java +++ b/core/src/test/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcessTest.java @@ -24,9 +24,7 @@ import java.io.ObjectOutputStream; import java.security.SecureRandom; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; -import java.util.Map; import java.util.Random; import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; @@ -36,14 +34,9 @@ import java.util.concurrent.TimeUnit; import java.util.concurrent.TimeoutException; import java.util.concurrent.atomic.AtomicInteger; -import java.util.concurrent.atomic.AtomicReference; -import ognl.OgnlContext; import org.apache.struts2.StrutsInternalTestCase; -import org.apache.struts2.util.ValueStack; -import org.apache.struts2.util.ValueStackFactory; import org.springframework.mock.web.MockHttpServletRequest; -import org.springframework.mock.web.MockHttpSession; import org.apache.struts2.ActionContext; import org.apache.struts2.ActionInvocation; @@ -168,83 +161,6 @@ public void testUnpreparedProcess() throws ExecutionException, InterruptedExcept assertEquals("Background thread Unprepared has not been prepared!", bp.getException().getMessage()); } - public void testSandboxContextIsCopiedAtHandoverAndStaysAnOgnlContext() { - ValueStack stack = container.getInstance(ValueStackFactory.class).createValueStack(); - ActionContext requestContext = ActionContext.of(stack.getContext()).withSession(new HashMap<>()); - AtomicReference> seen = new AtomicReference<>(); - MockActionInvocationWithActionInvoker invocation = new MockActionInvocationWithActionInvoker(() -> { - seen.set(ActionContext.getContext().getContextMap()); - return "done"; - }); - invocation.setInvocationContext(requestContext); - SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "copy"); - - BackgroundProcess bp = new StrutsBackgroundProcess(invocation, "copy", Thread.NORM_PRIORITY) - .withSessionSandbox(sandbox) - .prepare(); - requestContext.put("afterHandover", "x"); - executor.execute(bp); - await().atMost(5, TimeUnit.SECONDS).until(bp::isDone); - - assertNull(bp.getException()); - Map background = seen.get(); - assertTrue(background instanceof OgnlContext); - assertNotSame(requestContext.getContextMap(), background); - assertFalse(background.containsKey("afterHandover")); - assertSame(sandbox, ActionContext.of(background).getSession()); - assertSame(stack, ActionContext.of(background).getValueStack()); - assertNotSame(sandbox, requestContext.getSession()); - } - - public void testSandboxHonoursTheContextBoundByBeforeInvocation() { - AtomicReference> seen = new AtomicReference<>(); - MockActionInvocationWithActionInvoker invocation = new MockActionInvocationWithActionInvoker(() -> { - seen.set(ActionContext.getContext().getContextMap()); - return "done"; - }); - invocation.setInvocationContext(ActionContext.of()); - SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "custom"); - - BackgroundProcess bp = new StrutsBackgroundProcess(invocation, "custom", Thread.NORM_PRIORITY) { - @Override - protected void beforeInvocation() { - ActionContext.bind(ActionContext.of().with("custom", "yes")); - } - }.withSessionSandbox(sandbox).prepare(); - executor.execute(bp); - await().atMost(5, TimeUnit.SECONDS).until(bp::isDone); - - assertNull(bp.getException()); - assertEquals("yes", seen.get().get("custom")); - assertSame(sandbox, ActionContext.of(seen.get()).getSession()); - } - - public void testSandboxKeepsEditsOfASuperCallingBeforeInvocation() { - AtomicReference> seen = new AtomicReference<>(); - MockActionInvocationWithActionInvoker invocation = new MockActionInvocationWithActionInvoker(() -> { - seen.set(ActionContext.getContext().getContextMap()); - return "done"; - }); - ActionContext requestContext = ActionContext.of(); - invocation.setInvocationContext(requestContext); - SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "edited"); - - BackgroundProcess bp = new StrutsBackgroundProcess(invocation, "edited", Thread.NORM_PRIORITY) { - @Override - protected void beforeInvocation() throws Exception { - super.beforeInvocation(); - ActionContext.getContext().with("edited", "yes"); - } - }.withSessionSandbox(sandbox).prepare(); - executor.execute(bp); - await().atMost(5, TimeUnit.SECONDS).until(bp::isDone); - - assertNull(bp.getException()); - assertEquals("yes", seen.get().get("edited")); - assertSame(sandbox, ActionContext.of(seen.get()).getSession()); - assertFalse(requestContext.getContextMap().containsKey("edited")); - } - private static class MockActionInvocationWithActionInvoker extends MockActionInvocation { private final Callable actionInvoker; 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 index ee42fa18b6..ac855a53f6 100644 --- 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 @@ -1,10 +1,10 @@ -# WW-3576 SessionMap Staleness and execAndWait Session Sandbox — Implementation Plan +# 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) and give `execAndWait` background processes a sandboxed session reconciled on the polling request. +**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. A new `SandboxedSessionMap` (snapshot + ordered operation log) is created by `ExecuteAndWaitInterceptor` on the request thread, installed by `StrutsBackgroundProcess` on the background thread, and reconciled by `SessionSandboxReconciler` on the polling request that sees `isDone()`, gated on the session id, with per-action conflict resolution through `BackgroundSessionConflictAware`. +**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`. @@ -18,15 +18,10 @@ - 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. -- The session-id gate is not overridable by the hook. ## 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`). -- **`put(key, null)`** on the sandbox: Servlet semantics treat it as a removal, so it is logged as one — Task 3 `testPutNullIsLoggedAsRemove`. -- **`entrySet()` entry `setValue` on the sandbox**: must be logged like a `put`, not lost — Task 3 `testEntrySetValueIsLogged`. -- **Visibility between the background thread and the polling request**: the operation log written on one thread is read on another — Task 3 makes `SandboxedSessionMap` methods `synchronized`; Task 5 `testBackgroundWritesReachTheRealSession` exercises it across real threads. -- **Hook throws**: the exception propagates from the polling request, and no partial write-back is hidden — Task 4 `testHookExceptionPropagates`. --- @@ -191,280 +186,10 @@ git add core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java core/sr git commit -m "WW-3576 fix(core): treat an invalidated session as absent and stop caching SessionMap entries" ``` -### Task 3: `SandboxedSessionMap` - -**Files:** -- Modify: `core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java` (add `protected SessionMap()` for subclasses that never touch a request) -- Create: `core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java` -- Create: `core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java` -- Test: `core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java` (extends `junit.framework.TestCase`) - -**Interfaces:** -- Produces: - - `public class SandboxedSessionMap extends SessionMap` — `public SandboxedSessionMap(HttpSession session, String processName)` copies all attributes (shallow) and records `session.getId()`; overrides `get`, `put`, `remove`, `containsKey`, `entrySet`, `clear`, `invalidate`, all `synchronized`. - - `public String getStartSessionId()`, `public String getProcessName()` - - `public synchronized List getOperations()` — unmodifiable copy, in order. - - `public synchronized Object getSnapshotValue(String key)` — the value at snapshot time, `null` if absent. - - `public sealed interface SessionOperation` with records `Put(String key, Object value)`, `Remove(String key)`, `Invalidate()`. - -- [ ] **Step 1: Write the failing tests** - -```java -public void testReadsSeeSnapshotPlusOwnWritesAndNeverTheLiveSession() { - MockHttpSession live = new MockHttpSession(); - live.setAttribute("a", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - - live.setAttribute("a", "changed"); - sandbox.put("b", "2"); - - assertEquals("1", sandbox.get("a")); - assertEquals("2", sandbox.get("b")); - assertNull(live.getAttribute("b")); - assertEquals(live.getId(), sandbox.getStartSessionId()); - assertEquals(List.of(new SessionOperation.Put("b", "2")), sandbox.getOperations()); -} - -public void testRemoveClearAndInvalidateAreLoggedInOrder() { - MockHttpSession live = new MockHttpSession(); - live.setAttribute("a", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - - sandbox.remove("a"); - sandbox.invalidate(); - sandbox.put("user", "u"); - - assertEquals(List.of(new SessionOperation.Remove("a"), new SessionOperation.Invalidate(), - new SessionOperation.Put("user", "u")), sandbox.getOperations()); - assertEquals(Map.of("user", "u"), new HashMap<>(sandbox)); - assertEquals("1", sandbox.getSnapshotValue("a")); -} - -public void testPutNullIsLoggedAsRemove() { - SandboxedSessionMap sandbox = new SandboxedSessionMap(new MockHttpSession(), "p"); - sandbox.put("a", null); - assertEquals(List.of(new SessionOperation.Remove("a")), sandbox.getOperations()); -} - -public void testEntrySetValueIsLogged() { - MockHttpSession live = new MockHttpSession(); - live.setAttribute("a", "1"); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "p"); - - sandbox.entrySet().iterator().next().setValue("2"); - - assertEquals("2", sandbox.get("a")); - assertEquals(List.of(new SessionOperation.Put("a", "2")), sandbox.getOperations()); -} -``` - -Also: `clear()` logs one `Remove` per key present in the view at that moment. - -- [ ] **Step 2: Run them and verify they fail to compile** - -Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=SandboxedSessionMapTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` -Expected: `exit=1`, compilation error `cannot find symbol: class SandboxedSessionMap`. - -- [ ] **Step 3: Implement `SessionOperation`, `SandboxedSessionMap` and `protected SessionMap()`** - -View is a `LinkedHashMap` copied from the session; `invalidate()` clears the view and logs `Invalidate`. `SandboxedSessionMap` keeps its own fields and never touches `session`, `request` or the deprecated `entries`. It is not `Serializable`-relevant: the process holds it `transient` (Task 5). - -- [ ] **Step 4: Run the tests and verify they pass** - -Same command as Step 2. Expected: `exit=0`. - -- [ ] **Step 5: Commit** - -```bash -git add core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java core/src/main/java/org/apache/struts2/interceptor/exec/SessionOperation.java core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java -git commit -m "WW-3576 feat(core): add a sandboxed session map recording background writes" -``` - -### Task 4: Reconciliation and the per-action conflict hook - -**Files:** -- Create: `core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java` -- Create: `core/src/main/java/org/apache/struts2/action/SessionConflict.java` -- Create: `core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java` -- Create: `core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java` -- Test: `core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java` (extends `junit.framework.TestCase`) - -**Interfaces:** -- Consumes: Task 3's `SandboxedSessionMap`, `SessionOperation`. -- Produces: - - `public record SessionConflict(String key, Object snapshotValue, Object currentValue, Object backgroundValue)` — `null` means absent. - - `public final class SessionConflictResolution` — `static keepCurrent()`, `static useBackground()`, `static replaceWith(Object value)`, `static remove()`. - - `public interface BackgroundSessionConflictAware { SessionConflictResolution resolveSessionConflict(SessionConflict conflict); }` - - `public final class SessionSandboxReconciler` — `public static void reconcile(SandboxedSessionMap sandbox, HttpServletRequest request, Object action)`. - -- [ ] **Step 1: Write the failing tests** - -Fixture: `MockHttpServletRequest` with a `MockHttpSession live`; sandbox built from `live`. - -```java -public void testOnlyLoggedKeysAreApplied() // live "other" changed after snapshot, untouched by background → kept; background put "b" → applied -public void testSessionIdChangeDiscardsEverything() // live.invalidate(); request.getSession(true) new; background put "b" → new session has no "b"; one debug line names the process -public void testNoSessionDiscardsEverything() // live.invalidate(), no new session → reconcile returns, request.getSession(false) still null -public void testConflictKeepsCurrentByDefault() // snapshot a=1, live a=live, background a=bg → live a stays "live" -public void testHookUseBackground() // action returns useBackground() → a=bg -public void testHookReplaceWith() // replaceWith("merged") → a=merged -public void testHookRemove() // remove() → a absent -public void testHookSeesAllFourValues() // assert SessionConflict("a", "1", "live", "bg") -public void testRemovalOfAKeyChangedElsewhereIsAConflict() // snapshot a=1, live a=live, background remove a → default keeps "live" -public void testBackgroundInvalidateStartsAFreshSession() // background invalidate + put user=u → old live invalid, new session has only user=u, hook never called -public void testHookExceptionPropagates() // hook throws IllegalArgumentException → reconcile throws it -``` - -Each name above is one test with exactly the assertion in its comment. - -- [ ] **Step 2: Run them and verify they fail to compile** - -Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=SessionSandboxReconcilerTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` -Expected: `exit=1`, `cannot find symbol: class SessionSandboxReconciler`. - -- [ ] **Step 3: Implement the types and `reconcile`** - -Algorithm (not determined by the signature): - -``` -real = request.getSession(false) -if real == null or !real.getId().equals(sandbox.getStartSessionId()): LOG.debug(process name); return -ops = sandbox.getOperations(); last = index of last Invalidate, or -1 -if last >= 0: - real.invalidate(); target = request.getSession(true) - apply final value of every key touched after `last` to target (null → removeAttribute); no conflict checks - return -for each key touched in ops (in first-touch order), final = sandbox.get(key): - current = real.getAttribute(key); snapshot = sandbox.getSnapshotValue(key) - if Objects.equals(current, snapshot): write final (null → removeAttribute) - else if action instanceof BackgroundSessionConflictAware: apply its resolution - else: LOG.debug(process name, key); keep current -``` - -- [ ] **Step 4: Run the tests and verify they pass** - -Same command as Step 2. Expected: `exit=0`. - -- [ ] **Step 5: Commit** - -```bash -git add core/src/main/java/org/apache/struts2/action/BackgroundSessionConflictAware.java core/src/main/java/org/apache/struts2/action/SessionConflict.java core/src/main/java/org/apache/struts2/action/SessionConflictResolution.java core/src/main/java/org/apache/struts2/interceptor/exec/SessionSandboxReconciler.java core/src/test/java/org/apache/struts2/interceptor/exec/SessionSandboxReconcilerTest.java -git commit -m "WW-3576 feat(core): reconcile a session sandbox with per-action conflict resolution" -``` - -### Task 5: Wire the sandbox into `execAndWait` - -**Files:** -- Modify: `core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java` -- Modify: `core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java:64-92` -- Modify: `core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java:296-345` -- Test: `core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java` (extends `StrutsInternalTestCase`), plus a test action `core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java` implementing `SessionAware`, registered in `WaitConfigurationProvider` as `sessionAction` - -**Interfaces:** -- Consumes: Tasks 3–4. -- Produces: - - `BackgroundProcess`: `default BackgroundProcess withSessionSandbox(SandboxedSessionMap sandbox) { return this; }`, `default SandboxedSessionMap getSessionSandbox() { return null; }` - - `StrutsBackgroundProcess` overrides both; field `private transient SandboxedSessionMap sessionSandbox`. - -- [ ] **Step 1: Write the failing tests** - -`SessionWritingWaitAction.execute()` writes `"result" → "done"` through `ActionContext.getContext().getSession()` and `"aware" → "yes"` through its injected `SessionAware` map; a field `failWith` makes it throw after writing. - -```java -public void testBackgroundWritesReachTheRealSession() // first proxy → "wait"; wait for done; second proxy → "success"; httpSession has result=done and aware=yes -public void testBackgroundWritesAreInvisibleUntilPolled() // after done but before the second proxy: httpSession has neither "result" nor "aware" -public void testWritesOfAFailedActionAreApplied() // failWith set → second proxy throws; httpSession has result=done -public void testSessionAwareGetsTheLiveMapBackAfterWriteBack() // after second proxy, the action's session map is not a SandboxedSessionMap -``` - -The existing setUp puts a plain `HashMap` as the context session; these tests replace it with `new SessionMap(request)` so the context session is backed by `httpSession`. - -- [ ] **Step 2: Run them and verify they fail** - -Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=ExecuteAndWaitInterceptorTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` -Expected: `exit=1`; `testBackgroundWritesAreInvisibleUntilPolled` and `testSessionAwareGetsTheLiveMapBackAfterWriteBack` fail (writes are live today). - -- [ ] **Step 3: Implement the wiring** - -- Interceptor, creation branch: `new SandboxedSessionMap(httpSession, name)` and `getNewBackgroundProcess(...).withSessionSandbox(sandbox).prepare()`. -- Interceptor, `isDone()` branch, after `session.remove(bp_SessionKey)` and **before** `throw bp.getException()`: if `bp.getSessionSandbox()` is non-null, `SessionSandboxReconciler.reconcile(sandbox, ServletActionContext.getRequest(), bp.getAction())`, then re-inject `context.getSession()` into a `SessionAware` action. -- `StrutsBackgroundProcess` thread body: `beforeInvocation(); installSessionSandbox(); result = invocation.invokeActionOnly();` — `installSessionSandbox()` is private: when a sandbox is set, bind `ActionContext.of(new HashMap<>(invocation.getInvocationContext().getContextMap())).withSession(sandbox)` and call `withSession(sandbox)` on a `SessionAware` action. - -- [ ] **Step 4: Run the tests and verify they pass** - -Same command as Step 2. Expected: `exit=0`, existing tests (`testFromDeserializedSession`, token tests) still pass. - -- [ ] **Step 5: Commit** - -```bash -git add core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java core/src/test/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptorTest.java core/src/test/java/org/apache/struts2/interceptor/SessionWritingWaitAction.java -git commit -m "WW-3576 feat(core): run execAndWait background processes against a session sandbox" -``` - -### Task 6: Warn on sandbox escapes and document them - -**Files:** -- Modify: `core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java` -- Modify: `core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java` -- Modify: `core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java` -- Modify: Javadoc of `ExecuteAndWaitInterceptor` and `BackgroundProcess` -- Test: `core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java` - -**Interfaces:** -- Produces: `public static SandboxedSessionMap current()` (thread-local backed, read by `SessionMap`); `public static void runInSandbox(SandboxedSessionMap sandbox, Runnable work)` sets `current()` for the duration of `work` and clears it in `finally`; `void warnEscapeOnce()`. `StrutsBackgroundProcess`'s thread body runs `invokeActionOnly()` inside `runInSandbox` when a sandbox is set. - -- [ ] **Step 1: Write the failing test** - -```java -public void testWriteThroughRequestMapFromBackgroundThreadWarnsOnce() throws Exception { - MockHttpServletRequest request = new MockHttpServletRequest(); - MockHttpSession live = new MockHttpSession(); - request.setSession(live); - SessionMap requestMap = new SessionMap(request); - SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "longJob"); - - try (LogCapture logs = new LogCapture(SandboxedSessionMap.class)) { - Thread background = new Thread(() -> SandboxedSessionMap.runInSandbox(sandbox, () -> { - requestMap.put("a", "1"); - requestMap.put("b", "2"); - sandbox.put("c", "3"); - })); - background.start(); - background.join(5000); - - assertEquals("1", live.getAttribute("a")); - assertEquals(1, logs.messages().size()); - assertTrue(logs.messages().get(0).contains("longJob")); - assertFalse(logs.messages().get(0).contains("1")); - } -} -``` - -`runInSandbox(SandboxedSessionMap, Runnable)` is the package-level helper `StrutsBackgroundProcess` uses to set and clear `current()`. - -- [ ] **Step 2: Run it and verify it fails to compile** - -Run: `./mvnw -q test -DskipAssembly -pl core -Dtest=SandboxedSessionMapTest -Dsurefire.failIfNoSpecifiedTests=false; echo exit=$?` -Expected: `exit=1`, `cannot find symbol: method runInSandbox`. - -- [ ] **Step 3: Implement the escape warning and Javadoc** - -`SessionMap.put/remove/clear/invalidate`: when `SandboxedSessionMap.current()` is non-null and `this` is not a `SandboxedSessionMap`, call `warnEscapeOnce()`; the write still goes live. WARN (once per sandbox, `AtomicBoolean`): `Background process [{}] wrote to the request session, bypassing its session sandbox; use ActionContext.getContext().getSession() or SessionAware`. Javadoc on `ExecuteAndWaitInterceptor` and `BackgroundProcess`: the sandbox model, that `#session` and `invocation.getInvocationContext().getSession()` bypass it, and `BackgroundSessionConflictAware`. - -- [ ] **Step 4: Run the full core suite** - -Run: `./mvnw -q test -DskipAssembly -pl core > /private/tmp/claude-501/-Users-lukaszlenart-Projects-Apache-struts/9c5c77a5-710a-4c52-9f6b-ec54c8d4b6aa/scratchpad/core-tests.log 2>&1; echo exit=$?` -Expected: `exit=0`. - -- [ ] **Step 5: Commit** - -```bash -git add core/src/main/java/org/apache/struts2/interceptor/exec/SandboxedSessionMap.java core/src/main/java/org/apache/struts2/interceptor/exec/StrutsBackgroundProcess.java core/src/main/java/org/apache/struts2/dispatcher/SessionMap.java core/src/main/java/org/apache/struts2/interceptor/ExecuteAndWaitInterceptor.java core/src/main/java/org/apache/struts2/interceptor/exec/BackgroundProcess.java core/src/test/java/org/apache/struts2/interceptor/exec/SandboxedSessionMapTest.java -git commit -m "WW-3576 feat(core): warn when a background process writes around its session sandbox" -``` +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, sandbox model, `#session` escape, custom `BackgroundProcess` keeps shared map) and the struts-site `execAndWait` page. +- [ ] **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 index 2dfc042086..87bb488cea 100644 --- 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 @@ -1,4 +1,4 @@ -# WW-3576 — SessionMap staleness and the execAndWait session snapshot +# 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`). @@ -19,27 +19,20 @@ two symptoms are both staleness: 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 (request threads), and one map that is stale *by design* and explicit about it -(the `execAndWait` background thread), with a controlled write-back. +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. -- `execAndWait` background code gets a session view that does not depend on the (recycled) request. -- Background writes reach the real session once, at a well-defined point, never across users. -- Per-action control over write-back conflicts via a `*Aware` interface. +- No map resolves a session through a request that has already ended. ## Non-goals -- Making the rest of the shared `ActionContext` (request, response, parameters) safe for background - threads. -- Live, mid-process propagation of background writes. Progress reporting already goes through the - action (`ExecuteAndWaitInterceptor` pushes `bp.getAction()` on every poll), not through the session. -- A global (application-wide) conflict strategy bean. -- Enforcing the sandbox (throwing on escapes) or isolating the background thread's value stack and - `ActionContext`. That is a rewrite of the `execAndWait` mechanism to run in a real sandbox, planned - for 8.0.0 as [WW-5787](https://issues.apache.org/jira/browse/WW-5787). +- 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 @@ -48,11 +41,10 @@ when that session dies (request threads), and one map that is stale *by design* "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. -- Any path by which background writes land in a session other than the one the process started in. ## Design -### 1. Request-scoped `SessionMap` (default) +### 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 @@ -69,7 +61,7 @@ when that session dies (request threads), and one map that is stale *by design* 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 (sandbox escape, custom + 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 @@ -88,87 +80,13 @@ when that session dies (request threads), and one map that is stale *by design* 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. - -### 2. Sandboxed background session (`execAndWait` only) - -A `SessionMap` subclass (so existing `((SessionMap) session).invalidate()` casts keep working) that -never touches the request: - -- **Taken on the request thread**, when the interceptor creates the `BackgroundProcess` (inside the - existing `synchronized (httpSession)` block) — never on the background thread, where the request - may already be recycled. Holds: a shallow copy of the session attributes, the session id at that - moment, and a log of its own writes (puts, removes, and an invalidate request). -- Reads return the copy with the logged writes applied. -- **Installed for the background thread:** the background thread binds an `ActionContext` that is a - copy of the invocation context with `withSession(snapshot)`, so the original request thread keeps - its own request map. If the action is `SessionAware`, it is re-injected with the snapshot before the - invocation runs. -- **Sandbox model (decided):** the background process works only on its sandboxed session; the - supported way to change the real session is to write to the sandbox and let write-back reconcile it - (with the per-action hook deciding conflicts). There is deliberately no callback handing background - code the real session. Supported entry points: `ActionContext.getContext().getSession()` and - `SessionAware`. -- **Sandbox escapes (decided: warn only in 7.5.0):** OGNL `#session` and - `invocation.getInvocationContext().getSession()` still reach the request's pinned map, and writes - through them go live immediately, as today. A write through a request map from an `execAndWait` - background thread logs one WARN per background process (action name, never attribute values), - pointing to the sandbox entry points. Documented as dangerous in the `ExecuteAndWaitInterceptor` / - `BackgroundProcess` Javadoc, the `execAndWait` docs on struts-site, and the 7.5.0 migration notes. How - the map recognises a background thread is the builder's. -- *(Builder-check call, refined while planning)* Handed over through **default methods on - `BackgroundProcess`** — a fluent `withSessionSandbox(…)` (no-op by default) called by the interceptor - on whatever the overridable `getNewBackgroundProcess(…)` factory returns, and `getSessionSandbox()` - (null by default). Installing it happens in `StrutsBackgroundProcess`'s private thread body, *after* - the overridable `beforeInvocation()` (whose default binds the handoff copy, so a super-calling - override edits the context the action will use), so neither `super` nor a subclass that skips it can - undo it. A custom `BackgroundProcess` not - extending `StrutsBackgroundProcess` keeps today's shared-map behaviour — migration-notes line. -- Not serialized: held `transient` by the background process. A deserialized process already has a - null invocation and is restarted (WW-4900), so the snapshot is lost with it. - -### 3. Write-back - -- Happens on the **polling request that observes `bp.isDone()`** (the `else if (… bp.isDone())` branch - in `ExecuteAndWaitInterceptor#doIntercept`), on a request thread, inside the existing - `synchronized (httpSession)`, before the result is returned. -- **Session-id gate (decided, not overridable):** if the current session id differs from the snapshot's - start id, or there is no session, all background writes are discarded and logged at debug. -- Otherwise apply **only the logged writes**: keys the background never touched are left alone. -- A logged `invalidate()` is applied after the id gate passes; the conflict hook is not consulted for it. -- After write-back, a `SessionAware` action is re-injected with the request's session map so the - result rendering writes to the live session. -- If nobody polls after completion, the writes are never applied (the session expires with them). -- If another request invalidates the session during write-back, the remaining writes are discarded and - no replacement session is created; an exception from the conflict hook still propagates. -- When write-back replaces the session (logged `invalidate()`), the polling context gets a new request - `SessionMap` (and `DispatcherConstants.SESSION` helper) before re-injection. -- *(Builder-check call)* Write-back runs **before** `throw bp.getException()`: a failed background - action's writes are applied, matching today's behaviour where its writes were already live. -- *(Builder-check call)* The write log is an **ordered operation list** (put / remove / invalidate). - After a logged `invalidate()`, snapshot reads see an empty session. At write-back the real session is - invalidated, a fresh one is created on the polling request (`getSession(true)` — the background asked - for this, e.g. a login flow, so it is not the anti-goal's "absorb a write after *another* request - invalidated"), and the operations after the invalidate are applied to it without conflict checks. - -### 4. Conflicts and the per-action hook - -- **Conflict:** a key the background wrote (put or remove) whose value in the real session no longer - `equals` the snapshot value (absent counts as a value). Known limit: an object mutated in place - without a new `setAttribute` is the same reference, so it is not detected. -- **Default (decided):** keep the real session's value, drop the background's, log at debug. -- **Hook (decided: per action, `*Aware`):** an action implementing the new interface is consulted once - per conflicting key with: key, snapshot value, current value, background value. It must be able to - express: keep current, take background, set a different value, remove the key. Lives in - `org.apache.struts2.action` next to `SessionAware`/`ParameterNameAware`. Proposed names (builder may - refine): `BackgroundSessionConflictAware`, method - `resolveSessionConflict(SessionConflict)` returning a `SessionConflictResolution`. -- An exception thrown by the hook propagates like any other action exception from the polling request. +- `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`, -`ExecuteAndWaitInterceptorTest` extends `StrutsInternalTestCase` (both JUnit 3 — `testXxx()` names, no -`@Test`). +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 @@ -180,18 +98,10 @@ Match the target file's JUnit style: `SessionMapTest` extends `junit.framework.T `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. Snapshot: reads/writes isolated from the live session while running; write-back applies only logged - keys; id change discards everything; default conflict keeps current; hook outcomes (all four); - logged invalidate applied; `SessionAware` re-injection in both directions. -5. Sandbox escape: a write through the request map from a background thread goes live and logs exactly - one WARN per process; writes through the sandbox log nothing. -6. Existing `SessionMapTest`, `ExecuteAndWaitInterceptorTest` and the tests constructing +4. Existing `SessionMapTest`, `ExecuteAndWaitInterceptorTest` and the tests constructing `new SessionMap(request)` keep passing. ## Left to the builder -- Class names of the snapshot map and the conflict/resolution types; whether resolution is an enum plus - value or a small sealed hierarchy. -- How the `ActionContext` copy is made for the background thread. - Whether `CreateSessionInterceptor` is simplified in this change. - Debug log wording.