[jsscripting] Extend synchronization to common ScriptEngine methods (#13924)

* [jsscripting] Extend synchronization to common ScriptEngine methods

This extends the multi-thread synchronization to "eval" and "invokeMethod" and moves synchronization for "invokeFunction" to the DelegatingScriptEngineWithInvocableAndAutocloseableAndSynchronization class. Fixes the multi-thread access requested warnings described in the community (https://community.openhab.org/t/openhab-3-4-milestone-discussion/138093/130) and related to https://github.com/openhab/openhab-core/pull/3180.

* Revert "[jsscripting] Extend synchronization to common ScriptEngine methods"

This reverts commit aadd21e45879c10aad29bf279ddbb0afd789b0aa.

* [jsscripting] Extend synchronization to common ScriptEngine methods & Switch to ReentrantLock

This extends the multi-thread synchronization to "eval" and "invokeMethod" and moves synchronization for "invokeFunction" to the InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable class.
The synchronization mechanism changed from using synchronized to using a ReentrantLock together with catch_finally to avoid having deadlocks when an exception is thrown.
Fixes the multi-thread access requested warnings described in the community (https://community.openhab.org/t/openhab-3-4-milestone-discussion/138093/130) and related to https://github.com/openhab/openhab-core/pull/3180.

* [jsscripting] Reduce compiler warnings
* [jsscripting] Replace finally blocks & Wrap returns in afterInvocation
* [jsscripting] Fix deadlock caused by NoSuchMethodException in Invocable interface methods

During testing my latest changes, I noticed that there is a deadlock when invokeFunction or invokeMethod are called on a non-existing method.
This happens because the NoSuchMethodException keeps afterInvocation from running and therefore the lock never gets released.

* [jsscripting] Also rethrow NPE & Fix PMD warnings/errors
* [jsscripting] Wrap and rethrow other exceptions instead of returning them
* [jsscripting] Address review comment from @jpg0

Signed-off-by: Florian Hotze <florianh_dev@icloud.com>

bundles/org.openhab.automation.jsscripting/pom.xml

@@ -101,6 +101,13 @@
           </execution>
         </executions>
       </plugin>
+      <plugin>
+        <groupId>org.openhab.tools.sat</groupId>
+        <artifactId>sat-plugin</artifactId>
+        <configuration>
+          <pmdFilter>${project.basedir}/suppressions.properties</pmdFilter>
+        </configuration>
+      </plugin>
     </plugins>
   </build>
 

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<T extends ScriptEngine & Invocable & AutoClosea
     }
 
     @Override
-    public ScriptException afterThrowsInvocation(ScriptException se) {
-        Throwable cause = se.getCause();
+    public Exception afterThrowsInvocation(Exception e) {
+        Throwable cause = e.getCause();
         if (cause instanceof PolyglotException) {
             STACK_LOGGER.error("Failed to execute script:", cause);
         }
-        return se;
+        return e;
     }
 }

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/GraalJSScriptEngineFactory.java

@@ -17,6 +17,7 @@ import java.util.Map;
 
 import javax.script.ScriptEngine;
 
+import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.openhab.automation.jsscripting.internal.fs.watch.JSDependencyTracker;
 import org.openhab.core.automation.module.script.ScriptDependencyTracker;
@@ -37,6 +38,7 @@ import org.osgi.service.component.annotations.Reference;
 @Component(service = ScriptEngineFactory.class, configurationPid = "org.openhab.jsscripting", property = Constants.SERVICE_PID
         + "=org.openhab.jsscripting")
 @ConfigurableService(category = "automation", label = "JS Scripting", description_uri = "automation:jsscripting")
+@NonNullByDefault
 public final class GraalJSScriptEngineFactory implements ScriptEngineFactory {
     private static final String CFG_INJECTION_ENABLED = "injectionEnabled";
     private static final String INJECTION_CODE = "Object.assign(this, require('openhab'));";
@@ -80,7 +82,7 @@ public final class GraalJSScriptEngineFactory implements ScriptEngineFactory {
     }
 
     @Override
-    public ScriptEngine createScriptEngine(String scriptType) {
+    public @Nullable ScriptEngine createScriptEngine(String scriptType) {
         return new DebuggingGraalScriptEngine<>(
                 new OpenhabGraalJSScriptEngine(injectionEnabled ? INJECTION_CODE : null, jsScriptServiceUtil));
     }

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<String, Object> 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());
 

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);
     }
 }

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<GraalJSScriptEngine> {
@@ -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<String> scriptDependencyListener;
+    private @Nullable Consumer<String> scriptDependencyListener;
 
     private boolean initialized = false;
     private final String globalScript;
@@ -119,8 +122,9 @@ public class OpenhabGraalJSScriptEngine
                             @Override
                             public SeekableByteChannel newByteChannel(Path path, Set<? extends OpenOption> options,
                                     FileAttribute<?>... attrs) throws IOException {
-                                if (scriptDependencyListener != null) {
-                                    scriptDependencyListener.accept(path.toString());
+                                Consumer<String> 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

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;
     }

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<String, Function<String, Object>> types;
+    private Map<String, Function<String, Object>> types = new HashMap<>();
     private Map<String, Map<String, Object>> 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);
     }
 

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;
 

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<T extends ScriptEngine & Invocable & AutoCloseable>
         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<T exte
 
     @Override
     public void close() throws Exception {
-        if (Objects.nonNull(delegate))
-            delegate.close();
+        delegate.close();
     }
 }

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/scriptengine/InvocationInterceptingScriptEngineWithInvocableAndAutoCloseable.java

