Revert "Wrap ScriptEngine executions in a SafeCaller (openhab#3180)"

This reverts commit ff75505. Signed-off-by: Cody Cutrer <cody@cutrer.us>
ccutrer · Dec 13, 2022 · c6b73c2 · c6b73c2
1 parent d4ea62b
commit c6b73c2
Show file tree

Hide file tree

Showing 2 changed files with 30 additions and 50 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,7 +36,6 @@
 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;
@@ -55,14 +54,9 @@
 @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<>();
@@ -72,10 +66,8 @@ public class ScriptEngineManagerImpl implements ScriptEngineManager {
     private final Set<FactoryChangeListener> listeners = new HashSet<>();
 
     @Activate
-    public ScriptEngineManagerImpl(final @Reference ScriptExtensionManager scriptExtensionManager,
-            final @Reference SafeCaller safeCaller) {
+    public ScriptEngineManagerImpl(final @Reference ScriptExtensionManager scriptExtensionManager) {
         this.scriptExtensionManager = scriptExtensionManager;
-        this.safeCaller = safeCaller;
     }
 
     @Reference(cardinality = ReferenceCardinality.MULTIPLE, policy = ReferencePolicy.DYNAMIC)
@@ -189,15 +181,22 @@ public void loadScript(String engineIdentifier, InputStreamReader scriptData) {
         } else {
             ScriptEngine engine = container.getScriptEngine();
 
-            safeCall(engineIdentifier, () -> {
-                try {
-                    engine.eval(scriptData);
-                } catch (Exception ex) {
-                    logger.error("Error during evaluation of script '{}': {}", engineIdentifier, ex.getMessage());
-                    logger.debug("", ex);
+            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");
                 }
-            }, true);
-            callHook(engineIdentifier, engine, "scriptLoaded", true, engineIdentifier);
+            } catch (Exception ex) {
+                logger.error("Error during evaluation of script '{}': {}", engineIdentifier, ex.getMessage());
+                logger.debug("", ex);
+            }
         }
     }
 
@@ -210,10 +209,21 @@ public void removeEngine(String engineIdentifier) {
                 tracker.removeTracking(engineIdentifier);
             }
             ScriptEngine scriptEngine = container.getScriptEngine();
-            callHook(engineIdentifier, scriptEngine, "scriptUnloaded", false);
+            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");
+            }
 
             if (scriptEngine instanceof AutoCloseable) {
-                // we cannot use ScheduledExecutorService.execute here as it might execute the task in the calling
+                // we cannot not 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(() -> {
@@ -284,33 +294,4 @@ 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);
-    }
 }
diff --git a/.../java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImplTest.java b/.../java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImplTest.java
@@ -40,7 +40,6 @@
 import org.mockito.quality.Strictness;
 import org.openhab.core.automation.module.script.ScriptDependencyTracker;
 import org.openhab.core.automation.module.script.ScriptEngineFactory;
-import org.openhab.core.internal.common.SafeCallerImpl;
 
 /**
  * The {@link ScriptEngineManagerImplTest} is a test class for the {@link ScriptEngineManagerImpl}
@@ -74,7 +73,7 @@ public void setup() {
         when(scriptEngineFactoryMock.getDependencyTracker()).thenReturn(scriptDependencyTrackerMock);
         when(scriptDependencyTrackerMock.getTracker(any())).thenReturn(dependencyListenerMock);
 
-        scriptEngineManager = new ScriptEngineManagerImpl(scriptExtensionManagerMock, new SafeCallerImpl(null));
+        scriptEngineManager = new ScriptEngineManagerImpl(scriptExtensionManagerMock);
         scriptEngineManager.addScriptEngineFactory(scriptEngineFactoryMock);
     }