wrap ScriptEngine executions in a SafeCaller

fixes #3179 prevents scripts from being able to lock up ScriptFileWatcher permanently Signed-off-by: Cody Cutrer <cody@cutrer.us>
openhab · Nov 27, 2022 · 5e67129 · 5e67129
1 parent af37384
commit 5e67129
Showing 1 changed file with 48 additions and 29 deletions.
diff --git a/...main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java b/...main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java
@@ -36,6 +36,7 @@
 import org.openhab.core.automation.module.script.ScriptEngineFactory;
 import org.openhab.core.automation.module.script.ScriptEngineManager;
 import org.openhab.core.automation.module.script.ScriptExtensionManagerWrapper;
+import org.openhab.core.common.SafeCaller;
 import org.openhab.core.common.ThreadPoolManager;
 import org.osgi.service.component.annotations.Activate;
 import org.osgi.service.component.annotations.Component;
@@ -54,9 +55,14 @@
 @NonNullByDefault
 @Component(service = ScriptEngineManager.class)
 public class ScriptEngineManagerImpl implements ScriptEngineManager {
+    /**
+     * Timeout for scripts in milliseconds.
+     */
+    static private final long TIMEOUT = TimeUnit.SECONDS.toMillis(30);
 
     private final ScheduledExecutorService scheduler = ThreadPoolManager
             .getScheduledPool(ThreadPoolManager.THREAD_POOL_NAME_COMMON);
+    private final SafeCaller safeCaller;
 
     private final Logger logger = LoggerFactory.getLogger(ScriptEngineManagerImpl.class);
     private final Map<String, ScriptEngineContainer> loadedScriptEngineInstances = new HashMap<>();
@@ -66,8 +72,10 @@ public class ScriptEngineManagerImpl implements ScriptEngineManager {
     private final Set<FactoryChangeListener> listeners = new HashSet<>();
 
     @Activate
-    public ScriptEngineManagerImpl(final @Reference ScriptExtensionManager scriptExtensionManager) {
+    public ScriptEngineManagerImpl(final @Reference ScriptExtensionManager scriptExtensionManager,
+            final @Reference SafeCaller safeCaller) {
         this.scriptExtensionManager = scriptExtensionManager;
+        this.safeCaller = safeCaller;
     }
 
     @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC)
@@ -185,22 +193,15 @@ public void loadScript(String engineIdentifier, InputStreamReader scriptData,
                 addAttributeToScriptContext(engine, CONTEXT_KEY_DEPENDENCY_LISTENER, dependencyListener);
             }
 
-            try {
-                engine.eval(scriptData);
-                if (engine instanceof Invocable) {
-                    Invocable inv = (Invocable) engine;
-                    try {
-                        inv.invokeFunction("scriptLoaded", engineIdentifier);
-                    } catch (NoSuchMethodException e) {
-                        logger.trace("scriptLoaded() is not defined in the script: {}", engineIdentifier);
-                    }
-                } else {
-                    logger.trace("ScriptEngine does not support Invocable interface");
+            safeCall(engineIdentifier, () -> {
+                try {
+                    engine.eval(scriptData);
+                } catch (Exception ex) {
+                    logger.error("Error during evaluation of script '{}': {}", engineIdentifier, ex.getMessage());
+                    logger.debug("", ex);
                 }
-            } catch (Exception ex) {
-                logger.error("Error during evaluation of script '{}': {}", engineIdentifier, ex.getMessage());
-                logger.debug("", ex);
-            }
+            }, true);
+            callHook(engineIdentifier, engine, "scriptLoaded", true, engineIdentifier);
         }
     }
 
@@ -209,21 +210,10 @@ public void removeEngine(String engineIdentifier) {
         ScriptEngineContainer container = loadedScriptEngineInstances.remove(engineIdentifier);
         if (container != null) {
             ScriptEngine scriptEngine = container.getScriptEngine();
-            if (scriptEngine instanceof Invocable) {
-                Invocable inv = (Invocable) scriptEngine;
-                try {
-                    inv.invokeFunction("scriptUnloaded");
-                } catch (NoSuchMethodException e) {
-                    logger.trace("scriptUnloaded() is not defined in the script");
-                } catch (ScriptException ex) {
-                    logger.error("Error while executing script", ex);
-                }
-            } else {
-                logger.trace("ScriptEngine does not support Invocable interface");
-            }
+            callHook(engineIdentifier, scriptEngine, "scriptUnloaded", false);
 
             if (scriptEngine instanceof AutoCloseable) {
-                // we cannot not use ScheduledExecutorService.execute here as it might execute the task in the calling
+                // we cannot use ScheduledExecutorService.execute here as it might execute the task in the calling
                 // thread (calling ScriptEngine.close in the same thread may result in a deadlock if the ScriptEngine
                 // tries to Thread.join)
                 scheduler.schedule(() -> {
@@ -294,4 +284,33 @@ public void addFactoryChangeListener(FactoryChangeListener listener) {
     public void removeFactoryChangeListener(FactoryChangeListener listener) {
         listeners.remove(listener);
     }
+
+    private void safeCall(String engineIdentifier, Runnable r, boolean unloadOnTimeout) {
+        safeCaller.create(r, Runnable.class).withTimeout(TIMEOUT).onTimeout(() -> {
+            logger.warn("Script evaluation of '{}' takes more than {}ms", engineIdentifier, TIMEOUT);
+            if (unloadOnTimeout) {
+                removeEngine(engineIdentifier);
+            }
+        }).build().run();
+    }
+
+    private void callHook(String engineIdentifier, ScriptEngine engine, String hook, boolean unloadOnTimeout,
+            Object... args) {
+        if (!(engine instanceof Invocable)) {
+            logger.trace("ScriptEngine does not support Invocable interface");
+            return;
+        }
+
+        safeCall(engineIdentifier, () -> {
+            Invocable inv = (Invocable) engine;
+            try {
+                inv.invokeFunction(hook, args);
+            } catch (NoSuchMethodException e) {
+                logger.trace("{}() is not defined in the script '{}'", hook, engineIdentifier);
+            } catch (ScriptException ex) {
+                logger.error("Error while executing script '{}': {}", engineIdentifier, ex.getMessage());
+                logger.debug("", ex);
+            }
+        }, unloadOnTimeout);
+    }
 }