Wrap ScriptEngine executions in a SafeCaller (openhab#3180)

Fixes openhab#3179 Signed-off-by: Cody Cutrer <cody@cutrer.us>
ConnectorIO · Nov 30, 2022 · ff75505 · ff75505
1 parent f64874e
commit ff75505
Show file tree

Hide file tree

Showing 2 changed files with 50 additions and 30 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)
@@ -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/.../java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImplTest.java b/.../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);
  }