[UNOMI-897] Groovy data corruption and performance fixes (#720)
* 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-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-897 Code cleanup
* UNOMI-897 Fix issues reported by initial code review
* 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
* Add new error count to prevent from logging errors all the time.
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 87ed610..093a91d 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 @@
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<Integer>(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<? extends Script> 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.getDeclaredConstructor().newInstance();
+ setScriptVariables(script, action, event);
+
+ return new MetricAdapter<Integer>(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/ScriptMetadata.java b/extensions/groovy-actions/services/src/main/java/org/apache/unomi/groovy/actions/ScriptMetadata.java
new file mode 100644
index 0000000..57b44e3
--- /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.
+ * <p>
+ * 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.
+ * </p>
+ *
+ * <p>
+ * 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.
+ * </p>
+ *
+ * @since 2.7.0
+ */
+public final class ScriptMetadata {
+ private final String actionName;
+ private final String scriptContent;
+ private final String contentHash;
+ private final long creationTime;
+ private final Class<? extends Script> 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<? extends Script> 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.creationTime = 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.
+ * <p>
+ * This method uses SHA-256 hash comparison for efficient change detection
+ * without storing or comparing the full script content.
+ * </p>
+ *
+ * @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 getCreationTime() {
+ return creationTime;
+ }
+
+ /**
+ * Returns the compiled Groovy script class.
+ * <p>
+ * This class can be used to create new script instances for execution
+ * without requiring recompilation.
+ * </p>
+ *
+ * @return the compiled script class, never null
+ */
+ public Class<? extends Script> getCompiledClass() {
+ return compiledClass;
+ }
+}
\ No newline at end of file
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 4b6d545..1b4bf14 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,92 @@
*/
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.
+ * <p>
+ * 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.
+ * </p>
+ *
+ * <p>
+ * Key features:
+ * <ul>
+ * <li>Pre-compilation of scripts at startup</li>
+ * <li>Hash-based change detection for selective recompilation</li>
+ * <li>Thread-safe compilation and execution</li>
+ * <li>Unified caching architecture for compiled scripts</li>
+ * </ul>
+ * </p>
+ *
+ * <p>
+ * Thread Safety: Implementations must be thread-safe as this service
+ * is accessed concurrently during script execution.
+ * </p>
+ *
+ * @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.
+ * <p>
+ * 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.
+ * </p>
*
- * @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.
+ * <p>
+ * This method removes the action from both the cache and persistent storage,
+ * and cleans up any registered action types in the definitions service.
+ * </p>
*
- * @param id 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);
+ void remove(String actionName);
/**
- * Get a groovy code source object by an id
+ * Retrieves a pre-compiled script class from cache.
+ * <p>
+ * 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.
+ * </p>
*
- * @param id of the action to get
- * @return Groovy code source
+ * @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
*/
- GroovyCodeSource getGroovyCodeSource(String id);
+ Class<? extends Script> getCompiledScript(String actionName);
/**
- * Get an instantiated groovy shell object
+ * Retrieves script metadata for monitoring and change detection.
+ * <p>
+ * The returned metadata includes content hash, compilation timestamp,
+ * and the compiled class reference. This is useful for monitoring
+ * tools and debugging.
+ * </p>
*
- * @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 bee011c..3ad70b6 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,13 @@
import java.io.IOException;
import java.net.URL;
-import java.util.HashMap;
+import java.nio.charset.StandardCharsets;
import java.util.HashSet;
+import java.util.Set;
+
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 +59,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,17 +73,21 @@
private BundleContext bundleContext;
private GroovyScriptEngine groovyScriptEngine;
- private GroovyShell groovyShell;
- private Map<String, GroovyCodeSource> groovyCodeSourceMap;
+ private CompilerConfiguration compilerConfiguration;
private ScheduledFuture<?> scheduledFuture;
+ private final Object compilationLock = new Object();
+ private GroovyShell compilationShell;
+ private volatile Map<String, ScriptMetadata> scriptMetadataCache = new ConcurrentHashMap<>();
+ private final Map<String, Set<String>> 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";
private DefinitionsService definitionsService;
private PersistenceService persistenceService;
private SchedulerService schedulerService;
- private ActionExecutorDispatcher actionExecutorDispatcher;
private GroovyActionsServiceConfig config;
@Reference
@@ -96,14 +105,7 @@
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 +113,25 @@
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
@@ -140,7 +147,7 @@
* 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
*
@@ -152,25 +159,77 @@
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();
+ ScriptMetadata metadata = compileAndCreateMetadata(actionName, scriptContent);
+ long scriptCompilationTime = System.currentTimeMillis() - scriptStartTime;
+ totalCompilationTime += scriptCompilationTime;
+
+ 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<? extends Script> 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 +237,88 @@
return importCustomizer;
}
- @Override
- public void save(String actionName, String groovyScript) {
- GroovyCodeSource groovyCodeSource = buildClassScript(groovyScript, actionName);
- try {
- saveActionType(groovyShell.parse(groovyCodeSource).getClass().getMethod("execute").getAnnotation(Action.class));
- saveScript(actionName, groovyScript);
- LOGGER.info("The script {} has been loaded.", actionName);
- } catch (NoSuchMethodException e) {
- LOGGER.error("Failed to save the script {}", actionName, e);
+ /**
+ * 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");
}
}
/**
- * Build an action type from the annotation {@link Action}
- *
- * @param action Annotation containing the values to save
+ * Compiles a script and creates metadata with timing information.
+ */
+ private ScriptMetadata compileAndCreateMetadata(String actionName, String scriptContent) {
+ long compilationStartTime = System.currentTimeMillis();
+ Class<? extends Script> 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<? extends Script> 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) {
+ 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,
+ System.currentTimeMillis() - startTime);
+ return;
+ }
+
+ long compilationStartTime = System.currentTimeMillis();
+ ScriptMetadata metadata = compileAndCreateMetadata(actionName, groovyScript);
+ long compilationTime = System.currentTimeMillis() - compilationStartTime;
+
+ Action actionAnnotation = getActionAnnotation(metadata.getCompiledClass());
+ if (actionAnnotation != null) {
+ saveActionType(actionAnnotation);
+ }
+
+ saveScript(actionName, groovyScript);
+
+ 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);
+ }
+ }
+
+ /**
+ * 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 +331,170 @@
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);
- }
- persistenceService.remove(id, GroovyAction.class);
- }
- }
+ public void remove(String actionName) {
+ validateNotEmpty(actionName, "Action name");
- @Override
- public GroovyCodeSource getGroovyCodeSource(String id) {
- return groovyCodeSourceMap.get(id);
+ LOGGER.info("Removing script: {}", actionName);
+
+ 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());
+ if (actionAnnotation != null) {
+ definitionsService.removeActionType(actionAnnotation.id());
+ }
+ }
+
+ LOGGER.info("Script {} removed successfully", 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
+ * {@inheritDoc}
+ */
+ @Override
+ public Class<? extends Script> getCompiledScript(String id) {
+ validateNotEmpty(id, "Script ID");
+
+ ScriptMetadata metadata = scriptMetadataCache.get(id);
+ if (metadata == null) {
+ LOGGER.warn("Script {} not found in cache", id);
+ return null;
+ }
+ return metadata.getCompiledClass();
+ }
+
+ /**
+ * {@inheritDoc}
+ */
+ @Override
+ public ScriptMetadata getScriptMetadata(String actionName) {
+ validateNotEmpty(actionName, "Action name");
+
+ return scriptMetadataCache.get(actionName);
+ }
+
+ /**
+ * 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<String, GroovyCodeSource> 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<String, ScriptMetadata> newMetadataCache = new ConcurrentHashMap<>();
+ int unchangedCount = 0;
+ int recompiledCount = 0;
+ int errorCount = 0;
+ int newErrorCount = 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();
+ ScriptMetadata metadata = compileAndCreateMetadata(actionName, scriptContent);
+ 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);
+ }
+
+ } catch (Exception e) {
+ if (newErrorCount == 0 && recompiledCount == 0) {
+ LOGGER.info("Refreshing scripts from persistence layer...");
+ }
+
+ errorCount++;
+
+ // Prevent log spam for repeated compilation errors during refresh
+ String errorMessage = e.getMessage();
+ Set<String> 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
+ 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);
+ }
+ }
+ }
+
+ this.scriptMetadataCache = newMetadataCache;
+
+ 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);
+ 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
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 85d7d33..5af563e 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 @@
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 @@
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<? extends Script> 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_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<? extends Script> 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),
