From e000d703d0df279944ca0e83e0fa8eabea830666 Mon Sep 17 00:00:00 2001 From: Shinsuke Sugaya Date: Sun, 15 Mar 2026 12:23:07 +0900 Subject: [PATCH 1/2] refactor(script): add compiled script caching to GroovyEngine and improve DocBoostMatcher error handling GroovyEngine now caches compiled Groovy Script classes in an LRU map (max 100 entries), avoiding repeated compilation of the same script text. Each evaluation still creates a fresh Script instance for thread-safe binding isolation. GroovyClassLoaders are properly closed on eviction and via @PreDestroy. DocBoostMatcher now catches NumberFormatException when parsing boost values, logging a warning and returning 0.0f instead of propagating the exception. Includes comprehensive tests for cache behavior, binding isolation, eviction, concurrent evaluation, close lifecycle, and new DocBoostMatcher edge cases. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../fess/indexer/DocBoostMatcher.java | 10 +- .../fess/script/groovy/GroovyEngine.java | 135 ++++++--- .../fess/indexer/DocBoostMatcherTest.java | 118 ++++++++ .../fess/script/groovy/GroovyEngineTest.java | 258 ++++++++++++++++++ 4 files changed, 489 insertions(+), 32 deletions(-) diff --git a/src/main/java/org/codelibs/fess/indexer/DocBoostMatcher.java b/src/main/java/org/codelibs/fess/indexer/DocBoostMatcher.java index 2bd0b80e28..c54004f8ca 100644 --- a/src/main/java/org/codelibs/fess/indexer/DocBoostMatcher.java +++ b/src/main/java/org/codelibs/fess/indexer/DocBoostMatcher.java @@ -17,6 +17,8 @@ import java.util.Map; +import org.apache.logging.log4j.LogManager; +import org.apache.logging.log4j.Logger; import org.codelibs.fess.Constants; import org.codelibs.fess.opensearch.config.exentity.BoostDocumentRule; import org.codelibs.fess.util.ComponentUtil; @@ -29,6 +31,7 @@ * */ public class DocBoostMatcher { + private static final Logger logger = LogManager.getLogger(DocBoostMatcher.class); /** The expression used to calculate the boost value (defaults to "0") */ private String boostExpression = "0"; @@ -105,7 +108,12 @@ public float getValue(final Map map) { return ((Double) value).floatValue(); } if (value != null) { - return Float.parseFloat(value.toString()); + try { + return Float.parseFloat(value.toString()); + } catch (final NumberFormatException e) { + logger.warn("Failed to parse boost value: expression={}, value={}", boostExpression, value, e); + return 0.0f; + } } return 0.0f; diff --git a/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java b/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java index cfe3ba6c62..bad9f52ea2 100644 --- a/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java +++ b/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java @@ -15,8 +15,10 @@ */ package org.codelibs.fess.script.groovy; +import java.io.IOException; import java.util.Collections; import java.util.HashMap; +import java.util.LinkedHashMap; import java.util.Map; import org.apache.logging.log4j.LogManager; @@ -32,35 +34,60 @@ import groovy.lang.Binding; import groovy.lang.GroovyClassLoader; -import groovy.lang.GroovyShell; +import groovy.lang.Script; +import jakarta.annotation.PreDestroy; /** * Groovy script engine implementation that extends AbstractScriptEngine. * This class provides support for executing Groovy scripts with parameter binding * and DI container integration. * - *

Thread Safety: This class is thread-safe. Each evaluate() call creates - * a new GroovyShell instance to ensure thread isolation.

+ *

Thread Safety: This class is thread-safe. Each cached entry holds its own + * GroovyClassLoader. The cache is protected by Collections.synchronizedMap. + * Each evaluate() call creates a new Script instance to ensure thread isolation + * of bindings.

* - *

Resource Management: GroovyClassLoader instances are properly managed - * and cleaned up after script evaluation to prevent memory leaks.

+ *

Note on class-level isolation: Compiled Script classes are cached and reused. + * Class-level state (static fields, metaclass mutations) persists across evaluations + * of the same script. In Fess, scripts are short expressions configured by + * administrators (e.g., "data1 > 10", "10 * boost1 + boost2") and do not use + * static state, so this is acceptable.

+ * + *

Resource Management: Each cached entry's GroovyClassLoader is closed on + * eviction. All remaining entries are cleaned up via close() (@PreDestroy).

*/ public class GroovyEngine extends AbstractScriptEngine { private static final Logger logger = LogManager.getLogger(GroovyEngine.class); + private static final int DEFAULT_CACHE_SIZE = 100; + + private static final int MAX_SCRIPT_LOG_LENGTH = 200; + + private final Map scriptCache; + /** * Default constructor for GroovyEngine. */ public GroovyEngine() { super(); + scriptCache = Collections.synchronizedMap(new LinkedHashMap(DEFAULT_CACHE_SIZE + 1, 0.75f, true) { + @Override + protected boolean removeEldestEntry(final Map.Entry eldest) { + if (size() > DEFAULT_CACHE_SIZE) { + eldest.getValue().close(); + return true; + } + return false; + } + }); } /** * Evaluates a Groovy script template with the provided parameters. * - *

This method creates a new GroovyShell instance for each evaluation to ensure - * thread safety. The DI container is automatically injected into the binding map - * as "container" for script access.

+ *

This method caches compiled Script classes per script text. + * Each evaluation creates a new Script instance to ensure thread-safe binding isolation. + * The DI container is automatically injected into the binding map as "container".

* * @param template the Groovy script to evaluate (null-safe, returns null if empty) * @param paramMap the parameters to bind to the script (null-safe, treated as empty map if null) @@ -70,8 +97,6 @@ public GroovyEngine() { */ @Override public Object evaluate(final String template, final Map paramMap) { - // Null-safety: return null for blank templates - // Early return is safe here as no resources have been allocated yet if (StringUtil.isBlank(template)) { if (logger.isDebugEnabled()) { logger.debug("Template is blank, returning null"); @@ -79,57 +104,82 @@ public Object evaluate(final String template, final Map paramMap return null; } - // Null-safety: use empty map if paramMap is null final Map safeParamMap = paramMap != null ? paramMap : Collections.emptyMap(); - // Prepare binding map with parameters and DI container final Map bindingMap = new HashMap<>(safeParamMap); bindingMap.put("container", SingletonLaContainerFactory.getContainer()); - // Create GroovyShell with custom class loader for proper resource management - GroovyClassLoader classLoader = null; try { - // Get parent class loader with fallback to ensure robustness across threading contexts - ClassLoader parentClassLoader = Thread.currentThread().getContextClassLoader(); - if (parentClassLoader == null) { - parentClassLoader = GroovyEngine.class.getClassLoader(); - } - classLoader = new GroovyClassLoader(parentClassLoader); - final GroovyShell groovyShell = new GroovyShell(classLoader, new Binding(bindingMap)); + final CachedScript cached = getOrCompile(template); + final Script script = cached.scriptClass.getDeclaredConstructor().newInstance(); + script.setBinding(new Binding(bindingMap)); if (logger.isDebugEnabled()) { logger.debug("Evaluating Groovy script: template={}", template); } - final Object result = groovyShell.evaluate(template); + final Object result = script.run(); logScriptExecution(template, "success"); return result; } catch (final JobProcessingException e) { - // Rethrow JobProcessingException to allow scripts to signal job-specific errors - // that should be handled by the job framework if (logger.isDebugEnabled()) { logger.debug("Script raised JobProcessingException", e); } logScriptExecution(template, "failure:" + e.getClass().getSimpleName()); throw e; } catch (final Exception e) { - // Log and return null for other exceptions to maintain backward compatibility - logger.warn("Failed to evaluate Groovy script: template={}, parameters={}", template, safeParamMap, e); + final String truncatedScript = + template.length() > MAX_SCRIPT_LOG_LENGTH ? template.substring(0, MAX_SCRIPT_LOG_LENGTH) + "..." : template; + logger.warn("Failed to evaluate Groovy script: script(length={})={}, parameterKeys={}", template.length(), truncatedScript, + safeParamMap.keySet(), e); logScriptExecution(template, "failure:" + e.getClass().getSimpleName()); return null; - } finally { - // Properly clean up GroovyClassLoader resources - if (classLoader != null) { + } + } + + @SuppressWarnings("unchecked") + private CachedScript getOrCompile(final String template) { + synchronized (scriptCache) { + CachedScript cached = scriptCache.get(template); + if (cached != null) { + return cached; + } + ClassLoader parentClassLoader = Thread.currentThread().getContextClassLoader(); + if (parentClassLoader == null) { + parentClassLoader = GroovyEngine.class.getClassLoader(); + } + final GroovyClassLoader classLoader = new GroovyClassLoader(parentClassLoader); + try { + final Class scriptClass = (Class) classLoader.parseClass(template); + cached = new CachedScript(scriptClass, classLoader); + scriptCache.put(template, cached); + return cached; + } catch (final Exception e) { try { classLoader.clearCache(); classLoader.close(); - } catch (final Exception e) { - logger.warn("Failed to close GroovyClassLoader", e); + } catch (final IOException closeEx) { + logger.warn("Failed to close GroovyClassLoader after compilation failure", closeEx); } + throw e; } } } + /** + * Closes all cached GroovyClassLoaders and clears the script cache. + * Called by the DI container on shutdown. + */ + @PreDestroy + public void close() { + synchronized (scriptCache) { + for (final CachedScript cached : scriptCache.values()) { + cached.close(); + } + scriptCache.clear(); + } + } + /** * Returns the name identifier for this script engine. * @@ -195,4 +245,27 @@ protected void logScriptExecution(final String script, final String result) { } } + /** + * Holds a compiled Script class and its associated GroovyClassLoader. + * When evicted from the cache, close() releases the class loader resources. + */ + private static class CachedScript { + final Class scriptClass; + private final GroovyClassLoader classLoader; + + CachedScript(final Class scriptClass, final GroovyClassLoader classLoader) { + this.scriptClass = scriptClass; + this.classLoader = classLoader; + } + + void close() { + try { + classLoader.clearCache(); + classLoader.close(); + } catch (final IOException e) { + LogManager.getLogger(GroovyEngine.class).warn("Failed to close GroovyClassLoader", e); + } + } + } + } diff --git a/src/test/java/org/codelibs/fess/indexer/DocBoostMatcherTest.java b/src/test/java/org/codelibs/fess/indexer/DocBoostMatcherTest.java index 66cf111abd..23a6b44d6b 100644 --- a/src/test/java/org/codelibs/fess/indexer/DocBoostMatcherTest.java +++ b/src/test/java/org/codelibs/fess/indexer/DocBoostMatcherTest.java @@ -136,4 +136,122 @@ public void test_boost_params() { map.put("boost2", 2); assertTrue(12.0f == docBoostMatcher.getValue(map)); } + + @Test + public void test_getValue_floatReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("1.5f"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(1.5f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_doubleReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("2.5d"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(2.5f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_longReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("100L"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(100.0f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_numericStringReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("'3.14'"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertTrue(Math.abs(3.14f - docBoostMatcher.getValue(map)) < 0.001f); + } + + @Test + public void test_getValue_emptyStringReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("''"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(0.0f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_nullReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("null"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(0.0f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_match_nullMap() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setMatchExpression("true"); + assertFalse(docBoostMatcher.match(null)); + } + + @Test + public void test_match_emptyMap() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setMatchExpression("true"); + assertFalse(docBoostMatcher.match(new HashMap<>())); + } + + @Test + public void test_match_nullExpression() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + // matchExpression is null by default + final Map map = new HashMap(); + map.put("data1", 1); + assertFalse(docBoostMatcher.match(map)); + } + + @Test + public void test_match_nonBooleanReturn() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setMatchExpression("'string_value'"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertFalse(docBoostMatcher.match(map)); + } + + @Test + public void test_getValue_nonNumericString() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("'not_a_number'"); + docBoostMatcher.setMatchExpression("true"); + + final Map map = new HashMap(); + map.put("data1", 1); + assertEquals(0.0f, docBoostMatcher.getValue(map)); + } + + @Test + public void test_getValue_nullMap() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("10"); + assertEquals(0.0f, docBoostMatcher.getValue(null)); + } + + @Test + public void test_getValue_emptyMap() { + final DocBoostMatcher docBoostMatcher = new DocBoostMatcher(); + docBoostMatcher.setBoostExpression("10"); + assertEquals(0.0f, docBoostMatcher.getValue(new HashMap<>())); + } } diff --git a/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java b/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java index 5807c173aa..802e8a215b 100644 --- a/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java +++ b/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java @@ -15,9 +15,16 @@ */ package org.codelibs.fess.script.groovy; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.List; import java.util.Map; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; +import java.util.concurrent.Future; +import java.util.concurrent.atomic.AtomicInteger; import org.codelibs.fess.exception.JobProcessingException; import org.codelibs.fess.unit.UnitFessTestCase; @@ -37,6 +44,9 @@ protected void setUp(TestInfo testInfo) throws Exception { @Override protected void tearDown(TestInfo testInfo) throws Exception { + if (groovyEngine != null) { + groovyEngine.close(); + } ComponentUtil.setFessConfig(null); super.tearDown(testInfo); } @@ -408,6 +418,254 @@ public void test_evaluate_returnCollections() { assertTrue(mapResult instanceof java.util.Map); } + // ===== Cache Tests ===== + + /** + * Test that the same script evaluated multiple times produces correct results (cache hit) + */ + @Test + public void test_evaluate_cacheHit() { + final Map params = new HashMap<>(); + params.put("x", 5); + assertEquals(10, groovyEngine.evaluate("return x * 2", params)); + + params.put("x", 7); + assertEquals(14, groovyEngine.evaluate("return x * 2", params)); + + params.put("x", 0); + assertEquals(0, groovyEngine.evaluate("return x * 2", params)); + } + + /** + * Test that different scripts are cached separately + */ + @Test + public void test_evaluate_differentScriptsCached() { + final Map params = new HashMap<>(); + params.put("x", 5); + + assertEquals(10, groovyEngine.evaluate("return x * 2", params)); + assertEquals(25, groovyEngine.evaluate("return x * 5", params)); + assertEquals(10, groovyEngine.evaluate("return x * 2", params)); + } + + /** + * Test that cached scripts maintain binding isolation between evaluations + */ + @Test + public void test_evaluate_cacheBindingIsolation() { + final Map params1 = new HashMap<>(); + params1.put("name", "Alice"); + + final Map params2 = new HashMap<>(); + params2.put("name", "Bob"); + + assertEquals("Alice", groovyEngine.evaluate("return name", params1)); + assertEquals("Bob", groovyEngine.evaluate("return name", params2)); + assertEquals("Alice", groovyEngine.evaluate("return name", params1)); + } + + /** + * Test that close() can be called without error + */ + @Test + public void test_close() { + final GroovyEngine engine = new GroovyEngine(); + engine.evaluate("return 1", new HashMap<>()); + engine.close(); + } + + /** + * Test that cache eviction works when exceeding max size (100). + * Evaluates 110 distinct scripts, then verifies all still produce correct results. + */ + @Test + public void test_evaluate_cacheEviction() { + final GroovyEngine engine = new GroovyEngine(); + final Map params = new HashMap<>(); + + for (int i = 0; i < 110; i++) { + assertEquals(i, engine.evaluate("return " + i, params)); + } + + // Earlier scripts should still work (recompiled after eviction) + assertEquals(0, engine.evaluate("return 0", params)); + assertEquals(50, engine.evaluate("return 50", params)); + assertEquals(109, engine.evaluate("return 109", params)); + + engine.close(); + } + + /** + * Test that cached scripts do not leak binding state between evaluations. + * Verifies that local variables in one evaluation do not affect the next. + */ + @Test + public void test_evaluate_noStateLeakBetweenEvaluations() { + final GroovyEngine engine = new GroovyEngine(); + final String script = "def counter = 0; counter += x; return counter"; + + final Map params1 = new HashMap<>(); + params1.put("x", 5); + assertEquals(5, engine.evaluate(script, params1)); + + // Second evaluation of the same cached script should not see previous state + final Map params2 = new HashMap<>(); + params2.put("x", 3); + assertEquals(3, engine.evaluate(script, params2)); + + engine.close(); + } + + // ===== Compilation Failure Tests ===== + + /** + * Test that syntax errors are not cached and can be retried + */ + @Test + public void test_evaluate_syntaxErrorNotCached() { + final GroovyEngine engine = new GroovyEngine(); + final Map params = new HashMap<>(); + final String invalidScript = "this is not valid {{{"; + + // First call: syntax error returns null + assertNull(engine.evaluate(invalidScript, params)); + + // Second call: should also return null (not cached, recompiles and fails again) + assertNull(engine.evaluate(invalidScript, params)); + + // Valid script should still work after syntax errors + assertEquals(42, engine.evaluate("return 42", params)); + + engine.close(); + } + + /** + * Test that repeated syntax errors don't accumulate resources + */ + @Test + public void test_evaluate_repeatedSyntaxErrors() { + final GroovyEngine engine = new GroovyEngine(); + final Map params = new HashMap<>(); + + for (int i = 0; i < 10; i++) { + assertNull(engine.evaluate("invalid script {{{ " + i, params)); + } + + // Engine should still function normally + assertEquals("ok", engine.evaluate("return 'ok'", params)); + + engine.close(); + } + + // ===== Concurrent Evaluation Tests ===== + + /** + * Test concurrent evaluation of the same script from multiple threads + */ + @Test + public void test_evaluate_concurrentSameScript() throws Exception { + final GroovyEngine engine = new GroovyEngine(); + final int threadCount = 8; + final int iterationsPerThread = 50; + final ExecutorService executor = Executors.newFixedThreadPool(threadCount); + final CountDownLatch startLatch = new CountDownLatch(1); + final AtomicInteger errors = new AtomicInteger(0); + + final List> futures = new ArrayList<>(); + for (int t = 0; t < threadCount; t++) { + final int threadId = t; + futures.add(executor.submit(() -> { + try { + startLatch.await(); + for (int i = 0; i < iterationsPerThread; i++) { + final Map params = new HashMap<>(); + params.put("x", threadId * 1000 + i); + final Object result = engine.evaluate("return x * 2", params); + if (result == null || !result.equals((threadId * 1000 + i) * 2)) { + errors.incrementAndGet(); + } + } + } catch (final Exception e) { + errors.incrementAndGet(); + } + })); + } + + startLatch.countDown(); + for (final Future future : futures) { + future.get(); + } + + executor.shutdown(); + assertEquals(0, errors.get()); + engine.close(); + } + + /** + * Test concurrent evaluation of different scripts from multiple threads + */ + @Test + public void test_evaluate_concurrentDifferentScripts() throws Exception { + final GroovyEngine engine = new GroovyEngine(); + final int threadCount = 4; + final int scriptsPerThread = 20; + final ExecutorService executor = Executors.newFixedThreadPool(threadCount); + final CountDownLatch startLatch = new CountDownLatch(1); + final AtomicInteger errors = new AtomicInteger(0); + + final List> futures = new ArrayList<>(); + for (int t = 0; t < threadCount; t++) { + final int threadId = t; + futures.add(executor.submit(() -> { + try { + startLatch.await(); + for (int i = 0; i < scriptsPerThread; i++) { + final Map params = new HashMap<>(); + final int expected = threadId * 100 + i; + final Object result = engine.evaluate("return " + expected, params); + if (result == null || !result.equals(expected)) { + errors.incrementAndGet(); + } + } + } catch (final Exception e) { + errors.incrementAndGet(); + } + })); + } + + startLatch.countDown(); + for (final Future future : futures) { + future.get(); + } + + executor.shutdown(); + assertEquals(0, errors.get()); + engine.close(); + } + + // ===== Close Lifecycle Tests ===== + + /** + * Test that close() can be called multiple times without error + */ + @Test + public void test_close_idempotent() { + final GroovyEngine engine = new GroovyEngine(); + engine.evaluate("return 1", new HashMap<>()); + engine.close(); + engine.close(); // second close should not throw + } + + /** + * Test that close() on a fresh engine (no evaluations) works + */ + @Test + public void test_close_noEvaluations() { + final GroovyEngine engine = new GroovyEngine(); + engine.close(); + } + // ===== Edge Cases ===== /** From 7494480406ca394a90c25ed03b0ac9ebb1d3083f Mon Sep 17 00:00:00 2001 From: Shinsuke Sugaya Date: Sun, 15 Mar 2026 13:06:31 +0900 Subject: [PATCH 2/2] refactor(script): replace synchronized LinkedHashMap with Guava Cache in GroovyEngine Replace the manual synchronized LinkedHashMap-based LRU cache with Guava Cache for compiled Groovy scripts. This provides segment-based locking for lock-free concurrent reads and automatic eviction via RemovalListener. Also makes scriptCacheSize and maxScriptLogLength configurable via DI. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../fess/script/groovy/GroovyEngine.java | 126 +++++++++++------- src/main/resources/fess_se++.xml | 5 + .../fess/script/groovy/GroovyEngineTest.java | 18 +++ 3 files changed, 102 insertions(+), 47 deletions(-) diff --git a/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java b/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java index bad9f52ea2..9a52d2ee4d 100644 --- a/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java +++ b/src/main/java/org/codelibs/fess/script/groovy/GroovyEngine.java @@ -18,8 +18,8 @@ import java.io.IOException; import java.util.Collections; import java.util.HashMap; -import java.util.LinkedHashMap; import java.util.Map; +import java.util.concurrent.ExecutionException; import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; @@ -32,9 +32,14 @@ import org.lastaflute.di.core.factory.SingletonLaContainerFactory; import org.lastaflute.job.LaJobRuntime; +import com.google.common.cache.Cache; +import com.google.common.cache.CacheBuilder; +import com.google.common.cache.RemovalNotification; + import groovy.lang.Binding; import groovy.lang.GroovyClassLoader; import groovy.lang.Script; +import jakarta.annotation.PostConstruct; import jakarta.annotation.PreDestroy; /** @@ -43,9 +48,9 @@ * and DI container integration. * *

Thread Safety: This class is thread-safe. Each cached entry holds its own - * GroovyClassLoader. The cache is protected by Collections.synchronizedMap. - * Each evaluate() call creates a new Script instance to ensure thread isolation - * of bindings.

+ * GroovyClassLoader. The cache uses Guava Cache with segment-based locking for + * lock-free concurrent reads. Each evaluate() call creates a new Script instance + * to ensure thread isolation of bindings.

* *

Note on class-level isolation: Compiled Script classes are cached and reused. * Class-level state (static fields, metaclass mutations) persists across evaluations @@ -54,32 +59,65 @@ * static state, so this is acceptable.

* *

Resource Management: Each cached entry's GroovyClassLoader is closed on - * eviction. All remaining entries are cleaned up via close() (@PreDestroy).

+ * eviction via RemovalListener. All remaining entries are cleaned up via close() (@PreDestroy).

*/ public class GroovyEngine extends AbstractScriptEngine { private static final Logger logger = LogManager.getLogger(GroovyEngine.class); - private static final int DEFAULT_CACHE_SIZE = 100; + /** Maximum number of compiled scripts to cache. Configurable via DI. */ + protected int scriptCacheSize = 1000; - private static final int MAX_SCRIPT_LOG_LENGTH = 200; + /** Maximum length of script text included in warning log messages. Configurable via DI. */ + protected int maxScriptLogLength = 200; - private final Map scriptCache; + private Cache scriptCache; /** * Default constructor for GroovyEngine. */ public GroovyEngine() { super(); - scriptCache = Collections.synchronizedMap(new LinkedHashMap(DEFAULT_CACHE_SIZE + 1, 0.75f, true) { - @Override - protected boolean removeEldestEntry(final Map.Entry eldest) { - if (size() > DEFAULT_CACHE_SIZE) { - eldest.getValue().close(); - return true; - } - return false; - } - }); + buildScriptCache(); + } + + /** + * Rebuilds the script cache after DI injection. + * Called by the DI container after property injection. + */ + @PostConstruct + public void init() { + buildScriptCache(); + } + + private void buildScriptCache() { + final Cache oldCache = scriptCache; + scriptCache = CacheBuilder.newBuilder() + .maximumSize(scriptCacheSize) + .removalListener((final RemovalNotification notification) -> { + notification.getValue().close(); + }) + .build(); + if (oldCache != null) { + oldCache.invalidateAll(); + } + } + + /** + * Sets the maximum number of compiled scripts to cache. + * + * @param scriptCacheSize the cache size + */ + public void setScriptCacheSize(final int scriptCacheSize) { + this.scriptCacheSize = scriptCacheSize; + } + + /** + * Sets the maximum length of script text included in warning log messages. + * + * @param maxScriptLogLength the max length + */ + public void setMaxScriptLogLength(final int maxScriptLogLength) { + this.maxScriptLogLength = maxScriptLogLength; } /** @@ -129,7 +167,7 @@ public Object evaluate(final String template, final Map paramMap throw e; } catch (final Exception e) { final String truncatedScript = - template.length() > MAX_SCRIPT_LOG_LENGTH ? template.substring(0, MAX_SCRIPT_LOG_LENGTH) + "..." : template; + template.length() > maxScriptLogLength ? template.substring(0, maxScriptLogLength) + "..." : template; logger.warn("Failed to evaluate Groovy script: script(length={})={}, parameterKeys={}", template.length(), truncatedScript, safeParamMap.keySet(), e); logScriptExecution(template, "failure:" + e.getClass().getSimpleName()); @@ -139,30 +177,28 @@ public Object evaluate(final String template, final Map paramMap @SuppressWarnings("unchecked") private CachedScript getOrCompile(final String template) { - synchronized (scriptCache) { - CachedScript cached = scriptCache.get(template); - if (cached != null) { - return cached; - } - ClassLoader parentClassLoader = Thread.currentThread().getContextClassLoader(); - if (parentClassLoader == null) { - parentClassLoader = GroovyEngine.class.getClassLoader(); - } - final GroovyClassLoader classLoader = new GroovyClassLoader(parentClassLoader); - try { - final Class scriptClass = (Class) classLoader.parseClass(template); - cached = new CachedScript(scriptClass, classLoader); - scriptCache.put(template, cached); - return cached; - } catch (final Exception e) { + try { + return scriptCache.get(template, () -> { + ClassLoader parentClassLoader = Thread.currentThread().getContextClassLoader(); + if (parentClassLoader == null) { + parentClassLoader = GroovyEngine.class.getClassLoader(); + } + final GroovyClassLoader classLoader = new GroovyClassLoader(parentClassLoader); try { - classLoader.clearCache(); - classLoader.close(); - } catch (final IOException closeEx) { - logger.warn("Failed to close GroovyClassLoader after compilation failure", closeEx); + final Class scriptClass = (Class) classLoader.parseClass(template); + return new CachedScript(scriptClass, classLoader); + } catch (final Exception e) { + try { + classLoader.clearCache(); + classLoader.close(); + } catch (final IOException closeEx) { + logger.warn("Failed to close GroovyClassLoader after compilation failure", closeEx); + } + throw e; } - throw e; - } + }); + } catch (final ExecutionException e) { + throw (RuntimeException) e.getCause(); } } @@ -172,12 +208,8 @@ private CachedScript getOrCompile(final String template) { */ @PreDestroy public void close() { - synchronized (scriptCache) { - for (final CachedScript cached : scriptCache.values()) { - cached.close(); - } - scriptCache.clear(); - } + scriptCache.invalidateAll(); + scriptCache.cleanUp(); } /** diff --git a/src/main/resources/fess_se++.xml b/src/main/resources/fess_se++.xml index 6ffeed2ad3..4831a49ec7 100644 --- a/src/main/resources/fess_se++.xml +++ b/src/main/resources/fess_se++.xml @@ -4,6 +4,11 @@ + + diff --git a/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java b/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java index 802e8a215b..665542b022 100644 --- a/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java +++ b/src/test/java/org/codelibs/fess/script/groovy/GroovyEngineTest.java @@ -465,6 +465,24 @@ public void test_evaluate_cacheBindingIsolation() { assertEquals("Alice", groovyEngine.evaluate("return name", params1)); } + /** + * Test that configurable script cache size works with eviction + */ + @Test + public void test_configurableScriptCacheSize() { + final GroovyEngine engine = new GroovyEngine(); + engine.scriptCacheSize = 5; + engine.init(); + + final Map params = new HashMap<>(); + for (int i = 0; i < 10; i++) { + assertEquals(i, engine.evaluate("return " + i, params)); + } + // eviction後も再コンパイルで正しく動作 + assertEquals(0, engine.evaluate("return 0", params)); + engine.close(); + } + /** * Test that close() can be called without error */