Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wrap ScriptEngine executions in a SafeCaller #3180

Merged
merged 2 commits into from
Nov 30, 2022
Merged

Wrap ScriptEngine executions in a SafeCaller #3180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
829c150
wrap ScriptEngine executions in a SafeCaller
ccutrer Nov 27, 2022
0e00687
just use SafeCallerImpl directly, instead of trying to mock it out
ccutrer Nov 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 48 additions & 29 deletions ...main/java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<>();
Expand All @@ -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)
Expand Down Expand Up @@ -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);
}
}

Expand All @@ -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(() -> {
Expand Down Expand Up @@ -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);
}
}
3 changes: 2 additions & 1 deletion .../java/org/openhab/core/automation/module/script/internal/ScriptEngineManagerImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}
Expand Down Expand Up @@ -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);
}

Expand Down