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. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
