[
https://issues.apache.org/jira/browse/WW-5687?focusedWorklogId=1041209&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1041209
]
ASF GitHub Bot logged work on WW-5687:
--------------------------------------
Author: ASF GitHub Bot
Created on: 13/Sep/26 15:06
Start Date: 13/Sep/26 15:06
Worklog Time Spent: 10m
Work Description: lukaszlenart opened a new pull request, #1930:
URL: https://github.com/apache/struts/pull/1930
Fixes [WW-5687](https://issues.apache.org/jira/browse/WW-5687)
Follow-on of WW-5537, defence-in-depth: `StrutsTypeConverterHolder`
(`defaultMappings`, `mappings`, `unknownMappings`) and
`DefaultActionValidatorManager` (`validatorCache`, `validatorFileCache`) hold
application `Class` objects and converter/validator instances. They are now
cleared during `Dispatcher.cleanup()` together with the eight existing
`InternalDestroyable` beans.
## Why adapters instead of a second `<bean>` line
The container keys singletons by `(type, name)`, so registering the holder
class a second time under `InternalDestroyable` builds a second, empty instance
— `destroyInternalBeans()` would clear nothing (see the ticket for the identity
probe). The existing dual-registered destroyables get away with it only because
what they clear is `static`.
So this adds:
- `default void clearCache() {}` on `TypeConverterHolder` and
`ActionValidatorManager` — no-op for third-party implementations, overridden in
`StrutsTypeConverterHolder` and `DefaultActionValidatorManager`
(`AnnotationActionValidatorManager`, the default binding, inherits it).
- `TypeConverterHolderDestroyable` / `ActionValidatorManagerDestroyable` —
`@Inject` the bare type, which the `StrutsBeanSelectionProvider` alias resolves
to the singleton actually in use (a custom `struts.actionValidatorManager` /
`struts.converter.holder` binding is covered), and call `clearCache()`.
## Tests
`DispatcherCleanupTest` gains two tests that prime the caches on
`container.getInstance(...)` — the same instance the framework injects — and
assert they are empty after `cleanup()`. Both fail under the naive dual
registration, which is the property the ticket asks for. Discovery test
extended with the two new bean names. Full `core` suite: 3322 tests, 0 failures.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Issue Time Tracking
-------------------
Worklog Id: (was: 1041209)
Remaining Estimate: 0h
Time Spent: 10m
> Clear the conversion and validator caches on Dispatcher.cleanup() (WW-5537
> defence-in-depth follow-on)
> ------------------------------------------------------------------------------------------------------
>
> Key: WW-5687
> URL: https://issues.apache.org/jira/browse/WW-5687
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Reporter: Lukasz Lenart
> Priority: Minor
> Fix For: 7.4.0
>
> Time Spent: 10m
> Remaining Estimate: 0h
>
> Follow-on from WW-5537 (Closed/Fixed, shipped 6.9.0 and 7.2.1), which built
> the {{InternalDestroyable}} cleanup framework. This item was identified
> during that work but never filed, and is still not on {{main}} as of
> 2026-08-23.
> h3. What it is, and what it is not
> {{StrutsTypeConverterHolder}} (fields {{mappings}}, {{defaultMappings}},
> {{unknownMappingsInternal}}) and the {{ActionValidatorManager}}
> implementations (fields {{validatorCache}}, {{validatorFileCache}}) hold
> strong references to application {{Class}} objects and to converter/validator
> instances.
> These are container-scoped singletons with no static, thread, or otherwise
> external references — they are reachable only through the container and are
> collected together with it. They therefore do _not_ independently pin the
> webapp classloader; that is determined by whatever retains the container,
> which WW-5537 already addressed.
> _This is not a leak fix._ It is defence-in-depth, consistent with the other
> destroyables: clearing these caches during {{Dispatcher.cleanup()}} bounds
> retained heap in the window before the container graph is collected, and
> costs nothing if it already has been.
> h3. The registration trap
> The obvious implementation — make the two classes implement
> {{InternalDestroyable}} and add a second {{<bean
> type="...InternalDestroyable" .../>}} line for each — _does not work_, and
> this is the reason the item is worth a ticket rather than a five-line patch.
> The Struts container keys factories by (type, name), so declaring the same
> class under two bean types produces two independent singletons. Verified
> empirically against {{main}}:
> {code}FileManager(system) = 1910936570 |
> InternalDestroyable(defaultFileManager) = 807322507 | SAME? false
> RootAccessor(struts) = 1495608502 |
> InternalDestroyable(compoundRootAccessor) = 835773569 | SAME? false
> {code}
> {{Dispatcher.destroyInternalBeans()}} discovers destroyables with
> {{container.getInstanceNames(InternalDestroyable.class)}} and then
> {{container.getInstance(InternalDestroyable.class, name)}} — so it would
> receive a freshly built holder whose caches are empty, and clear nothing.
> The existing dual-registered destroyables are unaffected by this, because
> what they clear is static: {{DefaultFileManager.clearCache()}} and
> {{CompoundRootAccessor.clearCache()}} are both {{static}} and clear
> {{static}} fields, so any instance will do. The caches in this ticket are
> per-instance, which is what breaks the pattern.
> h3. Suggested implementation
> Follow the adapter pattern already used for the other six destroyables
> ({{ComponentCacheDestroyable}}, {{OgnlCacheDestroyable}},
> {{ScopeInterceptorCacheDestroyable}}, {{FreemarkerCacheDestroyable}},
> {{DebugUtilsCacheDestroyable}}, {{FinalizableReferenceQueueDestroyable}}):
> add small {{InternalDestroyable}} beans that resolve the _real_ singleton
> from the container and clear it, rather than making the holders themselves
> the destroyable.
> Note that {{AnnotationActionValidatorManager extends
> DefaultActionValidatorManager}} and is the default binding
> ({{name="struts"}}); {{DefaultActionValidatorManager}} is bound separately as
> {{name="no-annotations"}}. Whatever is done must cover the instance actually
> in use, not just the base class.
> h3. Verification
> A test that asserts the caches are non-empty before {{Dispatcher.cleanup()}}
> and empty afterwards — and, importantly, that fails if the destroyable clears
> a different instance than the one the framework uses. A naive registration
> passes any test that only checks "destroy() was called".
> h3. Provenance
> The original plan text lived only in unpushed local commit {{8019e6994}}
> ({{docs/superpowers/plans/2026-03-23-WW-5537-classloader-leak-fixes.md}},
> Task 5b), on a branch deleted 2026-08-23. Its Step 3 prescribed the naive
> dual registration described above, which the identity probe shows would be a
> no-op — that correction is the main thing this ticket adds over the original
> note.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
