[
https://issues.apache.org/jira/browse/WW-5650?focusedWorklogId=1030360&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1030360
]
ASF GitHub Bot logged work on WW-5650:
--------------------------------------
Author: ASF GitHub Bot
Created on: 14/Jul/26 15:01
Start Date: 14/Jul/26 15:01
Worklog Time Spent: 10m
Work Description: g0w6y commented on PR #1782:
URL: https://github.com/apache/struts/pull/1782#issuecomment-4970674067
The per-request prototype approach looks sound and preserves the fix from
#1775/#1776, so this is a non-blocking suggestion, not a gate.
One coverage point worth considering:
testObtainsFreshJSONUtilAndReaderPerInvocation asserts a fresh reader per
invocation, but not a fresh writer, and nothing asserts that JSONResult obtains
a fresh writer per request. Since StrutsJSONWriter now reverts to plain
instance fields and is explicitly documented not thread-safe (including the
lazily-initialized SimpleDateFormat in date()), the response-side protection
from WW-5644 now depends entirely on the prototype lifecycle. There is no live
bug today (JSONResult is instantiated per request), but if the bean were ever
changed to singleton, or a writer cached, the cross-request response leak would
return with no failing test to catch it.
Suggestion: (1) assert that two fresh JSONUtil instances carry distinct
writers (would need a getWriter() accessor, which JSONUtil does not currently
expose), and (2) a JSONResult-level test confirming a fresh writer per result.
Happy to send a patch for these if useful.
Issue Time Tracking
-------------------
Worklog Id: (was: 1030360)
Time Spent: 1.5h (was: 1h 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: 1.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)
