[automation] Synchronize script action/condition execution if engine …

…implements Lock (#4426) This moves the locking mechanism added in #4402 to the inheritors of AbstractScriptModuleHandler to synchronize execution context access as well. This fixes the problem thathttps://github.com/openhab/openhab-addons/pull/17510 worked around by using Thread.sleep. Signed-off-by: Florian Hotze <[email protected]>
openhab · Oct 26, 2024 · 922a206 · 922a206
1 parent 52b26ba
commit 922a206
Show file tree

Hide file tree

Showing 3 changed files with 57 additions and 31 deletions.
diff --git a/...g/openhab/core/automation/module/script/internal/handler/AbstractScriptModuleHandler.java b/...g/openhab/core/automation/module/script/internal/handler/AbstractScriptModuleHandler.java
@@ -17,8 +17,6 @@
 import java.util.Map.Entry;
 import java.util.Optional;
 import java.util.UUID;
-import java.util.concurrent.TimeUnit;
-import java.util.concurrent.locks.Lock;
 
 import javax.script.Compilable;
 import javax.script.CompiledScript;
@@ -38,6 +36,8 @@
 
 /**
  * This is an abstract class that can be used when implementing any module handler that handles scripts.
+ * <p>
+ * Remember to implement multi-thread synchronization in the concrete handler if the script engine is not thread-safe!
  *
  * @author Kai Kreuzer - Initial contribution
  * @author Simon Merschjohann - Initial contribution
@@ -212,28 +212,14 @@ protected void resetExecutionContext(ScriptEngine engine, Map<String, ?> context
     protected @Nullable Object eval(ScriptEngine engine, String script) {
         try {
             if (compiledScript.isPresent()) {
-                if (engine instanceof Lock lock && !lock.tryLock(1, TimeUnit.MINUTES)) {
-                    logger.error("Failed to acquire lock within one minute for script module of rule with UID '{}'",
-                            ruleUID);
-                    return null;
-                }
                 logger.debug("Executing pre-compiled script of rule with UID '{}'", ruleUID);
-                try {
-                    return compiledScript.get().eval(engine.getContext());
-                } finally { // Make sure that Lock is unlocked regardless of an exception being thrown or not to avoid
-                            // deadlocks
-                    if (engine instanceof Lock lock) {
-                        lock.unlock();
-                    }
-                }
+                return compiledScript.get().eval(engine.getContext());
             }
             logger.debug("Executing script of rule with UID '{}'", ruleUID);
             return engine.eval(script);
         } catch (ScriptException e) {
             logger.error("Script execution of rule with UID '{}' failed: {}", ruleUID, e.getMessage(),
                     logger.isDebugEnabled() ? e : null);
-        } catch (InterruptedException e) {
-            throw new RuntimeException(e);
         }
         return null;
     }

diff --git a/.../java/org/openhab/core/automation/module/script/internal/handler/ScriptActionHandler.java b/.../java/org/openhab/core/automation/module/script/internal/handler/ScriptActionHandler.java
@@ -14,6 +14,8 @@
 
 import java.util.HashMap;
 import java.util.Map;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.Lock;
 import java.util.function.Consumer;
 
 import javax.script.ScriptException;
@@ -31,7 +33,8 @@
  *
  * @author Kai Kreuzer - Initial contribution
  * @author Simon Merschjohann - Initial contribution
- * @author Florian Hotze - Add support for script pre-compilation
+ * @author Florian Hotze - Add support for script pre-compilation, Synchronize script context access if the ScriptEngine
+ *         implements locking
  */
 @NonNullByDefault
 public class ScriptActionHandler extends AbstractScriptModuleHandler<Action> implements ActionHandler {
@@ -76,10 +79,27 @@ public void compile() throws ScriptException {
         }
 
         getScriptEngine().ifPresent(scriptEngine -> {
-            setExecutionContext(scriptEngine, context);
-            Object result = eval(scriptEngine, script);
-            resultMap.put("result", result);
-            resetExecutionContext(scriptEngine, context);
+            try {
+                if (scriptEngine instanceof Lock lock && !lock.tryLock(1, TimeUnit.MINUTES)) {
+                    logger.error(
+                            "Failed to acquire lock within one minute for script module '{}' of rule with UID '{}'",
+                            module.getId(), ruleUID);
+                    return;
+                }
+            } catch (InterruptedException e) {
+                throw new RuntimeException(e);
+            }
+            try {
+                setExecutionContext(scriptEngine, context);
+                Object result = eval(scriptEngine, script);
+                resultMap.put("result", result);
+                resetExecutionContext(scriptEngine, context);
+            } finally { // Make sure that Lock is unlocked regardless of an exception being thrown or not to avoid
+                        // deadlocks
+                if (scriptEngine instanceof Lock lock) {
+                    lock.unlock();
+                }
+            }
         });
 
         return resultMap;

diff --git a/...va/org/openhab/core/automation/module/script/internal/handler/ScriptConditionHandler.java b/...va/org/openhab/core/automation/module/script/internal/handler/ScriptConditionHandler.java
@@ -14,6 +14,8 @@
 
 import java.util.Map;
 import java.util.Optional;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.locks.Lock;
 
 import javax.script.ScriptEngine;
 import javax.script.ScriptException;
@@ -30,7 +32,8 @@
  *
  * @author Kai Kreuzer - Initial contribution
  * @author Simon Merschjohann - Initial contribution
- * @author Florian Hotze - Add support for script pre-compilation
+ * @author Florian Hotze - Add support for script pre-compilation, Synchronize script context access if the ScriptEngine
+ *         implements locking
  */
 @NonNullByDefault
 public class ScriptConditionHandler extends AbstractScriptModuleHandler<Condition> implements ConditionHandler {
@@ -60,15 +63,32 @@ public boolean isSatisfied(final Map<String, Object> context) {
 
         if (engine.isPresent()) {
             ScriptEngine scriptEngine = engine.get();
-            setExecutionContext(scriptEngine, context);
-            Object returnVal = eval(scriptEngine, script);
-            if (returnVal instanceof Boolean boolean1) {
-                result = boolean1;
-            } else {
-                logger.error("Script of rule with UID '{}' did not return a boolean value, but '{}'", ruleUID,
-                        returnVal);
+            try {
+                if (scriptEngine instanceof Lock lock && !lock.tryLock(1, TimeUnit.MINUTES)) {
+                    logger.error(
+                            "Failed to acquire lock within one minute for script module '{}' of rule with UID '{}'",
+                            module.getId(), ruleUID);
+                    return result;
+                }
+            } catch (InterruptedException e) {
+                throw new RuntimeException(e);
+            }
+            try {
+                setExecutionContext(scriptEngine, context);
+                Object returnVal = eval(scriptEngine, script);
+                if (returnVal instanceof Boolean boolean1) {
+                    result = boolean1;
+                } else {
+                    logger.error("Script of rule with UID '{}' did not return a boolean value, but '{}'", ruleUID,
+                            returnVal);
+                }
+                resetExecutionContext(scriptEngine, context);
+            } finally { // Make sure that Lock is unlocked regardless of an exception being thrown or not to avoid
+                        // deadlocks
+                if (scriptEngine instanceof Lock lock) {
+                    lock.unlock();
+                }
             }
-            resetExecutionContext(scriptEngine, context);
         }
 
         return result;