From 59b66eb3f6830a4aaede0e868379ce8bc6971944 Mon Sep 17 00:00:00 2001 From: Cody Cutrer Date: Wed, 30 Nov 2022 14:36:49 -0700 Subject: [PATCH] Wrap ScriptEngine executions in a SafeCaller (#3180) Fixes #3179 Signed-off-by: Cody Cutrer GitOrigin-RevId: ff75505e703d55a6d62a800561ccee9b39ab7ca1 --- .../internal/ScriptEngineManagerImpl.java | 77 ++++++++++++------- .../internal/ScriptEngineManagerImplTest.java | 3 +- 2 files changed, 50 insertions(+), 30 deletions(-) diff --git a/bundles/org.opensmarthouse.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java b/bundles/org.opensmarthouse.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java index fc11724d4db..48d08a249e4 100644 --- a/bundles/org.opensmarthouse.core.automation.module.script/src/main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java +++ b/bundles/org.opensmarthouse.core.automation.module.script/src/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 loadedScriptEngineInstances = new HashMap<>(); @@ -66,8 +72,10 @@ public class ScriptEngineManagerImpl implements ScriptEngineManager { private final Set 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) @@ -181,22 +189,15 @@ public void loadScript(String engineIdentifier, InputStreamReader scriptData) { } else { ScriptEngine engine = container.getScriptEngine(); - 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) { tracker.removeTracking(engineIdentifier); } 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); + } } diff --git a/bundles/org.opensmarthouse.core.automation.module.script/src/test/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImplTest.java b/bundles/org.opensmarthouse.core.automation.module.script/src/test/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImplTest.java index 01d876d1a40..00215d212ec 100644 --- a/bundles/org.opensmarthouse.core.automation.module.script/src/test/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImplTest.java +++ b/bundles/org.opensmarthouse.core.automation.module.script/src/test/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImplTest.java @@ -40,6 +40,7 @@ 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} @@ -73,7 +74,7 @@ public void setup() { when(scriptEngineFactoryMock.getDependencyTracker()).thenReturn(scriptDependencyTrackerMock); when(scriptDependencyTrackerMock.getTracker(any())).thenReturn(dependencyListenerMock); - scriptEngineManager = new ScriptEngineManagerImpl(scriptExtensionManagerMock); + scriptEngineManager = new ScriptEngineManagerImpl(scriptExtensionManagerMock, new SafeCallerImpl(null)); scriptEngineManager.addScriptEngineFactory(scriptEngineFactoryMock); }