From cf47dc0f391bd096c5dd8c5afe1c6f9ccee97065 Mon Sep 17 00:00:00 2001 From: Florian Hotze Date: Mon, 24 Oct 2022 13:35:33 +0200 Subject: [PATCH] [jsscripting] Fix multi-thread access (#13582) * [jsscripting] Share the lock mechanism that was used only for rules This change moves the lock object that was originally created for ThreadsafeSimpleRuleDelegate to OpenhabGraalJSScriptEngine to make share it across the whole engine. * [jsscripting] Inject the lock object into the JS runtime * [jsscripting] Update `setTimeout` & `setInterval` polyfills to enable threadsafety * [jsscripting] Upgrade GraalJS from 21.3.0 to 22.3.0 * [jsscripting] Reduce compiler warnings * [jsscripting] Update node version of frontend-maven-plugin Signed-off-by: Florian Hotze --- .../pom.xml | 4 +- .../internal/GraalJSScriptEngineFactory.java | 2 +- .../internal/OpenhabGraalJSScriptEngine.java | 24 ++- .../ScriptExtensionModuleProvider.java | 7 +- .../fs/watch/JSScriptFileWatcher.java | 8 +- .../AbstractScriptExtensionProvider.java | 11 +- ...tDisposalAwareScriptExtensionProvider.java | 11 +- ...ptEngineWithInvocableAndAutocloseable.java | 68 +++++---- .../ThreadsafeSimpleRuleDelegate.java | 2 +- .../internal/threading/ThreadsafeTimers.java | 137 ++++++++++++++++++ ...pingScriptedAutomationManagerDelegate.java | 6 +- .../node_modules/@jsscripting-globals.js | 7 +- 12 files changed, 222 insertions(+), 65 deletions(-) create mode 100644 bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeTimers.java diff --git a/bundles/org.openhab.automation.jsscripting/pom.xml b/bundles/org.openhab.automation.jsscripting/pom.xml index 699539f8c6..0f3f46aa70 100644 --- a/bundles/org.openhab.automation.jsscripting/pom.xml +++ b/bundles/org.openhab.automation.jsscripting/pom.xml @@ -22,7 +22,7 @@ !jdk.internal.reflect.*, !jdk.vm.ci.services - 21.3.0 + 22.3.0 6.2.1 ${project.version} openhab@2.0.4 @@ -50,7 +50,7 @@ frontend-maven-plugin 1.12.0 - v12.16.1 + v16.17.1 target/js diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineFactory.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineFactory.java index 811b884e26..a8d4675c8a 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineFactory.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineFactory.java @@ -65,7 +65,7 @@ public final class GraalJSScriptEngineFactory implements ScriptEngineFactory { @Override public void scopeValues(ScriptEngine scriptEngine, Map scopeValues) { - // noop; the are retrieved via modules, not injected + // noop; they are retrieved via modules, not injected } @Override diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java index 66f3b4837c..8a1282c8fd 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/OpenhabGraalJSScriptEngine.java @@ -36,7 +36,6 @@ import java.util.function.Function; import javax.script.ScriptContext; import javax.script.ScriptException; -import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; import org.graalvm.polyglot.Context; import org.graalvm.polyglot.Engine; @@ -47,6 +46,7 @@ import org.openhab.automation.jsscripting.internal.fs.PrefixedSeekableByteChanne import org.openhab.automation.jsscripting.internal.fs.ReadOnlySeekableByteArrayChannel; import org.openhab.automation.jsscripting.internal.fs.watch.JSDependencyTracker; import org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable; +import org.openhab.automation.jsscripting.internal.threading.ThreadsafeTimers; import org.openhab.core.automation.module.script.ScriptExtensionAccessor; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -58,6 +58,7 @@ import com.oracle.truffle.js.scriptengine.GraalJSScriptEngine; * * @author Jonathan Gilbert - Initial contribution * @author Dan Cunningham - Script injections + * @author Florian Hotze - Create lock object for multi-thread synchronization */ public class OpenhabGraalJSScriptEngine extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable { @@ -68,9 +69,12 @@ public class OpenhabGraalJSScriptEngine // final CommonJS search path for our library private static final Path NODE_DIR = Paths.get("node_modules"); + // shared lock object for synchronization of multi-thread access + private final Object lock = new Object(); + // these fields start as null because they are populated on first use - private @NonNullByDefault({}) String engineIdentifier; - private @NonNullByDefault({}) Consumer scriptDependencyListener; + private String engineIdentifier; + private Consumer scriptDependencyListener; private boolean initialized = false; private String globalScript; @@ -83,6 +87,8 @@ public class OpenhabGraalJSScriptEngine super(null); // delegate depends on fields not yet initialised, so we cannot set it immediately this.globalScript = GLOBAL_REQUIRE + (injectionCode != null ? injectionCode : ""); + LOGGER.debug("Initializing GraalJS script engine..."); + // Custom translate JS Objects - > Java Objects HostAccess hostAccess = HostAccess.newBuilder(HostAccess.ALL) // Translate JS-Joda ZonedDateTime to java.time.ZonedDateTime @@ -194,14 +200,16 @@ public class OpenhabGraalJSScriptEngine } ScriptExtensionModuleProvider scriptExtensionModuleProvider = new ScriptExtensionModuleProvider( - scriptExtensionAccessor); + scriptExtensionAccessor, lock); Function, Function> wrapRequireFn = originalRequireFn -> moduleName -> scriptExtensionModuleProvider .locatorFor(delegate.getPolyglotContext(), engineIdentifier).locateModule(moduleName) .map(m -> (Object) m).orElseGet(() -> originalRequireFn.apply(new Object[] { moduleName })); delegate.getBindings(ScriptContext.ENGINE_SCOPE).put(REQUIRE_WRAPPER_NAME, wrapRequireFn); + // Injections into the JS runtime delegate.put("require", wrapRequireFn.apply((Function) delegate.get("require"))); + delegate.put("ThreadsafeTimers", new ThreadsafeTimers(lock)); initialized = true; @@ -215,8 +223,8 @@ public class OpenhabGraalJSScriptEngine /** * Tests if this is a root node directory, `/node_modules`, `C:\node_modules`, etc... * - * @param path - * @return + * @param path a root path + * @return whether the given path is a node root directory */ private boolean isRootNodePath(Path path) { return path.startsWith(path.getRoot().resolve(NODE_DIR)); @@ -226,8 +234,8 @@ public class OpenhabGraalJSScriptEngine * Converts a root node path to a class resource path for loading local modules * Ex: C:\node_modules\foo.js -> /node_modules/foo.js * - * @param path - * @return + * @param path a root path, e.g. C:\node_modules\foo.js + * @return the class resource path for loading local modules */ private String nodeFileToResource(Path path) { return "/" + path.subpath(0, path.getNameCount()).toString().replace('\\', '/'); diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/ScriptExtensionModuleProvider.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/ScriptExtensionModuleProvider.java index c85077c538..6793fbc627 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/ScriptExtensionModuleProvider.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/ScriptExtensionModuleProvider.java @@ -29,6 +29,7 @@ import org.openhab.core.automation.module.script.rulesupport.shared.ScriptedAuto * Class providing script extensions via CommonJS modules. * * @author Jonathan Gilbert - Initial contribution + * @author Florian Hotze - Pass in lock object for multi-thread synchronization */ @NonNullByDefault @@ -36,11 +37,13 @@ public class ScriptExtensionModuleProvider { private static final String RUNTIME_MODULE_PREFIX = "@runtime"; private static final String DEFAULT_MODULE_NAME = "Defaults"; + private final Object lock; private final ScriptExtensionAccessor scriptExtensionAccessor; - public ScriptExtensionModuleProvider(ScriptExtensionAccessor scriptExtensionAccessor) { + public ScriptExtensionModuleProvider(ScriptExtensionAccessor scriptExtensionAccessor, Object lock) { this.scriptExtensionAccessor = scriptExtensionAccessor; + this.lock = lock; } public ModuleLocator locatorFor(Context ctx, String engineIdentifier) { @@ -94,7 +97,7 @@ public class ScriptExtensionModuleProvider { for (Map.Entry entry : rv.entrySet()) { if (entry.getValue() instanceof ScriptedAutomationManager) { entry.setValue(new ThreadsafeWrappingScriptedAutomationManagerDelegate( - (ScriptedAutomationManager) entry.getValue())); + (ScriptedAutomationManager) entry.getValue(), lock)); } } diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/fs/watch/JSScriptFileWatcher.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/fs/watch/JSScriptFileWatcher.java index 3b728cc1f3..2285c26903 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/fs/watch/JSScriptFileWatcher.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/fs/watch/JSScriptFileWatcher.java @@ -15,6 +15,7 @@ package org.openhab.automation.jsscripting.internal.fs.watch; import java.io.File; import java.nio.file.Path; import java.nio.file.WatchEvent; +import java.util.Objects; import java.util.Optional; import org.eclipse.jdt.annotation.Nullable; @@ -31,7 +32,6 @@ import org.openhab.core.service.ReadyService; */ public class JSScriptFileWatcher extends ScriptFileWatcher { private static final String FILE_DIRECTORY = "automation" + File.separator + "js"; - private static final String IGNORE_DIR_NAME = "node_modules"; private final String ignorePath; @@ -45,8 +45,10 @@ public class JSScriptFileWatcher extends ScriptFileWatcher { @Override protected void processWatchEvent(@Nullable WatchEvent event, WatchEvent.@Nullable Kind kind, @Nullable Path path) { - if (!path.startsWith(ignorePath)) { - super.processWatchEvent(event, kind, path); + if (Objects.nonNull(path)) { + if (!path.startsWith(ignorePath)) { + super.processWatchEvent(event, kind, path); + } } } diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/AbstractScriptExtensionProvider.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/AbstractScriptExtensionProvider.java index 3f1d3cbe6c..97520e98cc 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/AbstractScriptExtensionProvider.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/AbstractScriptExtensionProvider.java @@ -13,13 +13,11 @@ package org.openhab.automation.jsscripting.internal.scope; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; +import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; +import org.eclipse.jdt.annotation.Nullable; import org.openhab.core.automation.module.script.ScriptExtensionProvider; import org.osgi.framework.BundleContext; import org.osgi.service.component.annotations.Activate; @@ -63,10 +61,11 @@ public abstract class AbstractScriptExtensionProvider implements ScriptExtension } @Override - public Object get(String scriptIdentifier, String type) throws IllegalArgumentException { + public @Nullable Object get(String scriptIdentifier, String type) throws IllegalArgumentException { Map forScript = idToTypes.computeIfAbsent(scriptIdentifier, k -> new HashMap<>()); - return forScript.computeIfAbsent(type, k -> types.get(k).apply(scriptIdentifier)); + return forScript.computeIfAbsent(type, + k -> Objects.nonNull(types.get(k)) ? types.get(k).apply(scriptIdentifier) : null); } @Override diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/ScriptDisposalAwareScriptExtensionProvider.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/ScriptDisposalAwareScriptExtensionProvider.java index 2a708774f6..380f0214d7 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/ScriptDisposalAwareScriptExtensionProvider.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scope/ScriptDisposalAwareScriptExtensionProvider.java @@ -13,13 +13,11 @@ package org.openhab.automation.jsscripting.internal.scope; -import java.util.Collection; -import java.util.Collections; -import java.util.HashMap; -import java.util.Map; +import java.util.*; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; +import org.eclipse.jdt.annotation.Nullable; import org.openhab.core.automation.module.script.ScriptExtensionProvider; import org.osgi.framework.BundleContext; import org.osgi.service.component.annotations.Activate; @@ -64,10 +62,11 @@ public abstract class ScriptDisposalAwareScriptExtensionProvider } @Override - public Object get(String scriptIdentifier, String type) throws IllegalArgumentException { + public @Nullable Object get(String scriptIdentifier, String type) throws IllegalArgumentException { Map forScript = idToTypes.computeIfAbsent(scriptIdentifier, k -> new HashMap<>()); - return forScript.computeIfAbsent(type, k -> types.get(k).apply(scriptIdentifier)); + return forScript.computeIfAbsent(type, + k -> Objects.nonNull(types.get(k)) ? types.get(k).apply(scriptIdentifier) : null); } @Override diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseable.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseable.java index 0e0971284b..f96ce621db 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseable.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/DelegatingScriptEngineWithInvocableAndAutocloseable.java @@ -14,6 +14,7 @@ package org.openhab.automation.jsscripting.internal.scriptengine; import java.io.Reader; +import java.util.Objects; import javax.script.Bindings; import javax.script.Invocable; @@ -22,6 +23,8 @@ import javax.script.ScriptEngine; import javax.script.ScriptEngineFactory; import javax.script.ScriptException; +import org.eclipse.jdt.annotation.Nullable; + /** * {@link ScriptEngine} implementation that delegates to a supplied ScriptEngine instance. Allows overriding specific * methods. @@ -37,83 +40,87 @@ public abstract class DelegatingScriptEngineWithInvocableAndAutocloseable { + synchronized (lock) { + callable.run(); + } + + }, identifier); + } + + public Timer createTimerWithArgument(ZonedDateTime instant, Object arg1, Runnable callable) { + return createTimerWithArgument(null, instant, arg1, callable); + } + + public Timer createTimerWithArgument(@Nullable String identifier, ZonedDateTime instant, Object arg1, + Runnable callable) { + Scheduler scheduler = ScriptServiceUtil.getScheduler(); + return new TimerImpl(scheduler, instant, () -> { + synchronized (lock) { + callable.run(); + } + + }, identifier); + } + + /** + * This is an implementation of the {@link Timer} interface. + * Copy of {@link org.openhab.core.model.script.internal.actions.TimerImpl} as this is not accessible from outside + * the + * package. + * + * @author Kai Kreuzer - Initial contribution + */ + @NonNullByDefault + public static class TimerImpl implements Timer { + + private final Scheduler scheduler; + private final ZonedDateTime startTime; + private final SchedulerRunnable runnable; + private final @Nullable String identifier; + private ScheduledCompletableFuture future; + + public TimerImpl(Scheduler scheduler, ZonedDateTime startTime, SchedulerRunnable runnable) { + this(scheduler, startTime, runnable, null); + } + + public TimerImpl(Scheduler scheduler, ZonedDateTime startTime, SchedulerRunnable runnable, + @Nullable String identifier) { + this.scheduler = scheduler; + this.startTime = startTime; + this.runnable = runnable; + this.identifier = identifier; + + future = scheduler.schedule(runnable, identifier, startTime.toInstant()); + } + + @Override + public boolean cancel() { + return future.cancel(true); + } + + @Override + public synchronized boolean reschedule(ZonedDateTime newTime) { + future.cancel(false); + future = scheduler.schedule(runnable, identifier, newTime.toInstant()); + return true; + } + + @Override + public @Nullable ZonedDateTime getExecutionTime() { + return future.isCancelled() ? null : ZonedDateTime.now().plusNanos(future.getDelay(TimeUnit.NANOSECONDS)); + } + + @Override + public boolean isActive() { + return !future.isDone(); + } + + @Override + public boolean isCancelled() { + return future.isCancelled(); + } + + @Override + public boolean isRunning() { + return isActive() && ZonedDateTime.now().isAfter(startTime); + } + + @Override + public boolean hasTerminated() { + return future.isDone(); + } + } +} diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeWrappingScriptedAutomationManagerDelegate.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeWrappingScriptedAutomationManagerDelegate.java index 2e8e31ade7..6684d065e8 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeWrappingScriptedAutomationManagerDelegate.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeWrappingScriptedAutomationManagerDelegate.java @@ -31,15 +31,17 @@ import org.openhab.core.automation.type.TriggerType; * instance of this class that they are registered with. * * @author Jonathan Gilbert - Initial contribution + * @author Florian Hotze - Pass in lock object for multi-thread synchronization */ @NonNullByDefault public class ThreadsafeWrappingScriptedAutomationManagerDelegate { private ScriptedAutomationManager delegate; - private Object lock = new Object(); + private final Object lock; - public ThreadsafeWrappingScriptedAutomationManagerDelegate(ScriptedAutomationManager delegate) { + public ThreadsafeWrappingScriptedAutomationManagerDelegate(ScriptedAutomationManager delegate, Object lock) { this.delegate = delegate; + this.lock = lock; } public void removeModuleType(String UID) { diff --git a/bundles/org.openhab.automation.jsscripting/src/main/resources/node_modules/@jsscripting-globals.js b/bundles/org.openhab.automation.jsscripting/src/main/resources/node_modules/@jsscripting-globals.js index f956729ad6..1972efcff4 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/resources/node_modules/@jsscripting-globals.js +++ b/bundles/org.openhab.automation.jsscripting/src/main/resources/node_modules/@jsscripting-globals.js @@ -2,10 +2,9 @@ (function (global) { 'use strict'; - //Append the script file name OR rule UID depending on which is available + // Append the script file name OR rule UID depending on which is available const defaultLoggerName = "org.openhab.automation.script" + (globalThis["javax.script.filename"] ? ".file." + globalThis["javax.script.filename"].replace(/^.*[\\\/]/, '') : globalThis["ruleUID"] ? ".ui." + globalThis["ruleUID"] : ""); const System = Java.type('java.lang.System'); - const ScriptExecution = Java.type('org.openhab.core.model.script.actions.ScriptExecution'); const ZonedDateTime = Java.type('java.time.ZonedDateTime'); const formatRegExp = /%[sdj%]/g; @@ -173,7 +172,7 @@ function setTimeout(cb, delay) { const args = Array.prototype.slice.call(arguments, 2); - return ScriptExecution.createTimerWithArgument( + return ThreadsafeTimers.createTimerWithArgument( ZonedDateTime.now().plusNanos(delay * 1000000), args, function (args) { @@ -191,7 +190,7 @@ function setInterval(cb, delay) { const args = Array.prototype.slice.call(arguments, 2); const delayNanos = delay * 1000000 - let timer = ScriptExecution.createTimerWithArgument( + let timer = ThreadsafeTimers.createTimerWithArgument( ZonedDateTime.now().plusNanos(delayNanos), args, function (args) { -- 2.47.3