diff --git a/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/handler/AbstractScriptModuleHandler.java b/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/handler/AbstractScriptModuleHandler.java index beb2724d60a..81f83748a1d 100644 --- a/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/handler/AbstractScriptModuleHandler.java +++ b/bundles/org.openhab.core.automation.module.script/src/main/java/org/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. + *

+ * 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 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/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/handler/ScriptActionHandler.java b/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/handler/ScriptActionHandler.java index a170683b881..a0019dc4227 100644 --- a/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/handler/ScriptActionHandler.java +++ b/bundles/org.openhab.core.automation.module.script/src/main/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 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/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/handler/ScriptConditionHandler.java b/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/handler/ScriptConditionHandler.java index a51ad384195..28646d0de43 100644 --- a/bundles/org.openhab.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/handler/ScriptConditionHandler.java +++ b/bundles/org.openhab.core.automation.module.script/src/main/java/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 implements ConditionHandler { @@ -60,15 +63,32 @@ public boolean isSatisfied(final Map 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;