[
https://issues.apache.org/jira/browse/WW-5650?focusedWorklogId=1030905&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1030905
]
ASF GitHub Bot logged work on WW-5650:
--------------------------------------
Author: ASF GitHub Bot
Created on: 17/Jul/26 10:53
Start Date: 17/Jul/26 10:53
Worklog Time Spent: 10m
Work Description: lukaszlenart commented on PR #1782:
URL: https://github.com/apache/struts/pull/1782#issuecomment-5002414349
Good coverage point. The gap reduces to one invariant: with
`StrutsJSONWriter` back on plain instance fields, WW-5644's response-side
safety now depends solely on the `JSONWriter` bean staying `scope="prototype"`.
I've guarded that directly rather than via a new accessor. `getWriter()` was
declined earlier (adding a production accessor purely for tests), and a
`JSONResult`-level test hits the same wall — `JSONResult` is
framework-instantiated with only setters. So instead
`JSONInterceptorTest#testObtainsFreshWriterPerAcquisition` asserts two
`container.getInstance(JSONWriter.class)` lookups return distinct instances.
That fails the build if the writer is ever switched to singleton or cached —
the exact regression you described — and mirrors how the reader is already
covered by `testObtainsFreshJSONUtilAndReaderPerInvocation`. No production API
change needed.
Issue Time Tracking
-------------------
Worklog Id: (was: 1030905)
Time Spent: 2h (was: 1h 50m)
> 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: 2h
> 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)
