From 0076cccb9b281a6c6974b8c08936aabcbc51ec92 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sat, 12 Jul 2025 21:02:42 +0200 Subject: [PATCH 1/6] UNOMI-897: Fix data corruption and performance issues in GroovyActionDispatcher - Fixed data corruption issue by removing shared GroovyShell instance with separate script instances for separate variables - Improved performance by moving to pre-compilation of scripts to avoid on-the-fly and previously systematic compilation of Groovy scripts - Add ScriptMetadata class for script management --- .../actions/GroovyActionDispatcher.java | 69 ++-- .../services/GroovyActionsService.java | 95 ++++- .../impl/GroovyActionsServiceImpl.java | 355 +++++++++++++++--- 3 files changed, 420 insertions(+), 99 deletions(-) diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java index 87ed6108bc..224e877522 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java @@ -16,33 +16,36 @@ */ package org.apache.unomi.groovy.actions; -import groovy.lang.GroovyCodeSource; -import groovy.lang.GroovyShell; import groovy.lang.Script; import org.apache.unomi.api.Event; import org.apache.unomi.api.actions.Action; import org.apache.unomi.api.actions.ActionDispatcher; +import org.apache.unomi.api.services.DefinitionsService; import org.apache.unomi.groovy.actions.services.GroovyActionsService; import org.apache.unomi.metrics.MetricAdapter; import org.apache.unomi.metrics.MetricsService; +import org.apache.unomi.services.actions.ActionExecutorDispatcher; import org.osgi.service.component.annotations.Component; import org.osgi.service.component.annotations.Reference; import org.slf4j.Logger; import org.slf4j.LoggerFactory; /** - * An implementation of an ActionDispatcher for the Groovy language. This dispatcher will load the groovy action script matching to an - * actionName. If a script if found, it will be executed. + * High-performance ActionDispatcher for pre-compiled Groovy scripts. + * Executes scripts without GroovyShell overhead using isolated instances. */ @Component(service = ActionDispatcher.class) public class GroovyActionDispatcher implements ActionDispatcher { private static final Logger LOGGER = LoggerFactory.getLogger(GroovyActionDispatcher.class.getName()); + private static final Logger GROOVY_ACTION_LOGGER = LoggerFactory.getLogger("GroovyAction"); private static final String GROOVY_PREFIX = "groovy"; private MetricsService metricsService; private GroovyActionsService groovyActionsService; + private DefinitionsService definitionsService; + private ActionExecutorDispatcher actionExecutorDispatcher; @Reference public void setMetricsService(MetricsService metricsService) { @@ -54,30 +57,52 @@ public void setGroovyActionsService(GroovyActionsService groovyActionsService) { this.groovyActionsService = groovyActionsService; } + @Reference + public void setDefinitionsService(DefinitionsService definitionsService) { + this.definitionsService = definitionsService; + } + + @Reference + public void setActionExecutorDispatcher(ActionExecutorDispatcher actionExecutorDispatcher) { + this.actionExecutorDispatcher = actionExecutorDispatcher; + } + public String getPrefix() { return GROOVY_PREFIX; } public Integer execute(Action action, Event event, String actionName) { - GroovyCodeSource groovyCodeSource = groovyActionsService.getGroovyCodeSource(actionName); - if (groovyCodeSource == null) { - LOGGER.warn("Couldn't find a Groovy action with name {}, action will not execute !", actionName); - } else { - GroovyShell groovyShell = groovyActionsService.getGroovyShell(); - groovyShell.setVariable("action", action); - groovyShell.setVariable("event", event); - Script script = groovyShell.parse(groovyCodeSource); - try { - return new MetricAdapter(metricsService, this.getClass().getName() + ".action.groovy." + actionName) { - @Override - public Integer execute(Object... args) throws Exception { - return (Integer) script.invokeMethod("execute", null); - } - }.runWithTimer(); - } catch (Exception e) { - LOGGER.error("Error executing Groovy action with key={}", actionName, e); - } + Class scriptClass = groovyActionsService.getCompiledScript(actionName); + if (scriptClass == null) { + LOGGER.warn("Couldn't find a Groovy action with name {}, action will not execute!", actionName); + return 0; + } + + try { + Script script = scriptClass.newInstance(); + setScriptVariables(script, action, event); + + return new MetricAdapter(metricsService, this.getClass().getName() + ".action.groovy." + actionName) { + @Override + public Integer execute(Object... args) throws Exception { + return (Integer) script.invokeMethod("execute", null); + } + }.runWithTimer(); + + } catch (Exception e) { + LOGGER.error("Error executing Groovy action with key={}", actionName, e); } return 0; } + + /** + * Sets required variables on script instance. + */ + private void setScriptVariables(Script script, Action action, Event event) { + script.setProperty("action", action); + script.setProperty("event", event); + script.setProperty("actionExecutorDispatcher", actionExecutorDispatcher); + script.setProperty("definitionsService", definitionsService); + script.setProperty("logger", GROOVY_ACTION_LOGGER); + } } diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java index 4b6d54505f..aa6bf8b1bc 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java @@ -16,43 +16,106 @@ */ package org.apache.unomi.groovy.actions.services; -import groovy.lang.GroovyCodeSource; -import groovy.lang.GroovyShell; -import groovy.util.GroovyScriptEngine; +import groovy.lang.Script; import org.apache.unomi.groovy.actions.GroovyAction; +import org.apache.unomi.groovy.actions.ScriptMetadata; + /** - * A service to load groovy files and manage {@link GroovyAction} + * Service interface for managing Groovy action scripts. + *

+ * This service provides functionality to load, compile, cache, and execute + * Groovy scripts as actions within the Apache Unomi framework. It implements + * optimized compilation and caching strategies to achieve high performance. + *

+ * + *

+ * Key features: + *

    + *
  • Pre-compilation of scripts at startup
    • + *
  • Hash-based change detection for selective recompilation
    • + *
  • Thread-safe compilation and execution
    • + *
  • Unified caching architecture for compiled scripts
    • + *
+ *

+ * + *

+ * Thread Safety: Implementations must be thread-safe as this service + * is accessed concurrently during script execution. + *

+ * + * @see GroovyAction + * @see ScriptMetadata + * @since 2.7.0 */ public interface GroovyActionsService { /** - * Save a groovy action from a groovy file + * Saves a Groovy action script with compilation and validation. + *

+ * This method compiles the script, validates it has the required + * annotations, persists it, and updates the internal cache. + * If the script content hasn't changed, recompilation is skipped. + *

* - * @param actionName actionName - * @param groovyScript script to save + * @param actionName the unique identifier for the action + * @param groovyScript the Groovy script source code + * @throws IllegalArgumentException if actionName or groovyScript is null + * @throws RuntimeException if compilation or persistence fails */ void save(String actionName, String groovyScript); /** - * Remove a groovy action + * Removes a Groovy action and all associated metadata. + *

+ * This method removes the action from both the cache and persistent storage, + * and cleans up any registered action types in the definitions service. + *

* - * @param id of the action to remove + * @param id the unique identifier of the action to remove + * @throws IllegalArgumentException if id is null */ void remove(String id); /** - * Get a groovy code source object by an id + * Retrieves a compiled script class, compiling on-demand if not cached. + *

+ * This method first checks the cache for a compiled version. If not found, + * it loads the script from persistence and compiles it. This method is + * provided for backward compatibility but may have performance implications. + *

+ * + * @param id the unique identifier of the action + * @return the compiled script class, or {@code null} if not found + * @throws IllegalArgumentException if id is null + */ + Class getOrCompileScript(String id); + + /** + * Retrieves a pre-compiled script class from cache. + *

+ * This is the preferred method for script execution as it returns + * pre-compiled classes without any compilation overhead. Returns + * {@code null} if the script is not found in the cache. + *

* - * @param id of the action to get - * @return Groovy code source + * @param id the unique identifier of the action + * @return the compiled script class, or {@code null} if not found in cache + * @throws IllegalArgumentException if id is null */ - GroovyCodeSource getGroovyCodeSource(String id); + Class getCompiledScript(String id); /** - * Get an instantiated groovy shell object + * Retrieves script metadata for monitoring and change detection. + *

+ * The returned metadata includes content hash, compilation timestamp, + * and the compiled class reference. This is useful for monitoring + * tools and debugging. + *

* - * @return GroovyShell + * @param actionName the unique identifier of the action + * @return the script metadata, or {@code null} if not found + * @throws IllegalArgumentException if actionName is null */ - GroovyShell getGroovyShell(); + ScriptMetadata getScriptMetadata(String actionName); } diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java index bee011cd6d..25a42ffbb5 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java @@ -19,6 +19,7 @@ import groovy.lang.GroovyClassLoader; import groovy.lang.GroovyCodeSource; import groovy.lang.GroovyShell; +import groovy.lang.Script; import groovy.util.GroovyScriptEngine; import org.apache.commons.io.IOUtils; import org.apache.unomi.api.Metadata; @@ -27,10 +28,10 @@ import org.apache.unomi.api.services.SchedulerService; import org.apache.unomi.groovy.actions.GroovyAction; import org.apache.unomi.groovy.actions.GroovyBundleResourceConnector; +import org.apache.unomi.groovy.actions.ScriptMetadata; import org.apache.unomi.groovy.actions.annotations.Action; import org.apache.unomi.groovy.actions.services.GroovyActionsService; import org.apache.unomi.persistence.spi.PersistenceService; -import org.apache.unomi.services.actions.ActionExecutorDispatcher; import org.codehaus.groovy.control.CompilerConfiguration; import org.codehaus.groovy.control.customizers.ImportCustomizer; import org.osgi.framework.BundleContext; @@ -43,10 +44,12 @@ import java.io.IOException; import java.net.URL; -import java.util.HashMap; +import java.nio.charset.StandardCharsets; import java.util.HashSet; + import java.util.Map; import java.util.TimerTask; +import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; @@ -55,7 +58,8 @@ import static java.util.Arrays.asList; /** - * Implementation of the GroovyActionService. Allows to create a groovy action from a groovy file + * High-performance GroovyActionsService implementation with pre-compilation, + * hash-based change detection, and thread-safe execution. */ @Component(service = GroovyActionsService.class, configurationPid = "org.apache.unomi.groovy.actions") @Designate(ocd = GroovyActionsServiceImpl.GroovyActionsServiceConfig.class) @@ -68,9 +72,12 @@ public class GroovyActionsServiceImpl implements GroovyActionsService { private BundleContext bundleContext; private GroovyScriptEngine groovyScriptEngine; - private GroovyShell groovyShell; - private Map groovyCodeSourceMap; + private CompilerConfiguration compilerConfiguration; private ScheduledFuture scheduledFuture; + + private final Object compilationLock = new Object(); + private GroovyShell compilationShell; + private volatile Map scriptMetadataCache = new ConcurrentHashMap<>(); private static final Logger LOGGER = LoggerFactory.getLogger(GroovyActionsServiceImpl.class.getName()); private static final String BASE_SCRIPT_NAME = "BaseScript"; @@ -78,7 +85,6 @@ public class GroovyActionsServiceImpl implements GroovyActionsService { private DefinitionsService definitionsService; private PersistenceService persistenceService; private SchedulerService schedulerService; - private ActionExecutorDispatcher actionExecutorDispatcher; private GroovyActionsServiceConfig config; @Reference @@ -96,14 +102,7 @@ public void setSchedulerService(SchedulerService schedulerService) { this.schedulerService = schedulerService; } - @Reference - public void setActionExecutorDispatcher(ActionExecutorDispatcher actionExecutorDispatcher) { - this.actionExecutorDispatcher = actionExecutorDispatcher; - } - public GroovyShell getGroovyShell() { - return groovyShell; - } @Activate public void start(GroovyActionsServiceConfig config, BundleContext bundleContext) { @@ -111,20 +110,25 @@ public void start(GroovyActionsServiceConfig config, BundleContext bundleContext this.config = config; this.bundleContext = bundleContext; - this.groovyCodeSourceMap = new HashMap<>(); GroovyBundleResourceConnector bundleResourceConnector = new GroovyBundleResourceConnector(bundleContext); GroovyClassLoader groovyLoader = new GroovyClassLoader(bundleContext.getBundle().adapt(BundleWiring.class).getClassLoader()); this.groovyScriptEngine = new GroovyScriptEngine(bundleResourceConnector, groovyLoader); - initializeGroovyShell(); + // Initialize Groovy compiler and compilation shell + initializeGroovyCompiler(); + try { loadBaseScript(); } catch (IOException e) { LOGGER.error("Failed to load base script", e); } + + // PRE-COMPILE ALL SCRIPTS AT STARTUP (no on-demand compilation) + preloadAllScripts(); + initializeTimers(); - LOGGER.info("Groovy action service initialized."); + LOGGER.info("Groovy action service initialized with {} scripts", scriptMetadataCache.size()); } @Deactivate @@ -136,15 +140,7 @@ public void onDestroy() { } /** - * Load the Base script. - * It's a script which provides utility functions that we can use in other groovy script - * The functions added by the base script could be called by the groovy actions executed in - * {@link org.apache.unomi.groovy.actions.GroovyActionDispatcher#execute} - * The base script would be added in the configuration of the {@link GroovyActionsServiceImpl#groovyShell GroovyShell} , so when a - * script will be parsed with the GroovyShell (groovyShell.parse(...)), the action will extends the base script, so the functions - * could be called - * - * @throws IOException + * Loads the base script that provides utility functions for Groovy actions. */ private void loadBaseScript() throws IOException { URL groovyBaseScriptURL = bundleContext.getBundle().getEntry("META-INF/base/BaseScript.groovy"); @@ -152,25 +148,78 @@ private void loadBaseScript() throws IOException { return; } LOGGER.debug("Found Groovy base script at {}, loading... ", groovyBaseScriptURL.getPath()); - GroovyCodeSource groovyCodeSource = new GroovyCodeSource(IOUtils.toString(groovyBaseScriptURL.openStream()), BASE_SCRIPT_NAME, "/groovy/script"); + GroovyCodeSource groovyCodeSource = new GroovyCodeSource(IOUtils.toString(groovyBaseScriptURL.openStream(), StandardCharsets.UTF_8), BASE_SCRIPT_NAME, "/groovy/script"); groovyScriptEngine.getGroovyClassLoader().parseClass(groovyCodeSource, true); } /** - * Initialize the groovyShell object and define the configuration which contains the name of the base script + * Initializes compiler configuration and shared compilation shell. */ - private void initializeGroovyShell() { - CompilerConfiguration compilerConfiguration = new CompilerConfiguration(); + private void initializeGroovyCompiler() { + // Configure the compiler with imports and base script + compilerConfiguration = new CompilerConfiguration(); compilerConfiguration.addCompilationCustomizers(createImportCustomizer()); - compilerConfiguration.setScriptBaseClass(BASE_SCRIPT_NAME); groovyScriptEngine.setConfig(compilerConfiguration); - groovyShell = new GroovyShell(groovyScriptEngine.getGroovyClassLoader(), compilerConfiguration); - groovyShell.setVariable("actionExecutorDispatcher", actionExecutorDispatcher); - groovyShell.setVariable("definitionsService", definitionsService); - groovyShell.setVariable("logger", LoggerFactory.getLogger("GroovyAction")); + + // Create single shared shell for compilation only + this.compilationShell = new GroovyShell(groovyScriptEngine.getGroovyClassLoader(), compilerConfiguration); + } + + /** + * Pre-compiles all scripts at startup to eliminate runtime compilation overhead. + */ + private void preloadAllScripts() { + long startTime = System.currentTimeMillis(); + LOGGER.info("Pre-compiling all Groovy scripts at startup..."); + + int successCount = 0; + int failureCount = 0; + long totalCompilationTime = 0; + + for (GroovyAction groovyAction : persistenceService.getAllItems(GroovyAction.class)) { + try { + String actionName = groovyAction.getName(); + String scriptContent = groovyAction.getScript(); + + long scriptStartTime = System.currentTimeMillis(); + Class scriptClass = compileScript(actionName, scriptContent); + long scriptCompilationTime = System.currentTimeMillis() - scriptStartTime; + totalCompilationTime += scriptCompilationTime; + + ScriptMetadata metadata = new ScriptMetadata(actionName, scriptContent, scriptClass); + scriptMetadataCache.put(actionName, metadata); + + successCount++; + LOGGER.debug("Pre-compiled script: {} ({}ms)", actionName, scriptCompilationTime); + + } catch (Exception e) { + failureCount++; + LOGGER.error("Failed to pre-compile script: {}", groovyAction.getName(), e); + } + } + + long totalTime = System.currentTimeMillis() - startTime; + LOGGER.info("Pre-compilation completed: {} scripts successfully compiled, {} failures. Total time: {}ms", + successCount, failureCount, totalTime); + LOGGER.debug("Pre-compilation metrics: Average per script: {}ms, Compilation overhead: {}ms", + successCount > 0 ? totalCompilationTime / successCount : 0, + totalTime - totalCompilationTime); + } + + /** + * Thread-safe script compilation using synchronized shared shell. + */ + private Class compileScript(String actionName, String scriptContent) { + GroovyCodeSource codeSource = buildClassScript(scriptContent, actionName); + synchronized(compilationLock) { + return compilationShell.parse(codeSource).getClass(); + } } + /** + * Creates import customizer with standard Unomi imports. + */ private ImportCustomizer createImportCustomizer() { ImportCustomizer importCustomizer = new ImportCustomizer(); importCustomizer.addImports("org.apache.unomi.api.services.EventService", "org.apache.unomi.groovy.actions.annotations.Action", @@ -178,25 +227,60 @@ private ImportCustomizer createImportCustomizer() { return importCustomizer; } + /** + * {@inheritDoc} + * Implementation performs hash-based change detection to skip unnecessary recompilation. + */ @Override public void save(String actionName, String groovyScript) { - GroovyCodeSource groovyCodeSource = buildClassScript(groovyScript, actionName); + if (actionName == null || actionName.trim().isEmpty()) { + throw new IllegalArgumentException("Action name cannot be null or empty"); + } + if (groovyScript == null || groovyScript.trim().isEmpty()) { + throw new IllegalArgumentException("Groovy script cannot be null or empty"); + } + + long startTime = System.currentTimeMillis(); + LOGGER.info("Saving script: {}", actionName); + try { - saveActionType(groovyShell.parse(groovyCodeSource).getClass().getMethod("execute").getAnnotation(Action.class)); + ScriptMetadata existingMetadata = scriptMetadataCache.get(actionName); + if (existingMetadata != null && !existingMetadata.hasChanged(groovyScript)) { + LOGGER.info("Script {} unchanged, skipping recompilation ({}ms)", actionName, + System.currentTimeMillis() - startTime); + return; + } + + long compilationStartTime = System.currentTimeMillis(); + Class scriptClass = compileScript(actionName, groovyScript); + long compilationTime = System.currentTimeMillis() - compilationStartTime; + + Action actionAnnotation = scriptClass.getMethod("execute").getAnnotation(Action.class); + if (actionAnnotation != null) { + saveActionType(actionAnnotation); + } + saveScript(actionName, groovyScript); - LOGGER.info("The script {} has been loaded.", actionName); - } catch (NoSuchMethodException e) { - LOGGER.error("Failed to save the script {}", actionName, e); + + ScriptMetadata metadata = new ScriptMetadata(actionName, groovyScript, scriptClass); + scriptMetadataCache.put(actionName, metadata); + + long totalTime = System.currentTimeMillis() - startTime; + LOGGER.info("Script {} saved and compiled successfully (total: {}ms, compilation: {}ms)", + actionName, totalTime, compilationTime); + + } catch (Exception e) { + long totalTime = System.currentTimeMillis() - startTime; + LOGGER.error("Failed to save script: {} ({}ms)", actionName, totalTime, e); + throw new RuntimeException("Failed to save script: " + actionName, e); } } /** - * Build an action type from the annotation {@link Action} - * - * @param action Annotation containing the values to save + * Builds and registers ActionType from Action annotation. */ private void saveActionType(Action action) { - Metadata metadata = new Metadata(null, action.id(), action.name().equals("") ? action.id() : action.name(), action.description()); + Metadata metadata = new Metadata(null, action.id(), action.name().isEmpty() ? action.id() : action.name(), action.description()); metadata.setHidden(action.hidden()); metadata.setReadOnly(true); metadata.setSystemTags(new HashSet<>(asList(action.systemTags()))); @@ -209,48 +293,197 @@ private void saveActionType(Action action) { definitionsService.setActionType(actionType); } + /** + * {@inheritDoc} + */ @Override public void remove(String id) { - if (groovyCodeSourceMap.containsKey(id)) { - try { - definitionsService.removeActionType( - groovyShell.parse(groovyCodeSourceMap.get(id)).getClass().getMethod("execute").getAnnotation(Action.class).id()); - } catch (NoSuchMethodException e) { - LOGGER.error("Failed to delete the action type for the id {}", id, e); + if (id == null || id.trim().isEmpty()) { + throw new IllegalArgumentException("Script ID cannot be null or empty"); + } + + LOGGER.info("Removing script: {}", id); + + ScriptMetadata removedMetadata = scriptMetadataCache.remove(id); + persistenceService.remove(id, GroovyAction.class); + + try { + if (removedMetadata != null) { + Class cachedClass = removedMetadata.getCompiledClass(); + Action actionAnnotation = cachedClass.getMethod("execute").getAnnotation(Action.class); + if (actionAnnotation != null) { + definitionsService.removeActionType(actionAnnotation.id()); + } } - persistenceService.remove(id, GroovyAction.class); + } catch (Exception e) { + LOGGER.error("Failed to remove action type for script: {}", id, e); } + + LOGGER.info("Script {} removed successfully", id); } + + /** + * {@inheritDoc} + * Performance Warning: Compiles on-demand if not cached. + */ + @Override + public Class getOrCompileScript(String id) { + if (id == null || id.trim().isEmpty()) { + throw new IllegalArgumentException("Script ID cannot be null or empty"); + } + + ScriptMetadata metadata = scriptMetadataCache.get(id); + if (metadata != null) { + return metadata.getCompiledClass(); + } + + long startTime = System.currentTimeMillis(); + LOGGER.warn("Script {} not found in cache, compiling on-demand (performance warning)", id); + + GroovyAction groovyAction = persistenceService.load(id, GroovyAction.class); + if (groovyAction == null) { + LOGGER.warn("Script {} not found in persistence, returning null ({}ms)", id, + System.currentTimeMillis() - startTime); + return null; + } + + try { + long compilationStartTime = System.currentTimeMillis(); + Class scriptClass = compileScript(id, groovyAction.getScript()); + long compilationTime = System.currentTimeMillis() - compilationStartTime; + + ScriptMetadata newMetadata = new ScriptMetadata(id, groovyAction.getScript(), scriptClass); + scriptMetadataCache.put(id, newMetadata); + + long totalTime = System.currentTimeMillis() - startTime; + LOGGER.warn("On-demand compilation completed for {} (total: {}ms, compilation: {}ms)", + id, totalTime, compilationTime); + return scriptClass; + } catch (Exception e) { + long totalTime = System.currentTimeMillis() - startTime; + LOGGER.error("Failed to compile script {} on-demand ({}ms)", id, totalTime, e); + return null; + } + } + + /** + * {@inheritDoc} + */ + @Override + public Class getCompiledScript(String id) { + if (id == null || id.trim().isEmpty()) { + throw new IllegalArgumentException("Script ID cannot be null or empty"); + } + + ScriptMetadata metadata = scriptMetadataCache.get(id); + if (metadata == null) { + LOGGER.warn("Script {} not found in cache", id); + return null; + } + return metadata.getCompiledClass(); + } + + /** + * {@inheritDoc} + */ @Override - public GroovyCodeSource getGroovyCodeSource(String id) { - return groovyCodeSourceMap.get(id); + public ScriptMetadata getScriptMetadata(String actionName) { + if (actionName == null || actionName.trim().isEmpty()) { + throw new IllegalArgumentException("Action name cannot be null or empty"); + } + + return scriptMetadataCache.get(actionName); } /** - * Build a GroovyCodeSource object and add it to the class loader of the groovyScriptEngine - * - * @param groovyScript groovy script as a string - * @param actionName Name of the action - * @return Built GroovyCodeSource + * Creates GroovyCodeSource for compilation. */ private GroovyCodeSource buildClassScript(String groovyScript, String actionName) { return new GroovyCodeSource(groovyScript, actionName, "/groovy/script"); } + /** + * Persists script to storage. + */ private void saveScript(String actionName, String script) { GroovyAction groovyScript = new GroovyAction(actionName, script); persistenceService.save(groovyScript); LOGGER.info("The script {} has been persisted.", actionName); } + /** + * Refreshes scripts from persistence with selective recompilation. + * Uses hash-based change detection and atomic cache updates. + */ private void refreshGroovyActions() { - Map refreshedGroovyCodeSourceMap = new HashMap<>(); - persistenceService.getAllItems(GroovyAction.class).forEach(groovyAction -> refreshedGroovyCodeSourceMap - .put(groovyAction.getName(), buildClassScript(groovyAction.getScript(), groovyAction.getName()))); - groovyCodeSourceMap = refreshedGroovyCodeSourceMap; + long startTime = System.currentTimeMillis(); + + Map newMetadataCache = new ConcurrentHashMap<>(); + int unchangedCount = 0; + int recompiledCount = 0; + int errorCount = 0; + long totalCompilationTime = 0; + + for (GroovyAction groovyAction : persistenceService.getAllItems(GroovyAction.class)) { + String actionName = groovyAction.getName(); + String scriptContent = groovyAction.getScript(); + + try { + ScriptMetadata existingMetadata = scriptMetadataCache.get(actionName); + if (existingMetadata != null && !existingMetadata.hasChanged(scriptContent)) { + newMetadataCache.put(actionName, existingMetadata); + unchangedCount++; + LOGGER.debug("Script {} unchanged during refresh, keeping cached version", actionName); + } else { + if (recompiledCount == 0) { + LOGGER.info("Refreshing scripts from persistence layer..."); + } + + long compilationStartTime = System.currentTimeMillis(); + Class scriptClass = compileScript(actionName, scriptContent); + long compilationTime = System.currentTimeMillis() - compilationStartTime; + totalCompilationTime += compilationTime; + + ScriptMetadata metadata = new ScriptMetadata(actionName, scriptContent, scriptClass); + newMetadataCache.put(actionName, metadata); + recompiledCount++; + LOGGER.info("Script {} recompiled during refresh ({}ms)", actionName, compilationTime); + } + + } catch (Exception e) { + if (errorCount == 0 && recompiledCount == 0) { + LOGGER.info("Refreshing scripts from persistence layer..."); + } + + errorCount++; + LOGGER.error("Failed to refresh script: {}", actionName, e); + + ScriptMetadata existingMetadata = scriptMetadataCache.get(actionName); + if (existingMetadata != null) { + newMetadataCache.put(actionName, existingMetadata); + LOGGER.warn("Keeping existing version of script {} due to compilation error", actionName); + } + } + } + + this.scriptMetadataCache = newMetadataCache; + + if (recompiledCount > 0 || errorCount > 0) { + long totalTime = System.currentTimeMillis() - startTime; + LOGGER.info("Script refresh completed: {} unchanged, {} recompiled, {} errors. Total time: {}ms", + unchangedCount, recompiledCount, errorCount, totalTime); + LOGGER.debug("Refresh metrics: Recompilation time: {}ms, Cache update overhead: {}ms", + totalCompilationTime, totalTime - totalCompilationTime); + } else { + LOGGER.debug("Script refresh completed: {} scripts checked, no changes detected ({}ms)", + unchangedCount, System.currentTimeMillis() - startTime); + } } + /** + * Initializes periodic script refresh timer. + */ private void initializeTimers() { TimerTask task = new TimerTask() { @Override From b6f3781ea756f4d828be87c2389be0a2bb13250d Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sat, 12 Jul 2025 21:03:31 +0200 Subject: [PATCH 2/6] UNOMI-897: Fix data corruption and performance issues in GroovyActionDispatcher - Fixed data corruption issue by removing shared GroovyShell instance with separate script instances for separate variables - Improved performance by moving to pre-compilation of scripts to avoid on-the-fly and previously systematic compilation of Groovy scripts - Add ScriptMetadata class for script management --- .../unomi/groovy/actions/ScriptMetadata.java | 156 ++++++++++++++++++ .../unomi/itests/GroovyActionsServiceIT.java | 18 +- 2 files changed, 165 insertions(+), 9 deletions(-) create mode 100644 extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/ScriptMetadata.java diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/ScriptMetadata.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/ScriptMetadata.java new file mode 100644 index 0000000000..724dd9098b --- /dev/null +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/ScriptMetadata.java @@ -0,0 +1,156 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.unomi.groovy.actions; + +import groovy.lang.Script; +import java.security.MessageDigest; +import java.security.NoSuchAlgorithmException; +import java.nio.charset.StandardCharsets; +import java.util.Base64; + +/** + * Metadata container for compiled Groovy scripts with hash-based change detection. + *

+ * This class encapsulates all metadata associated with a compiled Groovy script, + * including content hash for efficient change detection and the compiled class + * for direct execution without recompilation. + *

+ * + *

+ * Thread Safety: This class is immutable and thread-safe. All fields are final + * and the class provides no methods to modify its state after construction. + *

+ * + * @since 2.7.0 + */ +public final class ScriptMetadata { + private final String actionName; + private final String scriptContent; + private final String contentHash; + private final long lastModified; + private final Class compiledClass; + + /** + * Constructs a new ScriptMetadata instance. + * + * @param actionName the unique name/identifier of the action + * @param scriptContent the raw Groovy script content + * @param compiledClass the compiled Groovy script class + * @throws IllegalArgumentException if any parameter is null + */ + public ScriptMetadata(String actionName, String scriptContent, Class compiledClass) { + if (actionName == null) { + throw new IllegalArgumentException("Action name cannot be null"); + } + if (scriptContent == null) { + throw new IllegalArgumentException("Script content cannot be null"); + } + if (compiledClass == null) { + throw new IllegalArgumentException("Compiled class cannot be null"); + } + + this.actionName = actionName; + this.scriptContent = scriptContent; + this.contentHash = calculateHash(scriptContent); + this.lastModified = System.currentTimeMillis(); + this.compiledClass = compiledClass; + } + + /** + * Calculates SHA-256 hash of the given content. + * + * @param content the content to hash + * @return Base64 encoded SHA-256 hash + * @throws RuntimeException if SHA-256 algorithm is not available + */ + private String calculateHash(String content) { + try { + MessageDigest digest = MessageDigest.getInstance("SHA-256"); + byte[] hash = digest.digest(content.getBytes(StandardCharsets.UTF_8)); + return Base64.getEncoder().encodeToString(hash); + } catch (NoSuchAlgorithmException e) { + throw new RuntimeException("SHA-256 algorithm not available", e); + } + } + + /** + * Determines if the script content has changed compared to new content. + *

+ * This method uses SHA-256 hash comparison for efficient change detection + * without storing or comparing the full script content. + *

+ * + * @param newContent the new script content to compare against + * @return {@code true} if content has changed, {@code false} if unchanged + * @throws IllegalArgumentException if newContent is null + */ + public boolean hasChanged(String newContent) { + if (newContent == null) { + throw new IllegalArgumentException("New content cannot be null"); + } + return !contentHash.equals(calculateHash(newContent)); + } + + /** + * Returns the action name/identifier. + * + * @return the action name, never null + */ + public String getActionName() { + return actionName; + } + + /** + * Returns the original script content. + * + * @return the script content, never null + */ + public String getScriptContent() { + return scriptContent; + } + + /** + * Returns the SHA-256 hash of the script content. + * + * @return Base64 encoded content hash, never null + */ + public String getContentHash() { + return contentHash; + } + + /** + * Returns the timestamp when this metadata was created. + * + * @return creation timestamp in milliseconds since epoch + */ + public long getLastModified() { + return lastModified; + } + + /** + * Returns the compiled Groovy script class. + *

+ * This class can be used to create new script instances for execution + * without requiring recompilation. + *

+ * + * @return the compiled script class, never null + */ + public Class getCompiledClass() { + return compiledClass; + } +} \ No newline at end of file diff --git a/itests/src/test/java/org/apache/unomi/itests/GroovyActionsServiceIT.java b/itests/src/test/java/org/apache/unomi/itests/GroovyActionsServiceIT.java index 85d7d3385f..5af563e3d6 100644 --- a/itests/src/test/java/org/apache/unomi/itests/GroovyActionsServiceIT.java +++ b/itests/src/test/java/org/apache/unomi/itests/GroovyActionsServiceIT.java @@ -17,7 +17,7 @@ package org.apache.unomi.itests; -import groovy.lang.GroovyCodeSource; +import groovy.lang.Script; import org.apache.commons.io.IOUtils; import org.apache.unomi.api.Event; import org.apache.unomi.api.Profile; @@ -95,7 +95,7 @@ public void testGroovyActionsService_triggerGroovyAction() throws IOException, I groovyActionsService.save(UPDATE_ADDRESS_ACTION, loadGroovyAction(UPDATE_ADDRESS_ACTION_GROOVY_FILE)); keepTrying("Failed waiting for the creation of the GroovyAction for the trigger action test", - () -> groovyActionsService.getGroovyCodeSource(UPDATE_ADDRESS_ACTION), Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); + () -> groovyActionsService.getCompiledScript(UPDATE_ADDRESS_ACTION), Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); ActionType actionType = keepTrying("Failed waiting for the creation of the GroovyAction for trigger action test", () -> definitionsService.getActionType(UPDATE_ADDRESS_GROOVY_ACTION), Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); @@ -114,10 +114,10 @@ public void testGroovyActionsService_saveActionAndTestSavedValues() throws IOExc ActionType actionType = keepTrying("Failed waiting for the creation of the GroovyAction for the save test", () -> definitionsService.getActionType(UPDATE_ADDRESS_GROOVY_ACTION), Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); - GroovyCodeSource groovyCodeSource = keepTrying("Failed waiting for the creation of the GroovyAction for the save test", - () -> groovyActionsService.getGroovyCodeSource(UPDATE_ADDRESS_ACTION), Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); + Class compiledScript = keepTrying("Failed waiting for the creation of the GroovyAction for the save test", + () -> groovyActionsService.getCompiledScript(UPDATE_ADDRESS_ACTION), Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); - Assert.assertEquals(UPDATE_ADDRESS_ACTION, groovyActionsService.getGroovyCodeSource(UPDATE_ADDRESS_ACTION).getName()); + Assert.assertEquals(UPDATE_ADDRESS_ACTION, compiledScript.getSimpleName()); Assert.assertTrue(actionType.getMetadata().getId().contains(UPDATE_ADDRESS_GROOVY_ACTION)); Assert.assertEquals(2, actionType.getMetadata().getSystemTags().size()); @@ -133,14 +133,14 @@ public void testGroovyActionsService_saveActionAndTestSavedValues() throws IOExc public void testGroovyActionsService_removeGroovyAction() throws IOException, InterruptedException { groovyActionsService.save(UPDATE_ADDRESS_ACTION, loadGroovyAction(UPDATE_ADDRESS_ACTION_GROOVY_FILE)); - GroovyCodeSource groovyCodeSource = keepTrying("Failed waiting for the creation of the GroovyAction for the remove test", - () -> groovyActionsService.getGroovyCodeSource(UPDATE_ADDRESS_ACTION), Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); + Class compiledScript = keepTrying("Failed waiting for the creation of the GroovyAction for the remove test", + () -> groovyActionsService.getCompiledScript(UPDATE_ADDRESS_ACTION), Objects::nonNull, DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); - Assert.assertNotNull(groovyCodeSource); + Assert.assertNotNull(compiledScript); groovyActionsService.remove(UPDATE_ADDRESS_ACTION); - waitForNullValue("Groovy action is still present", () -> groovyActionsService.getGroovyCodeSource(UPDATE_ADDRESS_ACTION), + waitForNullValue("Groovy action is still present", () -> groovyActionsService.getCompiledScript(UPDATE_ADDRESS_ACTION), DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES); waitForNullValue("Action type is still present", () -> definitionsService.getActionType(UPDATE_ADDRESS_GROOVY_ACTION), From 3bcf4da514a717b307c0c2b56f0cb6ff73c31c10 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 13 Jul 2025 08:20:13 +0200 Subject: [PATCH 3/6] UNOMI-897 Code cleanup --- .../impl/GroovyActionsServiceImpl.java | 206 ++++++++++-------- 1 file changed, 113 insertions(+), 93 deletions(-) diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java index 25a42ffbb5..effc2be677 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java @@ -74,7 +74,7 @@ public class GroovyActionsServiceImpl implements GroovyActionsService { private GroovyScriptEngine groovyScriptEngine; private CompilerConfiguration compilerConfiguration; private ScheduledFuture scheduledFuture; - + private final Object compilationLock = new Object(); private GroovyShell compilationShell; private volatile Map scriptMetadataCache = new ConcurrentHashMap<>(); @@ -117,16 +117,16 @@ public void start(GroovyActionsServiceConfig config, BundleContext bundleContext // Initialize Groovy compiler and compilation shell initializeGroovyCompiler(); - + try { loadBaseScript(); } catch (IOException e) { LOGGER.error("Failed to load base script", e); } - + // PRE-COMPILE ALL SCRIPTS AT STARTUP (no on-demand compilation) preloadAllScripts(); - + initializeTimers(); LOGGER.info("Groovy action service initialized with {} scripts", scriptMetadataCache.size()); } @@ -140,7 +140,15 @@ public void onDestroy() { } /** - * Loads the base script that provides utility functions for Groovy actions. + * Load the Base script. + * It's a script which provides utility functions that we can use in other groovy script + * The functions added by the base script could be called by the groovy actions executed in + * {@link org.apache.unomi.groovy.actions.GroovyActionDispatcher#execute} + * The base script would be added in the configuration of the {@link GroovyActionsServiceImpl#groovyShell GroovyShell} , so when a + * script will be parsed with the GroovyShell (groovyShell.parse(...)), the action will extends the base script, so the functions + * could be called + * + * @throws IOException */ private void loadBaseScript() throws IOException { URL groovyBaseScriptURL = bundleContext.getBundle().getEntry("META-INF/base/BaseScript.groovy"); @@ -161,52 +169,51 @@ private void initializeGroovyCompiler() { compilerConfiguration.addCompilationCustomizers(createImportCustomizer()); compilerConfiguration.setScriptBaseClass(BASE_SCRIPT_NAME); groovyScriptEngine.setConfig(compilerConfiguration); - + // Create single shared shell for compilation only this.compilationShell = new GroovyShell(groovyScriptEngine.getGroovyClassLoader(), compilerConfiguration); } - + /** * Pre-compiles all scripts at startup to eliminate runtime compilation overhead. */ private void preloadAllScripts() { long startTime = System.currentTimeMillis(); LOGGER.info("Pre-compiling all Groovy scripts at startup..."); - + int successCount = 0; int failureCount = 0; long totalCompilationTime = 0; - + for (GroovyAction groovyAction : persistenceService.getAllItems(GroovyAction.class)) { try { String actionName = groovyAction.getName(); String scriptContent = groovyAction.getScript(); - + long scriptStartTime = System.currentTimeMillis(); - Class scriptClass = compileScript(actionName, scriptContent); + ScriptMetadata metadata = compileAndCreateMetadata(actionName, scriptContent); long scriptCompilationTime = System.currentTimeMillis() - scriptStartTime; totalCompilationTime += scriptCompilationTime; - - ScriptMetadata metadata = new ScriptMetadata(actionName, scriptContent, scriptClass); + scriptMetadataCache.put(actionName, metadata); - + successCount++; LOGGER.debug("Pre-compiled script: {} ({}ms)", actionName, scriptCompilationTime); - + } catch (Exception e) { failureCount++; LOGGER.error("Failed to pre-compile script: {}", groovyAction.getName(), e); } } - + long totalTime = System.currentTimeMillis() - startTime; - LOGGER.info("Pre-compilation completed: {} scripts successfully compiled, {} failures. Total time: {}ms", + LOGGER.info("Pre-compilation completed: {} scripts successfully compiled, {} failures. Total time: {}ms", successCount, failureCount, totalTime); - LOGGER.debug("Pre-compilation metrics: Average per script: {}ms, Compilation overhead: {}ms", + LOGGER.debug("Pre-compilation metrics: Average per script: {}ms, Compilation overhead: {}ms", successCount > 0 ? totalCompilationTime / successCount : 0, totalTime - totalCompilationTime); } - + /** * Thread-safe script compilation using synchronized shared shell. */ @@ -227,48 +234,76 @@ private ImportCustomizer createImportCustomizer() { return importCustomizer; } + /** + * Validates that a string parameter is not null or empty. + */ + private void validateNotEmpty(String value, String parameterName) { + if (value == null || value.trim().isEmpty()) { + throw new IllegalArgumentException(parameterName + " cannot be null or empty"); + } + } + + /** + * Compiles a script and creates metadata with timing information. + */ + private ScriptMetadata compileAndCreateMetadata(String actionName, String scriptContent) { + long compilationStartTime = System.currentTimeMillis(); + Class scriptClass = compileScript(actionName, scriptContent); + long compilationTime = System.currentTimeMillis() - compilationStartTime; + + LOGGER.debug("Script {} compiled in {}ms", actionName, compilationTime); + return new ScriptMetadata(actionName, scriptContent, scriptClass); + } + + /** + * Extracts Action annotation from script class if present. + */ + private Action getActionAnnotation(Class scriptClass) { + try { + return scriptClass.getMethod("execute").getAnnotation(Action.class); + } catch (Exception e) { + LOGGER.error("Failed to extract action annotation", e); + return null; + } + } + /** * {@inheritDoc} * Implementation performs hash-based change detection to skip unnecessary recompilation. */ @Override public void save(String actionName, String groovyScript) { - if (actionName == null || actionName.trim().isEmpty()) { - throw new IllegalArgumentException("Action name cannot be null or empty"); - } - if (groovyScript == null || groovyScript.trim().isEmpty()) { - throw new IllegalArgumentException("Groovy script cannot be null or empty"); - } - + validateNotEmpty(actionName, "Action name"); + validateNotEmpty(groovyScript, "Groovy script"); + long startTime = System.currentTimeMillis(); LOGGER.info("Saving script: {}", actionName); - + try { ScriptMetadata existingMetadata = scriptMetadataCache.get(actionName); if (existingMetadata != null && !existingMetadata.hasChanged(groovyScript)) { - LOGGER.info("Script {} unchanged, skipping recompilation ({}ms)", actionName, + LOGGER.info("Script {} unchanged, skipping recompilation ({}ms)", actionName, System.currentTimeMillis() - startTime); return; } - + long compilationStartTime = System.currentTimeMillis(); - Class scriptClass = compileScript(actionName, groovyScript); + ScriptMetadata metadata = compileAndCreateMetadata(actionName, groovyScript); long compilationTime = System.currentTimeMillis() - compilationStartTime; - - Action actionAnnotation = scriptClass.getMethod("execute").getAnnotation(Action.class); + + Action actionAnnotation = getActionAnnotation(metadata.getCompiledClass()); if (actionAnnotation != null) { saveActionType(actionAnnotation); } - + saveScript(actionName, groovyScript); - - ScriptMetadata metadata = new ScriptMetadata(actionName, groovyScript, scriptClass); + scriptMetadataCache.put(actionName, metadata); - + long totalTime = System.currentTimeMillis() - startTime; - LOGGER.info("Script {} saved and compiled successfully (total: {}ms, compilation: {}ms)", + LOGGER.info("Script {} saved and compiled successfully (total: {}ms, compilation: {}ms)", actionName, totalTime, compilationTime); - + } catch (Exception e) { long totalTime = System.currentTimeMillis() - startTime; LOGGER.error("Failed to save script: {} ({}ms)", actionName, totalTime, e); @@ -298,27 +333,20 @@ private void saveActionType(Action action) { */ @Override public void remove(String id) { - if (id == null || id.trim().isEmpty()) { - throw new IllegalArgumentException("Script ID cannot be null or empty"); - } - + validateNotEmpty(id, "Script ID"); + LOGGER.info("Removing script: {}", id); - + ScriptMetadata removedMetadata = scriptMetadataCache.remove(id); persistenceService.remove(id, GroovyAction.class); - - try { - if (removedMetadata != null) { - Class cachedClass = removedMetadata.getCompiledClass(); - Action actionAnnotation = cachedClass.getMethod("execute").getAnnotation(Action.class); - if (actionAnnotation != null) { - definitionsService.removeActionType(actionAnnotation.id()); - } + + if (removedMetadata != null) { + Action actionAnnotation = getActionAnnotation(removedMetadata.getCompiledClass()); + if (actionAnnotation != null) { + definitionsService.removeActionType(actionAnnotation.id()); } - } catch (Exception e) { - LOGGER.error("Failed to remove action type for script: {}", id, e); } - + LOGGER.info("Script {} removed successfully", id); } @@ -329,53 +357,48 @@ public void remove(String id) { */ @Override public Class getOrCompileScript(String id) { - if (id == null || id.trim().isEmpty()) { - throw new IllegalArgumentException("Script ID cannot be null or empty"); - } - + validateNotEmpty(id, "Script ID"); + ScriptMetadata metadata = scriptMetadataCache.get(id); if (metadata != null) { return metadata.getCompiledClass(); } - + long startTime = System.currentTimeMillis(); LOGGER.warn("Script {} not found in cache, compiling on-demand (performance warning)", id); - + GroovyAction groovyAction = persistenceService.load(id, GroovyAction.class); if (groovyAction == null) { - LOGGER.warn("Script {} not found in persistence, returning null ({}ms)", id, + LOGGER.warn("Script {} not found in persistence, returning null ({}ms)", id, System.currentTimeMillis() - startTime); return null; } - + try { long compilationStartTime = System.currentTimeMillis(); - Class scriptClass = compileScript(id, groovyAction.getScript()); + ScriptMetadata newMetadata = compileAndCreateMetadata(id, groovyAction.getScript()); long compilationTime = System.currentTimeMillis() - compilationStartTime; - - ScriptMetadata newMetadata = new ScriptMetadata(id, groovyAction.getScript(), scriptClass); + scriptMetadataCache.put(id, newMetadata); - + long totalTime = System.currentTimeMillis() - startTime; - LOGGER.warn("On-demand compilation completed for {} (total: {}ms, compilation: {}ms)", + LOGGER.warn("On-demand compilation completed for {} (total: {}ms, compilation: {}ms)", id, totalTime, compilationTime); - return scriptClass; + return newMetadata.getCompiledClass(); } catch (Exception e) { long totalTime = System.currentTimeMillis() - startTime; LOGGER.error("Failed to compile script {} on-demand ({}ms)", id, totalTime, e); return null; } } - + /** * {@inheritDoc} */ @Override public Class getCompiledScript(String id) { - if (id == null || id.trim().isEmpty()) { - throw new IllegalArgumentException("Script ID cannot be null or empty"); - } - + validateNotEmpty(id, "Script ID"); + ScriptMetadata metadata = scriptMetadataCache.get(id); if (metadata == null) { LOGGER.warn("Script {} not found in cache", id); @@ -383,16 +406,14 @@ public Class getCompiledScript(String id) { } return metadata.getCompiledClass(); } - + /** * {@inheritDoc} */ @Override public ScriptMetadata getScriptMetadata(String actionName) { - if (actionName == null || actionName.trim().isEmpty()) { - throw new IllegalArgumentException("Action name cannot be null or empty"); - } - + validateNotEmpty(actionName, "Action name"); + return scriptMetadataCache.get(actionName); } @@ -418,17 +439,17 @@ private void saveScript(String actionName, String script) { */ private void refreshGroovyActions() { long startTime = System.currentTimeMillis(); - + Map newMetadataCache = new ConcurrentHashMap<>(); int unchangedCount = 0; int recompiledCount = 0; int errorCount = 0; long totalCompilationTime = 0; - + for (GroovyAction groovyAction : persistenceService.getAllItems(GroovyAction.class)) { String actionName = groovyAction.getName(); String scriptContent = groovyAction.getScript(); - + try { ScriptMetadata existingMetadata = scriptMetadataCache.get(actionName); if (existingMetadata != null && !existingMetadata.hasChanged(scriptContent)) { @@ -439,26 +460,25 @@ private void refreshGroovyActions() { if (recompiledCount == 0) { LOGGER.info("Refreshing scripts from persistence layer..."); } - + long compilationStartTime = System.currentTimeMillis(); - Class scriptClass = compileScript(actionName, scriptContent); + ScriptMetadata metadata = compileAndCreateMetadata(actionName, scriptContent); long compilationTime = System.currentTimeMillis() - compilationStartTime; totalCompilationTime += compilationTime; - - ScriptMetadata metadata = new ScriptMetadata(actionName, scriptContent, scriptClass); + newMetadataCache.put(actionName, metadata); recompiledCount++; LOGGER.info("Script {} recompiled during refresh ({}ms)", actionName, compilationTime); } - + } catch (Exception e) { if (errorCount == 0 && recompiledCount == 0) { LOGGER.info("Refreshing scripts from persistence layer..."); } - + errorCount++; LOGGER.error("Failed to refresh script: {}", actionName, e); - + ScriptMetadata existingMetadata = scriptMetadataCache.get(actionName); if (existingMetadata != null) { newMetadataCache.put(actionName, existingMetadata); @@ -466,17 +486,17 @@ private void refreshGroovyActions() { } } } - + this.scriptMetadataCache = newMetadataCache; - + if (recompiledCount > 0 || errorCount > 0) { long totalTime = System.currentTimeMillis() - startTime; - LOGGER.info("Script refresh completed: {} unchanged, {} recompiled, {} errors. Total time: {}ms", + LOGGER.info("Script refresh completed: {} unchanged, {} recompiled, {} errors. Total time: {}ms", unchangedCount, recompiledCount, errorCount, totalTime); - LOGGER.debug("Refresh metrics: Recompilation time: {}ms, Cache update overhead: {}ms", + LOGGER.debug("Refresh metrics: Recompilation time: {}ms, Cache update overhead: {}ms", totalCompilationTime, totalTime - totalCompilationTime); } else { - LOGGER.debug("Script refresh completed: {} scripts checked, no changes detected ({}ms)", + LOGGER.debug("Script refresh completed: {} scripts checked, no changes detected ({}ms)", unchangedCount, System.currentTimeMillis() - startTime); } } From b129a070ee81c095fb9727a69d7f18b38dbac235 Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Fri, 18 Jul 2025 15:12:37 +0200 Subject: [PATCH 4/6] UNOMI-897 Fix issues reported by initial code review --- .../unomi/groovy/actions/GroovyActionDispatcher.java | 2 +- .../org/apache/unomi/groovy/actions/ScriptMetadata.java | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java index 224e877522..093a91d6f9 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/GroovyActionDispatcher.java @@ -79,7 +79,7 @@ public Integer execute(Action action, Event event, String actionName) { } try { - Script script = scriptClass.newInstance(); + Script script = scriptClass.getDeclaredConstructor().newInstance(); setScriptVariables(script, action, event); return new MetricAdapter(metricsService, this.getClass().getName() + ".action.groovy." + actionName) { diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/ScriptMetadata.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/ScriptMetadata.java index 724dd9098b..57b44e3ae4 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/ScriptMetadata.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/ScriptMetadata.java @@ -41,7 +41,7 @@ public final class ScriptMetadata { private final String actionName; private final String scriptContent; private final String contentHash; - private final long lastModified; + private final long creationTime; private final Class compiledClass; /** @@ -66,7 +66,7 @@ public ScriptMetadata(String actionName, String scriptContent, Class Date: Sat, 19 Jul 2025 13:20:13 +0200 Subject: [PATCH 5/6] UNOMI-897 Fix issues reported by code review - Standardized API parameter naming on actionName - Added logic to avoid logging compilation error on refreshes multiple times - Removed the unused getOrCompileScript method --- .../services/GroovyActionsService.java | 28 ++---- .../impl/GroovyActionsServiceImpl.java | 91 ++++++++----------- 2 files changed, 47 insertions(+), 72 deletions(-) diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java index aa6bf8b1bc..1b4bf14be2 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/GroovyActionsService.java @@ -28,7 +28,7 @@ * Groovy scripts as actions within the Apache Unomi framework. It implements * optimized compilation and caching strategies to achieve high performance. *

- * + * *

* Key features: *

    @@ -38,12 +38,12 @@ *
  • Unified caching architecture for compiled scripts
    • *
*

- * + * *

* Thread Safety: Implementations must be thread-safe as this service * is accessed concurrently during script execution. *

- * + * * @see GroovyAction * @see ScriptMetadata * @since 2.7.0 @@ -72,24 +72,10 @@ public interface GroovyActionsService { * and cleans up any registered action types in the definitions service. *

* - * @param id the unique identifier of the action to remove + * @param actionName the unique identifier of the action to remove * @throws IllegalArgumentException if id is null */ - void remove(String id); - - /** - * Retrieves a compiled script class, compiling on-demand if not cached. - *

- * This method first checks the cache for a compiled version. If not found, - * it loads the script from persistence and compiles it. This method is - * provided for backward compatibility but may have performance implications. - *

- * - * @param id the unique identifier of the action - * @return the compiled script class, or {@code null} if not found - * @throws IllegalArgumentException if id is null - */ - Class getOrCompileScript(String id); + void remove(String actionName); /** * Retrieves a pre-compiled script class from cache. @@ -99,11 +85,11 @@ public interface GroovyActionsService { * {@code null} if the script is not found in the cache. *

* - * @param id the unique identifier of the action + * @param actionName the unique identifier of the action * @return the compiled script class, or {@code null} if not found in cache * @throws IllegalArgumentException if id is null */ - Class getCompiledScript(String id); + Class getCompiledScript(String actionName); /** * Retrieves script metadata for monitoring and change detection. diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java index effc2be677..07f216276e 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java @@ -46,6 +46,7 @@ import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.HashSet; +import java.util.Set; import java.util.Map; import java.util.TimerTask; @@ -78,6 +79,8 @@ public class GroovyActionsServiceImpl implements GroovyActionsService { private final Object compilationLock = new Object(); private GroovyShell compilationShell; private volatile Map scriptMetadataCache = new ConcurrentHashMap<>(); + private final Map> loggedRefreshErrors = new ConcurrentHashMap<>(); + private static final int MAX_LOGGED_ERRORS = 100; // Prevent memory leak private static final Logger LOGGER = LoggerFactory.getLogger(GroovyActionsServiceImpl.class.getName()); private static final String BASE_SCRIPT_NAME = "BaseScript"; @@ -144,7 +147,7 @@ public void onDestroy() { * It's a script which provides utility functions that we can use in other groovy script * The functions added by the base script could be called by the groovy actions executed in * {@link org.apache.unomi.groovy.actions.GroovyActionDispatcher#execute} - * The base script would be added in the configuration of the {@link GroovyActionsServiceImpl#groovyShell GroovyShell} , so when a + * The base script would be added in the configuration of the {@link GroovyActionsServiceImpl#compilationShell GroovyShell} , so when a * script will be parsed with the GroovyShell (groovyShell.parse(...)), the action will extends the base script, so the functions * could be called * @@ -332,13 +335,16 @@ private void saveActionType(Action action) { * {@inheritDoc} */ @Override - public void remove(String id) { - validateNotEmpty(id, "Script ID"); + public void remove(String actionName) { + validateNotEmpty(actionName, "Action name"); - LOGGER.info("Removing script: {}", id); + LOGGER.info("Removing script: {}", actionName); - ScriptMetadata removedMetadata = scriptMetadataCache.remove(id); - persistenceService.remove(id, GroovyAction.class); + ScriptMetadata removedMetadata = scriptMetadataCache.remove(actionName); + persistenceService.remove(actionName, GroovyAction.class); + + // Clean up error tracking to prevent memory leak + loggedRefreshErrors.remove(actionName); if (removedMetadata != null) { Action actionAnnotation = getActionAnnotation(removedMetadata.getCompiledClass()); @@ -347,49 +353,7 @@ public void remove(String id) { } } - LOGGER.info("Script {} removed successfully", id); - } - - - /** - * {@inheritDoc} - * Performance Warning: Compiles on-demand if not cached. - */ - @Override - public Class getOrCompileScript(String id) { - validateNotEmpty(id, "Script ID"); - - ScriptMetadata metadata = scriptMetadataCache.get(id); - if (metadata != null) { - return metadata.getCompiledClass(); - } - - long startTime = System.currentTimeMillis(); - LOGGER.warn("Script {} not found in cache, compiling on-demand (performance warning)", id); - - GroovyAction groovyAction = persistenceService.load(id, GroovyAction.class); - if (groovyAction == null) { - LOGGER.warn("Script {} not found in persistence, returning null ({}ms)", id, - System.currentTimeMillis() - startTime); - return null; - } - - try { - long compilationStartTime = System.currentTimeMillis(); - ScriptMetadata newMetadata = compileAndCreateMetadata(id, groovyAction.getScript()); - long compilationTime = System.currentTimeMillis() - compilationStartTime; - - scriptMetadataCache.put(id, newMetadata); - - long totalTime = System.currentTimeMillis() - startTime; - LOGGER.warn("On-demand compilation completed for {} (total: {}ms, compilation: {}ms)", - id, totalTime, compilationTime); - return newMetadata.getCompiledClass(); - } catch (Exception e) { - long totalTime = System.currentTimeMillis() - startTime; - LOGGER.error("Failed to compile script {} on-demand ({}ms)", id, totalTime, e); - return null; - } + LOGGER.info("Script {} removed successfully", actionName); } /** @@ -466,6 +430,9 @@ private void refreshGroovyActions() { long compilationTime = System.currentTimeMillis() - compilationStartTime; totalCompilationTime += compilationTime; + // Clear error tracking on successful compilation + loggedRefreshErrors.remove(actionName); + newMetadataCache.put(actionName, metadata); recompiledCount++; LOGGER.info("Script {} recompiled during refresh ({}ms)", actionName, compilationTime); @@ -477,12 +444,34 @@ private void refreshGroovyActions() { } errorCount++; - LOGGER.error("Failed to refresh script: {}", actionName, e); + + // Prevent log spam for repeated compilation errors during refresh + String errorMessage = e.getMessage(); + Set scriptErrors = loggedRefreshErrors.get(actionName); + + if (scriptErrors == null || !scriptErrors.contains(errorMessage)) { + LOGGER.error("Failed to refresh script: {}", actionName, e); + + // Prevent memory leak by limiting tracked errors before adding new entries + if (scriptErrors == null && loggedRefreshErrors.size() >= MAX_LOGGED_ERRORS) { + // Remove one random entry to make space (simple eviction) + String firstKey = loggedRefreshErrors.keySet().iterator().next(); + loggedRefreshErrors.remove(firstKey); + } + + // Now safely add the error + if (scriptErrors == null) { + scriptErrors = ConcurrentHashMap.newKeySet(); + loggedRefreshErrors.put(actionName, scriptErrors); + } + scriptErrors.add(errorMessage); + + LOGGER.warn("Keeping existing version of script {} due to compilation error", actionName); + } ScriptMetadata existingMetadata = scriptMetadataCache.get(actionName); if (existingMetadata != null) { newMetadataCache.put(actionName, existingMetadata); - LOGGER.warn("Keeping existing version of script {} due to compilation error", actionName); } } } From d091295b906b08ad6fa2ecee9032f9140299a83a Mon Sep 17 00:00:00 2001 From: Serge Huber Date: Sun, 20 Jul 2025 07:57:34 +0200 Subject: [PATCH 6/6] Add new error count to prevent from logging errors all the time. --- .../actions/services/impl/GroovyActionsServiceImpl.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java index 07f216276e..3ad70b69b5 100644 --- a/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java +++ b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/services/impl/GroovyActionsServiceImpl.java @@ -408,6 +408,7 @@ private void refreshGroovyActions() { int unchangedCount = 0; int recompiledCount = 0; int errorCount = 0; + int newErrorCount = 0; long totalCompilationTime = 0; for (GroovyAction groovyAction : persistenceService.getAllItems(GroovyAction.class)) { @@ -439,7 +440,7 @@ private void refreshGroovyActions() { } } catch (Exception e) { - if (errorCount == 0 && recompiledCount == 0) { + if (newErrorCount == 0 && recompiledCount == 0) { LOGGER.info("Refreshing scripts from persistence layer..."); } @@ -450,6 +451,7 @@ private void refreshGroovyActions() { Set scriptErrors = loggedRefreshErrors.get(actionName); if (scriptErrors == null || !scriptErrors.contains(errorMessage)) { + newErrorCount++; LOGGER.error("Failed to refresh script: {}", actionName, e); // Prevent memory leak by limiting tracked errors before adding new entries @@ -478,7 +480,7 @@ private void refreshGroovyActions() { this.scriptMetadataCache = newMetadataCache; - if (recompiledCount > 0 || errorCount > 0) { + if (recompiledCount > 0 || newErrorCount > 0) { long totalTime = System.currentTimeMillis() - startTime; LOGGER.info("Script refresh completed: {} unchanged, {} recompiled, {} errors. Total time: {}ms", unchangedCount, recompiledCount, errorCount, totalTime);