This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5650-json-per-request-reader-writer in repository https://gitbox.apache.org/repos/asf/struts.git
commit b19f51aae431dc8e0606396412ff4842272c54eb Author: Lukasz Lenart <[email protected]> AuthorDate: Tue Jul 14 09:52:50 2026 +0200 WW-5650 revert StrutsJSONWriter to plain single-use instance fields --- .../org/apache/struts2/json/StrutsJSONWriter.java | 114 +++++++++------------ .../apache/struts2/json/StrutsJSONWriterTest.java | 74 ------------- 2 files changed, 48 insertions(+), 140 deletions(-) diff --git a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java index 59fadd9b3..08feb705e 100644 --- a/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java +++ b/plugins/json/src/main/java/org/apache/struts2/json/StrutsJSONWriter.java @@ -66,13 +66,9 @@ import java.util.regex.Pattern; * Serializes an object into JavaScript Object Notation (JSON). If cyclic * references are detected they will be nulled out. * </p> - * * <p> - * A single StrutsJSONWriter instance is shared across all concurrent responses handled by a given - * JSONResult/JSONInterceptor (it is injected once, not created per request), so the output buffer, - * cycle-detection stack and other per-write state are kept in a {@link ThreadLocal}, not instance - * fields -- otherwise two concurrent {@link #write(Object)} calls would corrupt each other's output, - * including returning one request's serialized data as another, unrelated request's response. + * This writer keeps per-write state in instance fields and is <strong>not thread-safe</strong>; + * obtain a fresh instance per write (the container serves it as a prototype bean). See WW-5650. * </p> */ public class StrutsJSONWriter implements JSONWriter { @@ -92,22 +88,17 @@ public class StrutsJSONWriter implements JSONWriter { BEAN_INFO_CACHE.clear(); } - private static final class WriteState { - private final StringBuilder buf = new StringBuilder(); - private final Deque<Object> stack = new ArrayDeque<>(); - private Object root; - private boolean buildExpr = true; - private String exprStack = ""; - private Collection<Pattern> excludeProperties; - private Collection<Pattern> includeProperties; - private boolean excludeNullProperties; - } - - private static final ThreadLocal<WriteState> WRITE_STATE = new ThreadLocal<>(); - + private final StringBuilder buf = new StringBuilder(); + private final Deque<Object> stack = new ArrayDeque<>(); private boolean ignoreHierarchy = true; + private Object root; + private boolean buildExpr = true; + private String exprStack = ""; + private Collection<Pattern> excludeProperties; + private Collection<Pattern> includeProperties; private DateFormat dateFormat; private boolean enumAsBean = ENUM_AS_BEAN_DEFAULT; + private boolean excludeNullProperties; private boolean excludeProxyProperties; private ProxyService proxyService; @@ -142,20 +133,18 @@ public class StrutsJSONWriter implements JSONWriter { @Override public String write(Object object, Collection<Pattern> excludeProperties, Collection<Pattern> includeProperties, boolean excludeNullProperties) throws JSONException { - WriteState state = new WriteState(); - state.excludeNullProperties = excludeNullProperties; - state.root = object; - state.buildExpr = ((excludeProperties != null) && !excludeProperties.isEmpty()) + this.excludeNullProperties = excludeNullProperties; + this.buf.setLength(0); + this.stack.clear(); + this.root = object; + this.exprStack = ""; + this.buildExpr = ((excludeProperties != null) && !excludeProperties.isEmpty()) || ((includeProperties != null) && !includeProperties.isEmpty()); - state.excludeProperties = excludeProperties; - state.includeProperties = includeProperties; - WRITE_STATE.set(state); - try { - this.value(object, null); - return state.buf.toString(); - } finally { - WRITE_STATE.remove(); - } + this.excludeProperties = excludeProperties; + this.includeProperties = includeProperties; + this.value(object, null); + + return this.buf.toString(); } /** @@ -171,7 +160,7 @@ public class StrutsJSONWriter implements JSONWriter { return; } - if (WRITE_STATE.get().stack.contains(object)) { + if (this.stack.contains(object)) { Class<?> clazz = object.getClass(); // cyclic reference @@ -196,8 +185,7 @@ public class StrutsJSONWriter implements JSONWriter { * @throws JSONException in case of error during serialize */ protected void process(Object object, Method method) throws JSONException { - WriteState state = WRITE_STATE.get(); - state.stack.push(object); + this.stack.push(object); if (object instanceof Class) { this.string(object); @@ -229,7 +217,7 @@ public class StrutsJSONWriter implements JSONWriter { processCustom(object, method); } - state.stack.pop(); + this.stack.pop(); } /** @@ -252,13 +240,12 @@ public class StrutsJSONWriter implements JSONWriter { protected void bean(Object object) throws JSONException { this.add("{"); - WriteState state = WRITE_STATE.get(); BeanInfo info; try { Class<?> clazz = excludeProxyProperties ? proxyService.ultimateTargetClass(object) : object.getClass(); - info = ((object == state.root) && this.ignoreHierarchy) + info = ((object == this.root) && this.ignoreHierarchy) ? getBeanInfoIgnoreHierarchy(clazz) : getBeanInfo(clazz); @@ -284,7 +271,7 @@ public class StrutsJSONWriter implements JSONWriter { continue; } String expr = null; - if (state.buildExpr) { + if (this.buildExpr) { expr = this.expandExpr(name); if (this.shouldExcludeProperty(expr)) { continue; @@ -299,7 +286,7 @@ public class StrutsJSONWriter implements JSONWriter { boolean propertyPrinted = this.add(name, value, accessor, hasData); hasData = hasData || propertyPrinted; - if (state.buildExpr) { + if (this.buildExpr) { this.setExprStack(expr); } } @@ -414,28 +401,25 @@ public class StrutsJSONWriter implements JSONWriter { } protected String expandExpr(int i) { - return WRITE_STATE.get().exprStack + "[" + i + "]"; + return this.exprStack + "[" + i + "]"; } protected String expandExpr(String property) { - String exprStack = WRITE_STATE.get().exprStack; - if (exprStack.isEmpty()) { + if (this.exprStack.isEmpty()) { return property; } - return exprStack + "." + property; + return this.exprStack + "." + property; } protected String setExprStack(String expr) { - WriteState state = WRITE_STATE.get(); - String s = state.exprStack; - state.exprStack = expr; + String s = this.exprStack; + this.exprStack = expr; return s; } protected boolean shouldExcludeProperty(String expr) { - WriteState state = WRITE_STATE.get(); - if (state.excludeProperties != null) { - for (Pattern pattern : state.excludeProperties) { + if (this.excludeProperties != null) { + for (Pattern pattern : this.excludeProperties) { if (pattern.matcher(expr).matches()) { if (LOG.isDebugEnabled()) { LOG.debug("Ignoring property because of exclude rule: " + expr); @@ -445,8 +429,8 @@ public class StrutsJSONWriter implements JSONWriter { } } - if (state.includeProperties != null) { - for (Pattern pattern : state.includeProperties) { + if (this.includeProperties != null) { + for (Pattern pattern : this.includeProperties) { if (pattern.matcher(expr).matches()) { return false; } @@ -463,7 +447,7 @@ public class StrutsJSONWriter implements JSONWriter { * Add name/value pair to buffer */ protected boolean add(String name, Object value, Method method, boolean hasData) throws JSONException { - if (WRITE_STATE.get().excludeNullProperties && value == null) { + if (excludeNullProperties && value == null) { return false; } if (hasData) { @@ -482,25 +466,24 @@ public class StrutsJSONWriter implements JSONWriter { protected void map(Map<?, ?> map, Method method) throws JSONException { this.add("{"); - WriteState state = WRITE_STATE.get(); Iterator<?> it = map.entrySet().iterator(); boolean warnedNonString = false; // one report per map boolean hasData = false; while (it.hasNext()) { Map.Entry<?, ?> entry = (Map.Entry<?, ?>) it.next(); - if (state.excludeNullProperties && entry.getValue() == null) { + if (excludeNullProperties && entry.getValue() == null) { continue; } Object key = entry.getKey(); if (key == null) { - LOG.error("Cannot build expression for null key in {}", state.exprStack); + LOG.error("Cannot build expression for null key in {}", exprStack); continue; } String expr = null; - if (state.buildExpr) { + if (this.buildExpr) { expr = this.expandExpr(key.toString()); if (this.shouldExcludeProperty(expr)) { continue; @@ -520,7 +503,7 @@ public class StrutsJSONWriter implements JSONWriter { this.value(key.toString(), method); this.add(":"); this.value(entry.getValue(), method); - if (state.buildExpr) { + if (this.buildExpr) { this.setExprStack(expr); } } @@ -587,11 +570,10 @@ public class StrutsJSONWriter implements JSONWriter { protected void array(Iterator<?> it, Method method) throws JSONException { this.add("["); - WriteState state = WRITE_STATE.get(); boolean hasData = false; for (int i = 0; it.hasNext(); i++) { String expr = null; - if (state.buildExpr) { + if (this.buildExpr) { expr = this.expandExpr(i); if (this.shouldExcludeProperty(expr)) { it.next(); @@ -604,7 +586,7 @@ public class StrutsJSONWriter implements JSONWriter { } hasData = true; this.value(it.next(), method); - if (state.buildExpr) { + if (this.buildExpr) { this.setExprStack(expr); } } @@ -618,13 +600,12 @@ public class StrutsJSONWriter implements JSONWriter { protected void array(Object object, Method method) throws JSONException { this.add("["); - WriteState state = WRITE_STATE.get(); int length = Array.getLength(object); boolean hasData = false; for (int i = 0; i < length; ++i) { String expr = null; - if (state.buildExpr) { + if (this.buildExpr) { expr = this.expandExpr(i); if (this.shouldExcludeProperty(expr)) { continue; @@ -636,7 +617,7 @@ public class StrutsJSONWriter implements JSONWriter { } hasData = true; this.value(Array.get(object, i), method); - if (state.buildExpr) { + if (this.buildExpr) { this.setExprStack(expr); } } @@ -692,14 +673,14 @@ public class StrutsJSONWriter implements JSONWriter { * Add object to buffer */ protected void add(Object obj) { - WRITE_STATE.get().buf.append(obj); + this.buf.append(obj); } /* * Add char to buffer */ protected void add(char c) { - WRITE_STATE.get().buf.append(c); + this.buf.append(c); } /** @@ -782,4 +763,5 @@ public class StrutsJSONWriter implements JSONWriter { return this; } } + } diff --git a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java index 2239c5f71..acc52dcaa 100644 --- a/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java +++ b/plugins/json/src/test/java/org/apache/struts2/json/StrutsJSONWriterTest.java @@ -39,11 +39,6 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; import java.util.TimeZone; -import java.util.concurrent.CountDownLatch; -import java.util.concurrent.ExecutorService; -import java.util.concurrent.Executors; -import java.util.concurrent.TimeUnit; -import java.util.concurrent.atomic.AtomicInteger; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -320,73 +315,4 @@ public class StrutsJSONWriterTest { assertTrue(json.contains("\"calendar\":\"2012-12-23T10:10:10\"")); } - public static class NamedBean { - private final String owner; - private final String secret; - - public NamedBean(String owner, String secret) { - this.owner = owner; - this.secret = secret; - } - - public String getOwner() { return owner; } - public String getSecret() { return secret; } - } - - /** - * A single StrutsJSONWriter instance is injected once into JSONUtil/JSONResult and reused - * across every concurrent response handled by that result, so write() must be safe to call - * concurrently from multiple threads on the same instance without one call's output buffer - * leaking into -- or being overwritten by -- another call's output. - * - * <p>Uses a higher thread count than the minimum needed to demonstrate the bug: with only two - * threads on a machine with many cores, the OS scheduler has no need to preempt either thread - * mid-call, so the race window is rarely hit and the test can pass even against the unpatched - * code. Sixteen threads contending for the same instance reproduces the corruption reliably.</p> - */ - @Test - public void testConcurrentReuseDoesNotSwapResponsesAcrossWrites() throws Exception { - JSONWriter sharedWriter = new StrutsJSONWriter(); - int threadCount = 16; - int iterations = 20_000; - AtomicInteger corrupted = new AtomicInteger(); - CountDownLatch start = new CountDownLatch(1); - ExecutorService pool = Executors.newFixedThreadPool(threadCount); - - for (int t = 0; t < threadCount; t++) { - String secret = "SECRET_" + t + "_9f8e7d6c5b4a3210"; - NamedBean own = new NamedBean("user" + t, secret); - pool.submit(() -> { - await(start); - for (int i = 0; i < iterations; i++) { - try { - String response = sharedWriter.write(own); - if (!response.contains(secret)) { - // this thread's own response doesn't even contain its own data: - // the shared buffer was corrupted or overwritten by another thread - corrupted.incrementAndGet(); - } - } catch (JSONException ignored) { - // a failed serialize under contention is not itself the thing measured here - } - } - }); - } - - start.countDown(); - pool.shutdown(); - assertTrue(pool.awaitTermination(120, TimeUnit.SECONDS)); - - assertEquals("a concurrently-written response must never overwrite or corrupt this " + - "thread's own response", 0, corrupted.get()); - } - - private static void await(CountDownLatch latch) { - try { - latch.await(); - } catch (InterruptedException e) { - Thread.currentThread().interrupt(); - } - } - }
