This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch feature/WW-3576-session-map-staleness in repository https://gitbox.apache.org/repos/asf/struts.git
commit 6fba74972a34c4598d4aca1fa006f8cf452f3b43 Author: Lukasz Lenart <[email protected]> AuthorDate: Sun Oct 18 10:34:20 2026 +0200 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 <[email protected]> --- .../org/apache/struts2/dispatcher/SessionMap.java | 40 ++++++++++++++----- .../interceptor/exec/BackgroundProcess.java | 12 ++++++ .../interceptor/exec/SandboxedSessionMap.java | 45 +++++++++++++--------- .../struts2/interceptor/exec/SessionOperation.java | 2 +- .../interceptor/exec/SessionSandboxReconciler.java | 8 ++++ .../interceptor/exec/StrutsBackgroundProcess.java | 14 +------ .../interceptor/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 3448ffffa..801f9c3e3 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 @@ package org.apache.struts2.dispatcher; 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.Collections; 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<String, Object> implements Serializa @Serial private static final long serialVersionUID = 4678843241638046854L; + private static final ThreadLocal<Runnable> 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 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa * Invalidate the http session. */ public void invalidate() { - warnIfSandboxBypassed(); + notifyLiveWrite(); HttpSession current = session; if (current == null) { return; @@ -93,7 +95,7 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @Override public void clear() { - warnIfSandboxBypassed(); + notifyLiveWrite(); HttpSession current = session; if (current == null) { return; @@ -131,6 +133,7 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa 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 class SessionMap extends AbstractMap<String, Object> implements Serializa */ @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 class SessionMap extends AbstractMap<String, Object> implements Serializa 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 <T> the type of the work's result + * @return the work's result + * @throws Exception whatever the work throws + */ + protected static <T> T callWithLiveWriteListener(final Runnable listener, final Callable<T> 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 @@ public class SessionMap extends AbstractMap<String, Object> implements Serializa */ @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 b17100b09..9a87e4958 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 e9e8b2619..81a315da7 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.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. + * 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<SandboxedSessionMap> CURRENT = new ThreadLocal<>(); private final transient Map<String, Object> snapshot = new LinkedHashMap<>(); private final transient Map<String, Object> 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 class SandboxedSessionMap extends SessionMap { view.putAll(snapshot); } - public static SandboxedSessionMap current() { - return CURRENT.get(); + <T> T runInSandbox(Callable<T> 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<SessionOperation> getOperations() { + synchronized List<SessionOperation> 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 class SandboxedSessionMap extends SessionMap { 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 class SandboxedSessionMap extends SessionMap { } } + /** + * 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 c07927040..342b70183 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 0f76f283b..f34a9462e 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 d05220a74..6ad0f2967 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 @@ public class StrutsBackgroundProcess implements BackgroundProcess, Serializable 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 115ce1da2..c000da4ad 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 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 { @@ -99,21 +100,47 @@ public class SandboxedSessionMapTest extends TestCase { request.setSession(live); SessionMap requestMap = new SessionMap(request); SandboxedSessionMap sandbox = new SandboxedSessionMap(live, "longJob"); + SandboxedSessionMap finished = new SandboxedSessionMap(live, "finishedJob"); + AtomicReference<Exception> 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()); } }
