[
https://issues.apache.org/jira/browse/WW-5650?focusedWorklogId=1030227&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1030227
]
ASF GitHub Bot logged work on WW-5650:
--------------------------------------
Author: ASF GitHub Bot
Created on: 14/Jul/26 08:45
Start Date: 14/Jul/26 08:45
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1782:
URL: https://github.com/apache/struts/pull/1782#discussion_r3577359179
##########
plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java:
##########
@@ -98,6 +99,7 @@ public String intercept(ActionInvocation invocation) throws
Exception {
String requestContentType = readContentType(request);
String requestContentTypeEncoding = readContentTypeEncoding(request);
+ JSONUtil jsonUtil = getJSONUtil();
Review Comment:
`getJSONUtil()` is called unconditionally, which creates a fresh
JSONUtil/reader/writer even for requests that don't have a JSON content type.
This adds avoidable per-request overhead and also makes the interceptor depend
on a configured Container even when it ends up ignoring the request due to
content type.
##########
plugins/json/src/test/java/org/apache/struts2/json/JSONInterceptorTest.java:
##########
@@ -905,6 +890,18 @@ public void
testExcludePropertiesNotAppliedToInputByDefault() throws Exception {
assertEquals("b", action.getBar());
}
+ public void testObtainsFreshJSONUtilAndReaderPerInvocation() {
+ JSONInterceptor interceptor = new JSONInterceptor();
+ interceptor.setContainer(container); // StrutsTestCase-provided
container
+
+ JSONUtil first = interceptor.getJSONUtil();
+ JSONUtil second = interceptor.getJSONUtil();
+
+ assertNotSame("interceptor must obtain a fresh JSONUtil per request",
first, second);
+ assertNotSame("each fresh JSONUtil must carry its own reader (no
shared parse state)",
+ first.getReader(), second.getReader());
+ }
Review Comment:
The freshness test only asserts distinct JSONUtil instances and distinct
readers. Since this PR also relies on obtaining a fresh writer per request
(because StrutsJSONWriter is now explicitly not thread-safe), the test should
also assert that each acquired JSONUtil carries a distinct writer instance.
##########
plugins/json/src/main/java/org/apache/struts2/json/JSONInterceptor.java:
##########
@@ -744,8 +746,12 @@ public void setJsonRpcContentType(String
jsonRpcContentType) {
}
@Inject
- public void setJsonUtil(JSONUtil jsonUtil) {
- this.jsonUtil = jsonUtil;
+ public void setContainer(Container container) {
+ this.container = container;
+ }
+
+ protected JSONUtil getJSONUtil() {
+ return container.getInstance(JSONUtil.class);
}
Review Comment:
`getJSONUtil()` will throw a NullPointerException if `setContainer()` was
not called (e.g., manual instantiation in tests/tools). Failing fast with a
clear exception makes misconfiguration easier to diagnose and avoids an NPE
stack trace pointing at `container.getInstance(...)`.
Issue Time Tracking
-------------------
Worklog Id: (was: 1030227)
Time Spent: 0.5h (was: 20m)
> Refactor JSON plugin reader/writer to per-request instances instead of shared
> mutable state
> -------------------------------------------------------------------------------------------
>
> Key: WW-5650
> URL: https://issues.apache.org/jira/browse/WW-5650
> Project: Struts 2
> Issue Type: Improvement
> Components: Plugin - JSON
> Reporter: Lukasz Lenart
> Assignee: Lukasz Lenart
> Priority: Major
> Fix For: 7.3.0
>
> Time Spent: 0.5h
> Remaining Estimate: 0h
>
> h2. Background
> The JSON plugin's request parser (StrutsJSONReader) and response serializer
> (StrutsJSONWriter) hold per-operation working state as instance fields:
> * StrutsJSONReader: character iterator, current char, current token, string
> buffer,
> and the nesting-depth counter used for the maxDepth/maxElements limits.
> * StrutsJSONWriter: output buffer, cyclic-reference stack, root object, and
> the
> expression-path state (buildExpr / exprStack / include-exclude patterns).
> Both are obtained through JSONUtil (getReader() / the injected writer), and
> JSONUtil
> is held once by the singleton JSONInterceptor. As a result a single reader
> and a single
> writer instance are reused across all concurrent requests handled by that
> interceptor.
> Because the working state lives on those shared instances, concurrent
> parse/serialize
> calls can interfere with each other, producing incorrect results and, for the
> reader,
> undermining the maxDepth/maxElements limits under load.
> h2. Interim fixes
> PR #1775 (WW-5643) and PR #1776 (WW-5644) address the immediate correctness
> problem by
> confining the per-operation state to a ThreadLocal, created fresh per call
> and cleared in
> a finally block. These are correct but rely on a per-request cleanup contract
> and leave
> some configuration fields shared. This ticket tracks the proper structural
> fix that
> supersedes that approach.
> h2. Proposed approach
> Stop sharing stateful reader/writer instances. The JSONReader/JSONWriter
> beans are already
> declared scope="prototype", so a fresh instance can be obtained per operation
> via the
> container. Refactor JSONUtil so that:
> * Each deserialize/serialize call obtains a fresh reader/writer, configures
> it (limits,
> ignoreHierarchy, dateFormat, include/exclude patterns, etc.), uses it, and
> discards it --
> all within a single scope. This removes the current configure-then-use split
> (applyLimitsToReader() + deserializeInput(); serialize() setters + write())
> that assumes
> a stable shared instance.
> * The standalone JSONUtil.getReader() accessor that returns a shared instance
> is deprecated
> / removed in favour of the per-operation flow.
> * The StrutsJSONReader/StrutsJSONWriter public extension points (the protected
> next()/object()/array()/string()/add()/bean()/map() methods used by
> subclasses) remain
> unchanged, so custom reader/writer subclasses and the struts.json.reader /
> struts.json.writer overrides keep working.
> * Once instances are no longer shared, the per-field ThreadLocal state
> introduced by the
> interim PRs is unwound (state returns to plain fields on the now-single-use
> instance).
> Performance note: the expensive bean-introspection cache in StrutsJSONWriter
> is a static
> ConcurrentMap shared across instances, so creating a fresh writer per request
> does not lose
> the reflection cache and is effectively free.
> h2. Scope
> * plugins/json -- JSONUtil, JSONInterceptor, StrutsJSONReader,
> StrutsJSONWriter
> * No change to public template/config surface (struts.json.reader /
> struts.json.writer,
> interceptor params).
> h2. Acceptance criteria
> * No reader/writer instance carries per-operation state across concurrent
> requests.
> * The concurrency regression tests added in #1775 and #1776 are preserved and
> pass against
> the refactored code.
> * Existing StrutsJSONReaderTest / JSONReaderTest / StrutsJSONWriterTest /
> JSONInterceptorTest
> suites pass unchanged.
> * Custom reader/writer subclasses and struts.json.reader/writer overrides
> continue to work.
> h2. Related
> * Supersedes the interim approach in #1775 (WW-5643) and #1776 (WW-5644).
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
