WW-5630 - Performance Issue SecurityMemberAccess#1721
Conversation
Update local branch
* Add size bound cache, 50, for Class lookup * Add unit test Code generated by Copilot
lukaszlenart
left a comment
lukaszlenart
left a comment
There was a problem hiding this comment.
Thanks for tackling WW-5630 — the motivation is solid. SecurityMemberAccess is Scope.PROTOTYPE (StrutsBeanSelectionProvider.java:454), so it's reconstructed on every value-stack creation (~per request), and each construction re-runs the @Inject use* setters → validateClasses → ClassLoader.loadClass(...). Memoizing that is worthwhile, and Caffeine is already a core dependency.
Two blocking issues before merge, both rooted in the cache key:
- Key on classloader identity, not
String.valueOf(classLoader). This can return aClass<?>from the wrong loader (collisions / constanttoString()), and thoseClassobjects feed OGNL exclusion/allowlist checks that rely on==— security-adjacent. - Static cache holding strong
Class<?>values pins classloaders — relevant given the WW-5537 leak work on adjacent branches.
Both are solved together with Caffeine.newBuilder().maximumSize(...).weakKeys().weakValues().build() keyed on the real ClassLoader (e.g. Cache<ClassLoader, Cache<String, Class<?>>>). weakKeys() gives identity equality (fixes #1) and GC-safety (fixes #2). Details inline.
* Cache ClassLoader directly * Use weakKeys and weakValues * Comment on the ClassLookupException * Additional Unit Tests Assistance in coding using co-pilot
|
@lukaszlenart I believe I've resolved all the concerns you raised and added additional UTs as well. Test set: org.apache.struts2.util.ConfigParseUtilTestTests run: 9, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.001 s -- in org.apache.struts2.util.ConfigParseUtilTest |
lukaszlenart
left a comment
lukaszlenart
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — this addresses all four points cleanly:
- ✅ Keyed on the actual
ClassLoadervia a nestedCache<ClassLoader, Cache<String, Class<?>>>withweakKeys()→ identity equality (no moretoString()collisions) and GC-safe. - ✅
weakValues()so cachedClassobjects no longer pin their loader. - ✅
ClassLookupKey/classLoaderNameremoved; cast invariant now documented onClassLookupException. - ✅ Added the same-
toString()collision test plus several others.
I ran mvn test -DskipAssembly -pl core -Dtest=ConfigParseUtilTest locally — 9/9 green. Approving.
One non-blocking nuance left on the outer cache (inline) — worth a look but not a blocker.
* Limit outer, Classloader, to 25. Ensure memory bounding. * Limit inner, Classes, to 50. Ensure memory bounding. * Additional UTs With co-pilot assitance
|
@brianandle thanks, all looks good 💪 |
* WW-5630 - Performance Issue SecurityMemberAccess * Add size bound cache, 50, for Class lookup * Add unit test Code generated by Copilot * WW-5630 - Add additional UT * WW-5630 - Add UT for non-existent class * WW-5630 - Review feedback changes * Cache ClassLoader directly * Use weakKeys and weakValues * Comment on the ClassLookupException * Additional Unit Tests Assistance in coding using co-pilot * WW-5630 - Additional review * Limit outer, Classloader, to 25. Ensure memory bounding. * Limit inner, Classes, to 50. Ensure memory bounding. * Additional UTs With co-pilot assitance (cherry picked from commit 210dc86)
2 participants
Closes WW-5630