[
https://issues.apache.org/jira/browse/WW-5540?focusedWorklogId=1031834&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-1031834
]
ASF GitHub Bot logged work on WW-5540:
--------------------------------------
Author: ASF GitHub Bot
Created on: 23/Jul/26 12:02
Start Date: 23/Jul/26 12:02
Worklog Time Spent: 10m
Work Description: Copilot commented on code in PR #1808:
URL: https://github.com/apache/struts/pull/1808#discussion_r3637916733
##########
core/src/main/java/org/apache/struts2/text/StrutsLocalizedTextProvider.java:
##########
@@ -65,6 +65,11 @@ public String findText(Class<?> startClazz, String textKey,
Locale locale, Strin
LOG.debug("Key is null, short-circuit to default message");
return defaultMessage;
}
+
+ // Trigger bundle reload (and cache invalidation) once, before any
cached hierarchy lookup,
+ // so that in reload/devMode the hierarchy caches are cleared before
they are read.
+ reloadBundles(valueStack != null ? valueStack.getContext() : null);
Review Comment:
The hoisted reload call uses `valueStack != null ? valueStack.getContext() :
null`. When callers pass a null ValueStack (e.g., via
TextProviderSupport#getText(..., ValueStack)), this makes `reloadBundles` run
with a null context, so the per-request `RELOADED` flag can’t be set and
bundles/caches will be cleared on every lookup in reload mode. Use the
ActionContext context map as a fallback when the stack is null.
##########
core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java:
##########
@@ -90,6 +93,20 @@ protected ClassLoader getCurrentThreadContextClassLoader() {
return Thread.currentThread().getContextClassLoader();
}
+ private int currentLoaderHashCode() {
+ return getCurrentThreadContextClassLoader().hashCode();
+ }
Review Comment:
`currentLoaderHashCode()` currently uses `ClassLoader.hashCode()`, which can
be overridden and is more collision-prone than identity. Since this value is
used to partition caches across classloaders, prefer
`System.identityHashCode(...)` to avoid cross-classloader cache contamination
when a custom ClassLoader overrides `hashCode()`.
##########
core/src/main/java/org/apache/struts2/text/AbstractLocalizedTextProvider.java:
##########
@@ -582,23 +628,113 @@ protected String findMessage(Class<?> clazz, String key,
String indexedKey, Loca
// traverse up hierarchy
if (clazz.isInterface()) {
interfaces = clazz.getInterfaces();
-
for (Class<?> anInterface : interfaces) {
- msg = findMessage(anInterface, key, indexedKey, locale, args,
checked, valueStack);
-
+ msg = findMessageRaw(anInterface, key, indexedKey, locale,
checked);
if (msg != null) {
return msg;
}
}
} else {
if (!clazz.equals(Object.class) && !clazz.isPrimitive()) {
- return findMessage(clazz.getSuperclass(), key, indexedKey,
locale, args, checked, valueStack);
+ return findMessageRaw(clazz.getSuperclass(), key, indexedKey,
locale, checked);
}
}
return null;
}
+ /**
+ * Cached resolution of the class/interface/superclass hierarchy for a
key. Returns the raw pattern
+ * found, or {@link #NOT_FOUND} when the key is absent from the entire
hierarchy. Keyed on the
+ * context classloader hash + class name + key + locale, so no {@link
Class} reference is retained.
+ * Uses get + putIfAbsent (never computeIfAbsent) because the
child-property path recurses into findText.
+ */
+ protected String resolveClassHierarchyRaw(Class<?> clazz, String textKey,
String indexedKey, Locale locale) {
+ TextCacheKey cacheKey = new TextCacheKey(currentLoaderHashCode(),
clazz.getName(), textKey, locale);
+ String cached = classHierarchyCache.get(cacheKey);
Review Comment:
`resolveClassHierarchyRaw`’s cache key ignores the `indexedKey` parameter
even though the resolution logic consults both `textKey` and `indexedKey`. If
this method is called with the same `textKey` but different `indexedKey`
values, the cache can return an incorrect raw pattern. Include the indexed key
in the cache key (or remove the parameter and derive it internally) to make the
cache key match the lookup inputs.
This issue also appears on line 703 of the same file.
Issue Time Tracking
-------------------
Worklog Id: (was: 1031834)
Time Spent: 40m (was: 0.5h)
> Add caching to AbstractLocalizedTextProvider
> --------------------------------------------
>
> Key: WW-5540
> URL: https://issues.apache.org/jira/browse/WW-5540
> Project: Struts 2
> Issue Type: Improvement
> Components: Core
> Affects Versions: 6.7.4, 7.0.3
> Reporter: Kusal Kithul-Godage
> Assignee: Lukasz Lenart
> Priority: Minor
> Fix For: 7.3.0
>
> Time Spent: 40m
> Remaining Estimate: 0h
>
> Performance enhancement
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
