[
https://issues.apache.org/jira/browse/WW-5650?focusedWorklogId=1030221&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1030221
]
ASF GitHub Bot logged work on WW-5650:
--------------------------------------
Author: ASF GitHub Bot
Created on: 14/Jul/26 08:25
Start Date: 14/Jul/26 08:25
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1782:
URL: https://github.com/apache/struts/pull/1782
## What
Follow-up refactor of the JSON plugin's reader/writer thread-safety handling.
The interim fixes in #1775 / #1776 confined `StrutsJSONReader` /
`StrutsJSONWriter` per-operation
state to a `ThreadLocal`, created fresh per call and cleared in a `finally`.
That works, but it
relies on a per-request cleanup contract and keeps a single reader/writer
instance shared across
threads.
This change removes that machinery in favour of a simpler, structural
approach: the singleton
`JSONInterceptor` now obtains a **fresh `JSONUtil` (and thus a fresh reader
+ writer) per request**
via the container, so no parse/serialize state is ever shared across threads
in the first place.
With instances no longer shared, `StrutsJSONReader` / `StrutsJSONWriter`
revert to plain,
single-use instance fields and are documented as not thread-safe.
## Why this is safe / minimal
- `JSONResult` was already built per request (it injects a `prototype`
`JSONUtil`) and is
untouched — it never shared state.
- `JSONReader`, `JSONWriter` and `JSONUtil` are already registered
`scope="prototype"`, so
`container.getInstance(JSONUtil.class)` hands back a fresh graph each call
and continues to honour
the `struts.json.reader` / `struts.json.writer` overrides.
- This mirrors an existing in-tree pattern (`AbstractFileUploadInterceptor`
injects `Container` and
resolves collaborators per request).
## Changes
- `JSONInterceptor`: drop the cached `@Inject JSONUtil` field; inject
`Container` and resolve a
fresh `JSONUtil` per `intercept()` (new `protected getJSONUtil()` seam).
- `StrutsJSONReader` / `StrutsJSONWriter`: revert `ThreadLocal` state back
to plain instance fields;
add a "not thread-safe — obtain a fresh instance per operation" class note.
- Tests: the old shared-single-instance concurrency tests asserted an
invariant this design
intentionally drops (a single shared instance being safe), so they are
replaced by an
interceptor-level test asserting a distinct `JSONUtil`/reader is obtained
per acquisition.
## Testing
`mvn test -DskipAssembly -pl plugins/json` → all green (141 tests).
`JSONWriterOverrideTest` passes,
confirming `struts.json.reader` / `struts.json.writer` overrides still
resolve through the container.
## Notes
- Public/protected extension-method signatures of the reader/writer are
unchanged; custom
subclasses and reader/writer overrides continue to work.
- The interceptor freshness test asserts distinctness via `getJSONUtil()`
(JSONUtil exposes no
writer getter); this guards against re-caching/singleton-scoping the util.
Fixes [WW-5650](https://issues.apache.org/jira/browse/WW-5650)
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1030221)
Remaining Estimate: 0h
Time Spent: 10m
> 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: 10m
> 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)
