Lukasz Lenart created WW-5650:
---------------------------------
Summary: 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
Fix For: 7.3.0
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)
