This is an automated email from the ASF dual-hosted git repository. lukaszlenart pushed a commit to branch WW-5716 in repository https://gitbox.apache.org/repos/asf/struts.git
commit cc69a45311f2060566188394cde5927df795b122 Author: Lukasz Lenart <[email protected]> AuthorDate: Fri Sep 4 10:28:32 2026 +0200 WW-5716 fix(tiles): bound the per-locale definition caches The Tiles definition caches are keyed by the resolved Locale, which by default derives from the request. Both CachingLocaleUrlDefinitionDAO's locale2definitionMap and AbstractPatternDefinitionResolver's localePatternPaths grew without limit and were never reduced for the lifetime of the web application. Bound locale2definitionMap with an insertion-order LinkedHashMap capped at maxCachedLocales (default 1000, configurable via setMaxCachedLocales). On eviction the DAO removes the same key from the pattern resolver via the new PatternDefinitionResolver#removePatternPaths, keeping both maps in lockstep (the resolver's keys are always a subset of the DAO's). localePatternPaths becomes a ConcurrentHashMap since the DAO now removes keys off the request thread. Eviction only re-incurs a load, never changes rendering. Fixes https://issues.apache.org/jira/browse/WW-5716 Co-Authored-By: Claude Opus 4.8 <[email protected]> Claude-Session: https://claude.ai/code/session_015huuB72yvZygWEXKUYDAou --- .../dao/CachingLocaleUrlDefinitionDAO.java | 38 ++++++++++++++++++++-- .../pattern/AbstractPatternDefinitionResolver.java | 10 ++++-- .../pattern/PatternDefinitionResolver.java | 8 +++++ .../dao/CachingLocaleUrlDefinitionDAOTest.java | 37 +++++++++++++++++++++ .../AbstractPatternDefinitionResolverTest.java | 29 +++++++++++++++++ 5 files changed, 118 insertions(+), 4 deletions(-) diff --git a/plugins/tiles/src/main/java/org/apache/tiles/core/definition/dao/CachingLocaleUrlDefinitionDAO.java b/plugins/tiles/src/main/java/org/apache/tiles/core/definition/dao/CachingLocaleUrlDefinitionDAO.java index b280fc97b..5caaef402 100644 --- a/plugins/tiles/src/main/java/org/apache/tiles/core/definition/dao/CachingLocaleUrlDefinitionDAO.java +++ b/plugins/tiles/src/main/java/org/apache/tiles/core/definition/dao/CachingLocaleUrlDefinitionDAO.java @@ -26,7 +26,6 @@ import org.apache.tiles.request.ApplicationContext; import org.apache.tiles.request.ApplicationResource; import org.apache.tiles.request.locale.LocaleUtil; -import java.util.HashMap; import java.util.LinkedHashMap; import java.util.Locale; import java.util.Map; @@ -53,6 +52,13 @@ public class CachingLocaleUrlDefinitionDAO extends BaseLocaleUrlDefinitionDAO im */ public static final String CHECK_REFRESH_INIT_PARAMETER = "org.apache.tiles.definition.dao.LocaleUrlDefinitionDAO.CHECK_REFRESH"; + /** + * Default upper bound on the number of customization keys (locales) whose definitions are cached at once. Since the + * customization key is derived from the request locale, this bounds the cache so it cannot grow without limit as + * distinct locales are encountered. + */ + public static final int DEFAULT_MAX_CACHED_LOCALES = 1000; + /** * The locale-specific set of definitions objects. * @@ -60,6 +66,12 @@ public class CachingLocaleUrlDefinitionDAO extends BaseLocaleUrlDefinitionDAO im */ protected Map<Locale, Map<String, Definition>> locale2definitionMap; + /** + * Maximum number of customization keys (locales) retained in {@link #locale2definitionMap}. When exceeded, the + * eldest entry is evicted (and reloaded on demand if requested again). + */ + protected int maxCachedLocales = DEFAULT_MAX_CACHED_LOCALES; + /** * Flag that, when <code>true</code>, enables automatic checking of URLs * changing. @@ -82,7 +94,29 @@ public class CachingLocaleUrlDefinitionDAO extends BaseLocaleUrlDefinitionDAO im */ public CachingLocaleUrlDefinitionDAO(ApplicationContext applicationContext) { super(applicationContext); - locale2definitionMap = new HashMap<>(); + locale2definitionMap = new LinkedHashMap<Locale, Map<String, Definition>>(16, 0.75f, false) { + @Override + protected boolean removeEldestEntry(Map.Entry<Locale, Map<String, Definition>> eldest) { + if (size() <= maxCachedLocales) { + return false; + } + if (definitionResolver != null) { + definitionResolver.removePatternPaths(eldest.getKey()); + } + return true; + } + }; + } + + /** + * Sets the maximum number of customization keys (locales) whose definitions are cached. When more distinct keys are + * requested, the eldest cached entry is evicted so the cache cannot grow without bound. Evicted entries are reloaded + * on demand if requested again, so eviction never changes rendering, only re-incurs a load. + * + * @param maxCachedLocales the maximum number of cached customization keys; values below 1 are treated as 1 + */ + public void setMaxCachedLocales(int maxCachedLocales) { + this.maxCachedLocales = Math.max(1, maxCachedLocales); } /** diff --git a/plugins/tiles/src/main/java/org/apache/tiles/core/definition/pattern/AbstractPatternDefinitionResolver.java b/plugins/tiles/src/main/java/org/apache/tiles/core/definition/pattern/AbstractPatternDefinitionResolver.java index a960ec9bf..0d51e1c77 100644 --- a/plugins/tiles/src/main/java/org/apache/tiles/core/definition/pattern/AbstractPatternDefinitionResolver.java +++ b/plugins/tiles/src/main/java/org/apache/tiles/core/definition/pattern/AbstractPatternDefinitionResolver.java @@ -21,9 +21,9 @@ package org.apache.tiles.core.definition.pattern; import org.apache.tiles.api.Definition; import java.util.ArrayList; -import java.util.HashMap; import java.util.List; import java.util.Map; +import java.util.concurrent.ConcurrentHashMap; /** * A pattern definition resolver that stores {@link DefinitionPatternMatcher} @@ -39,7 +39,7 @@ public abstract class AbstractPatternDefinitionResolver<T> implements PatternDef /** * Stores patterns depending on the locale they refer to. */ - private final Map<T, List<DefinitionPatternMatcher>> localePatternPaths = new HashMap<>(); + private final Map<T, List<DefinitionPatternMatcher>> localePatternPaths = new ConcurrentHashMap<>(); /** {@inheritDoc} */ public Definition resolveDefinition(String name, T customizationKey) { @@ -103,4 +103,10 @@ public abstract class AbstractPatternDefinitionResolver<T> implements PatternDef if (localePatternPaths.get(customizationKey) != null) localePatternPaths.get(customizationKey).clear(); } + + /** {@inheritDoc} */ + @Override + public void removePatternPaths(T customizationKey) { + localePatternPaths.remove(customizationKey); + } } diff --git a/plugins/tiles/src/main/java/org/apache/tiles/core/definition/pattern/PatternDefinitionResolver.java b/plugins/tiles/src/main/java/org/apache/tiles/core/definition/pattern/PatternDefinitionResolver.java index 959f082b9..82772bdeb 100644 --- a/plugins/tiles/src/main/java/org/apache/tiles/core/definition/pattern/PatternDefinitionResolver.java +++ b/plugins/tiles/src/main/java/org/apache/tiles/core/definition/pattern/PatternDefinitionResolver.java @@ -60,4 +60,12 @@ public interface PatternDefinitionResolver<T> { * @param customizationKey customization key */ void clearPatternPaths(T customizationKey); + + /** + * Removes the stored patterns for a specific customization key entirely, including the key itself. Used when the + * owning definitions cache evicts a customization key so that the pattern store cannot grow without bound. + * + * @param customizationKey customization key + */ + void removePatternPaths(T customizationKey); } diff --git a/plugins/tiles/src/test/java/org/apache/tiles/core/definition/dao/CachingLocaleUrlDefinitionDAOTest.java b/plugins/tiles/src/test/java/org/apache/tiles/core/definition/dao/CachingLocaleUrlDefinitionDAOTest.java index 048577321..51f5501ff 100644 --- a/plugins/tiles/src/test/java/org/apache/tiles/core/definition/dao/CachingLocaleUrlDefinitionDAOTest.java +++ b/plugins/tiles/src/test/java/org/apache/tiles/core/definition/dao/CachingLocaleUrlDefinitionDAOTest.java @@ -34,6 +34,7 @@ import org.apache.tiles.request.ApplicationResource; import org.apache.tiles.request.locale.URLApplicationResource; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -368,4 +369,40 @@ public class CachingLocaleUrlDefinitionDAOTest extends TestCase { assertEquals(1, attributes.size()); verify(applicationContext); } + + /** + * The definitions cache is keyed by locale, so it must not grow beyond the configured bound as distinct + * locales are requested; the eldest entry is evicted instead, and the eviction is propagated to the pattern + * resolver so its per-locale store cannot grow without bound either. + */ + public void testLocaleCacheIsBounded() { + List<ApplicationResource> sourceURLs = new ArrayList<>(); + sourceURLs.add(url1); + sourceURLs.add(url2); + sourceURLs.add(url3); + definitionDao.setSources(sourceURLs); + definitionDao.setReader(new DigesterDefinitionsReader()); + + List<Locale> evicted = new ArrayList<>(); + WildcardDefinitionPatternMatcherFactory factory = new WildcardDefinitionPatternMatcherFactory(); + PatternDefinitionResolver<Locale> recordingResolver = new BasicPatternDefinitionResolver<Locale>(factory, factory) { + @Override + public void removePatternPaths(Locale customizationKey) { + evicted.add(customizationKey); + super.removePatternPaths(customizationKey); + } + }; + definitionDao.setPatternDefinitionResolver(recordingResolver); + definitionDao.setMaxCachedLocales(2); + + for (Locale locale : new Locale[]{Locale.US, Locale.FRENCH, Locale.CANADA_FRENCH, Locale.CHINA}) { + assertNotNull("Definitions for " + locale + " were not loaded.", + definitionDao.getDefinitions(locale)); + } + + assertEquals("Definitions cache must not grow beyond the configured bound", + 2, definitionDao.locale2definitionMap.size()); + assertEquals("Evicted locales must be removed from the pattern resolver in lockstep", + Arrays.asList(Locale.US, Locale.FRENCH), evicted); + } } diff --git a/plugins/tiles/src/test/java/org/apache/tiles/core/definition/pattern/AbstractPatternDefinitionResolverTest.java b/plugins/tiles/src/test/java/org/apache/tiles/core/definition/pattern/AbstractPatternDefinitionResolverTest.java index db9c00f6e..dc46e0cf0 100644 --- a/plugins/tiles/src/test/java/org/apache/tiles/core/definition/pattern/AbstractPatternDefinitionResolverTest.java +++ b/plugins/tiles/src/test/java/org/apache/tiles/core/definition/pattern/AbstractPatternDefinitionResolverTest.java @@ -79,6 +79,35 @@ public class AbstractPatternDefinitionResolverTest { testResolveDefinitionImpl(); } + /** + * Test method for + * {@link AbstractPatternDefinitionResolver#removePatternPaths(Object)}: the key and its patterns are dropped + * entirely, so nothing resolves for that key afterwards. + */ + @Test + public void testRemovePatternPaths() { + firstMatcher = createMock(DefinitionPatternMatcher.class); + thirdMatcher = createMock(DefinitionPatternMatcher.class); + + Definition firstDefinition = new Definition("first", null, null); + Definition firstTransformedDefinition = new Definition("firstTransformed", null, null); + + expect(firstMatcher.createDefinition("firstTransformed")).andReturn(firstTransformedDefinition); + replay(firstMatcher, thirdMatcher); + + Map<String, Definition> localeDefsMap = new LinkedHashMap<>(); + localeDefsMap.put("first", firstDefinition); + resolver.storeDefinitionPatterns(localeDefsMap, 1); + + assertEquals(firstTransformedDefinition, resolver.resolveDefinition("firstTransformed", 1)); + + resolver.removePatternPaths(1); + assertNull("removePatternPaths must drop the entry so nothing resolves for the key", + resolver.resolveDefinition("firstTransformed", 1)); + + verify(firstMatcher, thirdMatcher); + } + private void testResolveDefinitionImpl() { firstMatcher = createMock(DefinitionPatternMatcher.class);