@@ -14,6 +14,7 @@
 package org.openhab.automation.jsscripting.internal.scriptengine;
 
 import java.io.Reader;
+import java.lang.reflect.UndeclaredThrowableException;
 
 import javax.script.Bindings;
 import javax.script.Invocable;
@@ -38,17 +39,21 @@ public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoClos
     protected void beforeInvocation() {
     }
 
-    protected ScriptException afterThrowsInvocation(ScriptException se) {
-        return se;
+    protected Object afterInvocation(Object obj) {
+        return obj;
+    }
+
+    protected Exception afterThrowsInvocation(Exception e) {
+        return e;
     }
 
     @Override
     public Object eval(String s, ScriptContext scriptContext) throws ScriptException {
         try {
             beforeInvocation();
-            return super.eval(s, scriptContext);
+            return afterInvocation(super.eval(s, scriptContext));
         } catch (ScriptException se) {
-            throw afterThrowsInvocation(se);
+            throw (ScriptException) afterThrowsInvocation(se);
         }
     }
 
@@ -56,9 +61,9 @@ public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoClos
     public Object eval(Reader reader, ScriptContext scriptContext) throws ScriptException {
         try {
             beforeInvocation();
-            return super.eval(reader, scriptContext);
+            return afterInvocation(super.eval(reader, scriptContext));
         } catch (ScriptException se) {
-            throw afterThrowsInvocation(se);
+            throw (ScriptException) afterThrowsInvocation(se);
         }
     }
 
@@ -66,9 +71,9 @@ public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoClos
     public Object eval(String s) throws ScriptException {
         try {
             beforeInvocation();
-            return super.eval(s);
+            return afterInvocation(super.eval(s));
         } catch (ScriptException se) {
-            throw afterThrowsInvocation(se);
+            throw (ScriptException) afterThrowsInvocation(se);
         }
     }
 
@@ -76,9 +81,9 @@ public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoClos
     public Object eval(Reader reader) throws ScriptException {
         try {
             beforeInvocation();
-            return super.eval(reader);
+            return afterInvocation(super.eval(reader));
         } catch (ScriptException se) {
-            throw afterThrowsInvocation(se);
+            throw (ScriptException) afterThrowsInvocation(se);
         }
     }
 
@@ -86,9 +91,9 @@ public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoClos
     public Object eval(String s, Bindings bindings) throws ScriptException {
         try {
             beforeInvocation();
-            return super.eval(s, bindings);
+            return afterInvocation(super.eval(s, bindings));
         } catch (ScriptException se) {
-            throw afterThrowsInvocation(se);
+            throw (ScriptException) afterThrowsInvocation(se);
         }
     }
 
@@ -96,29 +101,47 @@ public abstract class InvocationInterceptingScriptEngineWithInvocableAndAutoClos
     public Object eval(Reader reader, Bindings bindings) throws ScriptException {
         try {
             beforeInvocation();
-            return super.eval(reader, bindings);
+            return afterInvocation(super.eval(reader, bindings));
         } catch (ScriptException se) {
-            throw afterThrowsInvocation(se);
+            throw (ScriptException) afterThrowsInvocation(se);
         }
     }
 
     @Override
-    public Object invokeMethod(Object o, String s, Object... objects) throws ScriptException, NoSuchMethodException {
+    public Object invokeMethod(Object o, String s, Object... objects)
+            throws ScriptException, NoSuchMethodException, NullPointerException, IllegalArgumentException {
         try {
             beforeInvocation();
-            return super.invokeMethod(o, s, objects);
+            return afterInvocation(super.invokeMethod(o, s, objects));
         } catch (ScriptException se) {
-            throw afterThrowsInvocation(se);
+            throw (ScriptException) afterThrowsInvocation(se);
+        } catch (NoSuchMethodException e) { // Make sure to unlock on exceptions from Invocable.invokeMethod to avoid
+                                            // deadlocks
+            throw (NoSuchMethodException) afterThrowsInvocation(e);
+        } catch (NullPointerException e) {
+            throw (NullPointerException) afterThrowsInvocation(e);
+        } catch (IllegalArgumentException e) {
+            throw (IllegalArgumentException) afterThrowsInvocation(e);
+        } catch (Exception e) {
+            throw new UndeclaredThrowableException(afterThrowsInvocation(e)); // Wrap and rethrow other exceptions
         }
     }
 
     @Override
-    public Object invokeFunction(String s, Object... objects) throws ScriptException, NoSuchMethodException {
+    public Object invokeFunction(String s, Object... objects)
+            throws ScriptException, NoSuchMethodException, NullPointerException {
         try {
             beforeInvocation();
-            return super.invokeFunction(s, objects);
+            return afterInvocation(super.invokeFunction(s, objects));
         } catch (ScriptException se) {
-            throw afterThrowsInvocation(se);
+            throw (ScriptException) afterThrowsInvocation(se);
+        } catch (NoSuchMethodException e) { // Make sure to unlock on exceptions from Invocable.invokeFunction to avoid
+                                            // deadlocks
+            throw (NoSuchMethodException) afterThrowsInvocation(e);
+        } catch (NullPointerException e) {
+            throw (NullPointerException) afterThrowsInvocation(e);
+        } catch (Exception e) {
+            throw new UndeclaredThrowableException(afterThrowsInvocation(e)); // Wrap and rethrow other exceptions
         }
     }
 }

bundles/org.openhab.automation.jsscripting/src/main/java/org/openhab/automation/jsscripting/internal/threading/ThreadsafeSimpleRuleDelegate.java

@@ -15,6 +15,7 @@ package org.openhab.automation.jsscripting.internal.threading;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.locks.Lock;
 
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
@@ -38,7 +39,7 @@ import org.openhab.core.config.core.Configuration;
 @NonNullByDefault
 class ThreadsafeSimpleRuleDelegate implements Rule, SimpleRuleActionHandler {
 
-    private final Object lock;
+    private final Lock lock;
     private final SimpleRule delegate;
 
     /**
@@ -47,7 +48,7 @@ class ThreadsafeSimpleRuleDelegate implements Rule, SimpleRuleActionHandler {
      * @param lock rule executions will synchronize on this object
      * @param delegate the delegate to forward invocations to
      */
-    ThreadsafeSimpleRuleDelegate(Object lock, SimpleRule delegate) {
+    ThreadsafeSimpleRuleDelegate(Lock lock, SimpleRule delegate) {
         this.lock = lock;
         this.delegate = delegate;
     }
@@ -55,8 +56,11 @@ class ThreadsafeSimpleRuleDelegate implements Rule, SimpleRuleActionHandler {
     @Override
     @NonNullByDefault({})
     public Object execute(Action module, Map<String, ?> 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();
         }
     }
 

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
      *         <code>clearTimeout()</code> 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<Object> 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
      *         <code>clearInterval()</code> 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<Object> 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);

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;
     }

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