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 699539f8c..0f3f46aa7 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 811b884e2..a8d4675c8 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 66f3b4837..8a1282c8f 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 c85077c53..6793fbc62 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 3b728cc1f..2285c2690 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 3f1d3cbe6..97520e98c 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 2a708774f..380f0214d 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 0e0971284..f96ce621d 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 2e8e31ade..6684d065e 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 f956729ad..1972efcff 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) {