diff --git a/bundles/org.openhab.automation.jsscripting/pom.xml b/bundles/org.openhab.automation.jsscripting/pom.xml index 6247b4058..4ea4af2c9 100644 --- a/bundles/org.openhab.automation.jsscripting/pom.xml +++ b/bundles/org.openhab.automation.jsscripting/pom.xml @@ -101,6 +101,13 @@ + + org.openhab.tools.sat + sat-plugin + + ${project.basedir}/suppressions.properties + + diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/DebuggingGraalScriptEngine.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/DebuggingGraalScriptEngine.java index 4855f02c2..3cedaead0 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/DebuggingGraalScriptEngine.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/DebuggingGraalScriptEngine.java @@ -15,7 +15,6 @@ package org.openhab.automation.jsscripting.internal; import javax.script.Invocable; import javax.script.ScriptEngine; -import javax.script.ScriptException; import org.graalvm.polyglot.PolyglotException; import org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable; @@ -38,11 +37,11 @@ class DebuggingGraalScriptEngine( new OpenhabGraalJSScriptEngine(injectionEnabled ? INJECTION_CODE : null, jsScriptServiceUtil)); } diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSRuntimeFeatures.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSRuntimeFeatures.java index ca5d30115..62f6339c3 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSRuntimeFeatures.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSRuntimeFeatures.java @@ -14,6 +14,7 @@ package org.openhab.automation.jsscripting.internal; import java.util.HashMap; import java.util.Map; +import java.util.concurrent.locks.Lock; import org.eclipse.jdt.annotation.NonNullByDefault; import org.openhab.automation.jsscripting.internal.threading.ThreadsafeTimers; @@ -31,7 +32,7 @@ public class JSRuntimeFeatures { private final Map features = new HashMap<>(); public final ThreadsafeTimers threadsafeTimers; - JSRuntimeFeatures(Object lock, JSScriptServiceUtil jsScriptServiceUtil) { + JSRuntimeFeatures(Lock lock, JSScriptServiceUtil jsScriptServiceUtil) { this.threadsafeTimers = new ThreadsafeTimers(lock, jsScriptServiceUtil.getScriptExecution(), jsScriptServiceUtil.getScheduler()); diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSScriptServiceUtil.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSScriptServiceUtil.java index 8531cacfd..9ecbf21f5 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSScriptServiceUtil.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/JSScriptServiceUtil.java @@ -12,6 +12,8 @@ */ package org.openhab.automation.jsscripting.internal; +import java.util.concurrent.locks.Lock; + import org.eclipse.jdt.annotation.NonNullByDefault; import org.openhab.core.automation.module.script.action.ScriptExecution; import org.openhab.core.scheduler.Scheduler; @@ -44,7 +46,7 @@ public class JSScriptServiceUtil { return scriptExecution; } - public JSRuntimeFeatures getJSRuntimeFeatures(Object lock) { + public JSRuntimeFeatures getJSRuntimeFeatures(Lock lock) { return new JSRuntimeFeatures(lock, this); } } 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 92dfd5b73..bd401d0c2 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 @@ -30,6 +30,8 @@ import java.time.ZonedDateTime; import java.util.Collections; import java.util.Map; import java.util.Set; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; import java.util.function.Consumer; import java.util.function.Function; @@ -58,7 +60,8 @@ 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; Inject the {@link JSRuntimeFeatures} - * into the JS context; Fix memory leak caused by HostObject by making HostAccess reference static + * into the JS context; Fix memory leak caused by HostObject by making HostAccess reference static; Switch to + * {@link Lock} for multi-thread synchronization */ public class OpenhabGraalJSScriptEngine extends InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable { @@ -83,13 +86,13 @@ public class OpenhabGraalJSScriptEngine }, HostAccess.TargetMappingPrecedence.LOW) .build(); - /** Shared lock object for synchronization of multi-thread access */ - private final Object lock = new Object(); + /** {@link Lock} synchronization of multi-thread access */ + private final Lock lock = new ReentrantLock(); private final JSRuntimeFeatures jsRuntimeFeatures; // these fields start as null because they are populated on first use private String engineIdentifier; - private Consumer scriptDependencyListener; + private @Nullable Consumer scriptDependencyListener; private boolean initialized = false; private final String globalScript; @@ -119,8 +122,9 @@ public class OpenhabGraalJSScriptEngine @Override public SeekableByteChannel newByteChannel(Path path, Set options, FileAttribute... attrs) throws IOException { - if (scriptDependencyListener != null) { - scriptDependencyListener.accept(path.toString()); + Consumer localScriptDependencyListener = scriptDependencyListener; + if (localScriptDependencyListener != null) { + localScriptDependencyListener.accept(path.toString()); } if (path.toString().endsWith(".js")) { @@ -174,6 +178,8 @@ public class OpenhabGraalJSScriptEngine @Override protected void beforeInvocation() { + lock.lock(); + if (initialized) { return; } @@ -227,11 +233,15 @@ public class OpenhabGraalJSScriptEngine } @Override - public Object invokeFunction(String s, Object... objects) throws ScriptException, NoSuchMethodException { - // Synchronize multi-thread access to avoid exceptions when reloading a script file while the script is running - synchronized (lock) { - return super.invokeFunction(s, objects); - } + protected Object afterInvocation(Object obj) { + lock.unlock(); + return obj; + } + + @Override + protected Exception afterThrowsInvocation(Exception e) { + lock.unlock(); + return e; } @Override 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 6793fbc62..da131c27f 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 @@ -16,6 +16,7 @@ import java.io.IOException; import java.util.HashMap; import java.util.Map; import java.util.Optional; +import java.util.concurrent.locks.Lock; import org.eclipse.jdt.annotation.NonNullByDefault; import org.graalvm.polyglot.Context; @@ -29,7 +30,8 @@ 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 + * @author Florian Hotze - Pass in lock object for multi-thread synchronization; Switch to {@link Lock} for multi-thread + * synchronization */ @NonNullByDefault @@ -37,11 +39,11 @@ 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 Lock lock; private final ScriptExtensionAccessor scriptExtensionAccessor; - public ScriptExtensionModuleProvider(ScriptExtensionAccessor scriptExtensionAccessor, Object lock) { + public ScriptExtensionModuleProvider(ScriptExtensionAccessor scriptExtensionAccessor, Lock lock) { this.scriptExtensionAccessor = scriptExtensionAccessor; this.lock = lock; } 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 97520e98c..537ec6324 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,7 +13,11 @@ package org.openhab.automation.jsscripting.internal.scope; -import java.util.*; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; @@ -28,7 +32,7 @@ import org.osgi.service.component.annotations.Activate; * @author Jonathan Gilbert - Initial contribution */ public abstract class AbstractScriptExtensionProvider implements ScriptExtensionProvider { - private Map> types; + private Map> types = new HashMap<>(); private Map> idToTypes = new ConcurrentHashMap<>(); protected abstract String getPresetName(); @@ -41,7 +45,7 @@ public abstract class AbstractScriptExtensionProvider implements ScriptExtension @Activate public void activate(final BundleContext context) { - types = new HashMap<>(); + types.clear(); initializeTypes(context); } 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 380f0214d..d8ae613f4 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,7 +13,11 @@ package org.openhab.automation.jsscripting.internal.scope; -import java.util.*; +import java.util.Collection; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Objects; import java.util.concurrent.ConcurrentHashMap; import java.util.function.Function; 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 f96ce621d..8b55017ac 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,7 +14,6 @@ package org.openhab.automation.jsscripting.internal.scriptengine; import java.io.Reader; -import java.util.Objects; import javax.script.Bindings; import javax.script.Invocable; @@ -23,7 +22,7 @@ import javax.script.ScriptEngine; import javax.script.ScriptEngineFactory; import javax.script.ScriptException; -import org.eclipse.jdt.annotation.Nullable; +import org.eclipse.jdt.annotation.NonNull; /** * {@link ScriptEngine} implementation that delegates to a supplied ScriptEngine instance. Allows overriding specific @@ -33,94 +32,91 @@ import org.eclipse.jdt.annotation.Nullable; */ public abstract class DelegatingScriptEngineWithInvocableAndAutocloseable implements ScriptEngine, Invocable, AutoCloseable { - protected T delegate; + protected @NonNull T delegate; - public DelegatingScriptEngineWithInvocableAndAutocloseable(T delegate) { + public DelegatingScriptEngineWithInvocableAndAutocloseable(@NonNull T delegate) { this.delegate = delegate; } @Override - public @Nullable Object eval(String s, ScriptContext scriptContext) throws ScriptException { - return Objects.nonNull(delegate) ? delegate.eval(s, scriptContext) : null; + public Object eval(String s, ScriptContext scriptContext) throws ScriptException { + return delegate.eval(s, scriptContext); } @Override - public @Nullable Object eval(Reader reader, ScriptContext scriptContext) throws ScriptException { - return Objects.nonNull(delegate) ? delegate.eval(reader, scriptContext) : null; + public Object eval(Reader reader, ScriptContext scriptContext) throws ScriptException { + return delegate.eval(reader, scriptContext); } @Override - public @Nullable Object eval(String s) throws ScriptException { - return Objects.nonNull(delegate) ? delegate.eval(s) : null; + public Object eval(String s) throws ScriptException { + return delegate.eval(s); } @Override - public @Nullable Object eval(Reader reader) throws ScriptException { - return Objects.nonNull(delegate) ? delegate.eval(reader) : null; + public Object eval(Reader reader) throws ScriptException { + return delegate.eval(reader); } @Override - public @Nullable Object eval(String s, Bindings bindings) throws ScriptException { - return Objects.nonNull(delegate) ? delegate.eval(s, bindings) : null; + public Object eval(String s, Bindings bindings) throws ScriptException { + return delegate.eval(s, bindings); } @Override - public @Nullable Object eval(Reader reader, Bindings bindings) throws ScriptException { - return Objects.nonNull(delegate) ? delegate.eval(reader, bindings) : null; + public Object eval(Reader reader, Bindings bindings) throws ScriptException { + return delegate.eval(reader, bindings); } @Override public void put(String s, Object o) { - if (Objects.nonNull(delegate)) - delegate.put(s, o); + delegate.put(s, o); } @Override - public @Nullable Object get(String s) { - return Objects.nonNull(delegate) ? delegate.get(s) : null; + public Object get(String s) { + return delegate.get(s); } @Override - public @Nullable Bindings getBindings(int i) { - return Objects.nonNull(delegate) ? delegate.getBindings(i) : null; + public Bindings getBindings(int i) { + return delegate.getBindings(i); } @Override public void setBindings(Bindings bindings, int i) { - if (Objects.nonNull(delegate)) - delegate.setBindings(bindings, i); + delegate.setBindings(bindings, i); } @Override - public @Nullable Bindings createBindings() { - return Objects.nonNull(delegate) ? delegate.createBindings() : null; + public Bindings createBindings() { + return delegate.createBindings(); } @Override - public @Nullable ScriptContext getContext() { - return Objects.nonNull(delegate) ? delegate.getContext() : null; + public ScriptContext getContext() { + return delegate.getContext(); } @Override public void setContext(ScriptContext scriptContext) { - if (Objects.nonNull(delegate)) - delegate.setContext(scriptContext); + delegate.setContext(scriptContext); } @Override - public @Nullable ScriptEngineFactory getFactory() { - return Objects.nonNull(delegate) ? delegate.getFactory() : null; + public ScriptEngineFactory getFactory() { + return delegate.getFactory(); } @Override - public @Nullable Object invokeMethod(Object o, String s, Object... objects) - throws ScriptException, NoSuchMethodException { - return Objects.nonNull(delegate) ? delegate.invokeMethod(o, s, objects) : null; + public Object invokeMethod(Object o, String s, Object... objects) + throws ScriptException, NoSuchMethodException, IllegalArgumentException { + return delegate.invokeMethod(o, s, objects); } @Override - public @Nullable Object invokeFunction(String s, Object... objects) throws ScriptException, NoSuchMethodException { - return Objects.nonNull(delegate) ? delegate.invokeFunction(s, objects) : null; + public Object invokeFunction(String s, Object... objects) throws ScriptException, NoSuchMethodException { + return delegate.invokeFunction(s, objects); } @Override @@ -135,7 +131,6 @@ public abstract class DelegatingScriptEngineWithInvocableAndAutocloseable inputs) { - synchronized (lock) { + lock.lock(); + try { return delegate.execute(module, inputs); + } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid deadlocks + lock.unlock(); } } diff --git a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeTimers.java b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeTimers.java index 513dceee4..c87bb4077 100644 --- a/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeTimers.java +++ b/bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeTimers.java @@ -19,6 +19,7 @@ import java.time.temporal.Temporal; import java.util.Map; import java.util.concurrent.ConcurrentHashMap; import java.util.concurrent.atomic.AtomicLong; +import java.util.concurrent.locks.Lock; import org.eclipse.jdt.annotation.Nullable; import org.openhab.core.automation.module.script.action.ScriptExecution; @@ -35,7 +36,7 @@ import org.openhab.core.scheduler.SchedulerTemporalAdjuster; * Threadsafe reimplementation of the timer creation methods of {@link ScriptExecution} */ public class ThreadsafeTimers { - private final Object lock; + private final Lock lock; private final Scheduler scheduler; private final ScriptExecution scriptExecution; // Mapping of positive, non-zero integer values (used as timeoutID or intervalID) and the Scheduler @@ -43,7 +44,7 @@ public class ThreadsafeTimers { private AtomicLong lastId = new AtomicLong(); private String identifier = "noIdentifier"; - public ThreadsafeTimers(Object lock, ScriptExecution scriptExecution, Scheduler scheduler) { + public ThreadsafeTimers(Lock lock, ScriptExecution scriptExecution, Scheduler scheduler) { this.lock = lock; this.scheduler = scheduler; this.scriptExecution = scriptExecution; @@ -79,8 +80,12 @@ public class ThreadsafeTimers { */ public Timer createTimer(@Nullable String identifier, ZonedDateTime instant, Runnable closure) { return scriptExecution.createTimer(identifier, instant, () -> { - synchronized (lock) { + lock.lock(); + try { closure.run(); + } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid + // deadlocks + lock.unlock(); } }); } @@ -108,12 +113,16 @@ public class ThreadsafeTimers { * @return Positive integer value which identifies the timer created; this value can be passed to * clearTimeout() to cancel the timeout. */ - public long setTimeout(Runnable callback, Long delay, Object... args) { + public long setTimeout(Runnable callback, Long delay, @Nullable Object... args) { long id = lastId.incrementAndGet(); ScheduledCompletableFuture future = scheduler.schedule(() -> { - synchronized (lock) { + lock.lock(); + try { callback.run(); idSchedulerMapping.remove(id); + } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid + // deadlocks + lock.unlock(); } }, identifier + ".timeout." + id, Instant.now().plusMillis(delay)); idSchedulerMapping.put(id, future); @@ -157,11 +166,15 @@ public class ThreadsafeTimers { * @return Numeric, non-zero value which identifies the timer created; this value can be passed to * clearInterval() to cancel the interval. */ - public long setInterval(Runnable callback, Long delay, Object... args) { + public long setInterval(Runnable callback, Long delay, @Nullable Object... args) { long id = lastId.incrementAndGet(); ScheduledCompletableFuture future = scheduler.schedule(() -> { - synchronized (lock) { + lock.lock(); + try { callback.run(); + } finally { // Make sure that Lock is unlocked regardless of an exception is thrown or not to avoid + // deadlocks + lock.unlock(); } }, identifier + ".interval." + id, new LoopingAdjuster(Duration.ofMillis(delay))); idSchedulerMapping.put(id, future); 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 6684d065e..fef38d3f8 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 @@ -13,6 +13,8 @@ package org.openhab.automation.jsscripting.internal.threading; +import java.util.concurrent.locks.Lock; + import org.eclipse.jdt.annotation.NonNullByDefault; import org.openhab.core.automation.Rule; import org.openhab.core.automation.module.script.rulesupport.shared.ScriptedAutomationManager; @@ -31,15 +33,16 @@ 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 + * @author Florian Hotze - Pass in lock object for multi-thread synchronization; Switch to {@link Lock} for multi-thread + * synchronization */ @NonNullByDefault public class ThreadsafeWrappingScriptedAutomationManagerDelegate { private ScriptedAutomationManager delegate; - private final Object lock; + private final Lock lock; - public ThreadsafeWrappingScriptedAutomationManagerDelegate(ScriptedAutomationManager delegate, Object lock) { + public ThreadsafeWrappingScriptedAutomationManagerDelegate(ScriptedAutomationManager delegate, Lock lock) { this.delegate = delegate; this.lock = lock; } diff --git a/bundles/org.openhab.automation.jsscripting/suppressions.properties b/bundles/org.openhab.automation.jsscripting/suppressions.properties new file mode 100644 index 000000000..f57fe0e51 --- /dev/null +++ b/bundles/org.openhab.automation.jsscripting/suppressions.properties @@ -0,0 +1,2 @@ +# Please check here how to add suppressions https://maven.apache.org/plugins/maven-pmd-plugin/examples/violation-exclusions.html +org.openhab.automation.jsscripting.internal.scriptengine.InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable=AvoidThrowingNullPointerException,AvoidCatchingNPE